WIP : InfluxDB support #450 #1058
witten
Reference in New Issue
Block a user
Delete Branch "gautamaggarwal2810/borgmatic:issue999"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is first draft for issue #450.
This looks like a great start! I haven't looked at tests yet, but I wanted to get you some feedback sooner rather than later. Let me know if you have any questions.
@@ -1181,6 +1181,81 @@ properties:description: |Support for the "borgmatic bootstrap" action, used to extractborgmatic configuration files from a backup archive.influxdb_databases:I would recommend moving this schema section to the bottom of the existing schema for databases. My thinking is that since this is the last database added, it's less of a major database (for borgmatic users) than the existing ones like PostgreSQL.
@@ -1184,0 +1192,4 @@influx_command:type: stringdescription: |The InfluxDB executable to use. Defaults to `influx`.I think the convention in schema descriptions for this kind of thing is double quotes (e.g.
"influx").@@ -1184,0 +1196,4 @@token:type: stringdescription: |Token to authenticate request. Required to use InfluxDB.Each of these need examples (e.g.
example: ...) so that they show up properly in generated configuration. You can tryborgmatic config generate -d test.yamlto verify.@@ -1184,0 +1201,4 @@type: stringdescription: |HTTP address of InfluxDB. Defaults to http://127.0.0.1:8086.example: https://example.com:8086Other database hooks tend to use
hostnameto mean a bare hostname (e.g.example.com) and then have a separateportoption for any port. And in fact I think you'll need something like that so thatborgmatic restoreworks with this.The main downside I can see of this is that there wouldn't be a way to specify
httpvs.https. Maybe a separatetlsboolean option would make sense here? But then that might need to be plumbed through torestoreas well...I dunno, let me know your thoughts on this. I'm not super familiar with InfluxDB, so maybe giving connection strings as URLs is standard.
Yes giving connection strings as URL is standard in Influxdb. But it would diverge from conventions used for other database hooks and can cause confusion for users familiar with other hooks.
If consistency with other hooks is a priority, we can take up hostname,port and tls approach.
But if Influxdb users are more accustomed to connection strings, we can retain URL approach but document clearly to avoid confusion.
So how should I proceed on this??
My thinking is that consistency with other hooks should be the priority here, not just because of consistency for consistency's sake, but for the very practical reason that a bunch of the existing database code assumes there's a hostname, port, etc. and it would actually be more work to not have those.
@@ -1184,0 +1202,4 @@description: |HTTP address of InfluxDB. Defaults to http://127.0.0.1:8086.example: https://example.com:8086org_id:borgmatic option names tend towards the super verbose and spelled out. So calling this
organization_idmight be more in keeping with that convention. (Usuallyiditself isn't expanded because it's so ubiquitous.)@@ -1184,0 +1206,4 @@type: numberdescription: |The ID of the organization.org_name:Similar here.
@@ -1184,0 +1209,4 @@org_name:type: stringdescription: |The name of the organization.Should probably mention that it's mutually exclutive with
org_id.@@ -1184,0 +1217,4 @@bucket_id:type: numberdescription: |The ID of the bucket to backup.IMO this should mention that this option is mutually exclusive with
bucket_name.@@ -1184,0 +1218,4 @@type: numberdescription: |The ID of the bucket to backup.configs_path:Maybe
configs_path->configurations_path?@@ -1184,0 +1222,4 @@type: stringdescription: |Path to the influx CLI configurations.active_config:Similar here.. Maybe
active_config->active_configuration.@@ -1184,0 +1229,4 @@restore_bucket:type: stringdescription: |New name to use for the restored bucket.It might be good to add similar
restore_*option name variants for other options as well. For instance, many database hooks have options likerestore_hostname,restore_port, etc.@@ -1184,0 +1230,4 @@type: stringdescription: |New name to use for the restored bucket.new_org:And I recommend calling this
restore_orgor better yetrestore_organization.@@ -1184,0 +1246,4 @@full:type: booleandescription: |Fully restore and replace all data on server.How does this work exactly? Does it ignore the
restore_bucketand restore all buckets? Or something else?@@ -0,0 +23,4 @@def use_streaming(databases, config):'''Return whether dump streaming is used for this hook. (Spoiler: It isn't.)This comment should be updated so it's consistent with what the function does!
@@ -0,0 +40,4 @@Dump the given InfluxDB databases to a named pipe. The databases are supplied as a sequence ofdicts, one dict describing each database as per the configuration schema. Use the borgmaticruntime directory to construct the destination path (used for the directory format and the givenlog prefix in any log entries.Missing close parenthesis.
@@ -0,0 +58,4 @@make_dump_path(borgmatic_runtime_directory),name,database.get('hostname'),database.get('port'),It doesn't look like
portis actually in the schema—although see above for idea about how it could be!@@ -0,0 +71,4 @@dump.create_named_pipe_for_dump(dump_filename)try:execute_command(command, shell=True, run_to_completion=False)I think you can probably safely omit
shell=True, because I don't see anything in the command building below that relies on shell features (redirection, etc.)@@ -0,0 +73,4 @@try:execute_command(command, shell=True, run_to_completion=False)except subprocess.CalledProcessError as error:logger.error(f"Command failed: {error}")Feel free to disagree, but I don't feel like this kind of catch-and-log-and-raise adds a ton of value, especially since it's just displaying the error—which presumably borgmatic already does higher up in the call stack...?
@@ -0,0 +80,4 @@patterns.append(borgmatic.borg.pattern.Pattern(os.path.join(borgmatic_runtime_directory, 'influxdb_databases'))I think this needs:
... added in, so that downstream hooks like filesystem snapshotting work properly.
@@ -0,0 +90,4 @@'''Return the backup command.'''hostname = database.get('dump_hostname', database.get('hostname')) # Prioritize 'dump_hostname'It doesn't look like
dump_hostnameis in the schema...? Was it intended to be?@@ -0,0 +164,4 @@dry_run_label = ' (dry run; not actually restoring anything)' if dry_run else ''dump_filename = dump.make_data_source_dump_filename(make_dump_path(borgmatic_runtime_directory),data_source['name'],I didn't actually see a
nameoption in the schema. There probably needs to be one there for the rest of the database machinery to work as well...?@@ -0,0 +165,4 @@dump_filename = dump.make_data_source_dump_filename(make_dump_path(borgmatic_runtime_directory),data_source['name'],data_source.get('hostname'),Probably needs
portas well if it's there in themake_data_source_dump_filename()call indump_data_sources(). Otherwise the filenames won't match.Thanks for your feedback @witten. I will make the suggested changes. I am sorry that my response is a bit late but I had a very hectic week at university, so couldnt start before.
Hey no worries.. School comes first!
Just checking in.. Please let me know if you plan to get back to this. Thanks!
@gautamaggarwal2810 Do you want me to implement the suggested changes here? I would love to see this feature released.
As far as I'm concerned, abandoned PRs are fair game for you to pick up and run with (or rewrite). I totally understand that people get busy with real life.
Tried to continue this PR on #1142
Closing this due to the continued PR at #1142. Please follow along there. And big thanks to @gautamaggarwal2810 for getting this ball rolling!
Pull request closed