-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add configuring bucket-name from args for RO commands #589 #590
base: master
Are you sure you want to change the base?
Changes from 7 commits
541401c
64105b3
af5072a
4a19bb0
88e712f
f0a7875
8bb8235
ac4b33d
f456293
8196785
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,8 @@ | |
|
||
|
||
def orchestrate(config, backup_name, seed_target, temp_dir, host_list, keep_auth, bypass_checks, verify, keyspaces, | ||
tables, parallel_restores, use_sstableloader=False, version_target=None, ignore_racks=False): | ||
tables, parallel_restores, use_sstableloader=False, version_target=None, ignore_racks=False, | ||
bucket_name=None, prefix=None): | ||
monitoring = Monitoring(config=config.monitoring) | ||
try: | ||
restore_start_time = datetime.datetime.now() | ||
|
@@ -57,7 +58,7 @@ def orchestrate(config, backup_name, seed_target, temp_dir, host_list, keep_auth | |
logging.error(err_msg) | ||
raise RuntimeError(err_msg) | ||
|
||
storage = Storage(config=config.storage) | ||
storage = Storage(config=config.storage, bucket_name=bucket_name, prefix=prefix) | ||
|
||
try: | ||
cluster_backup = storage.get_cluster_backup(backup_name) | ||
|
@@ -68,7 +69,7 @@ def orchestrate(config, backup_name, seed_target, temp_dir, host_list, keep_auth | |
|
||
restore = RestoreJob(cluster_backup, config, temp_dir, host_list, seed_target, keep_auth, verify, | ||
parallel_restores, keyspaces, tables, bypass_checks, use_sstableloader, version_target, | ||
ignore_racks) | ||
ignore_racks, bucket_name, prefix) | ||
restore.execute() | ||
|
||
restore_end_time = datetime.datetime.now() | ||
|
@@ -103,7 +104,7 @@ class RestoreJob(object): | |
|
||
def __init__(self, cluster_backup, config, temp_dir, host_list, seed_target, keep_auth, verify, | ||
parallel_restores, keyspaces=None, tables=None, bypass_checks=False, use_sstableloader=False, | ||
version_target=None, ignore_racks=False): | ||
version_target=None, ignore_racks=False, bucket_name=None, prefix=None): | ||
self.id = uuid.uuid4() | ||
self.ringmap = None | ||
self.cluster_backup = cluster_backup | ||
|
@@ -129,6 +130,8 @@ def __init__(self, cluster_backup, config, temp_dir, host_list, seed_target, kee | |
self.fqdn_resolver = HostnameResolver(fqdn_resolver, k8s_mode) | ||
self._version_target = version_target | ||
self.ignore_racks = ignore_racks | ||
self.bucket_name = bucket_name | ||
self.prefix = prefix | ||
|
||
def prepare_restore(self): | ||
logging.info('Ensuring the backup is found and is complete') | ||
|
@@ -427,7 +430,7 @@ def _build_restore_cmd(self): | |
command = 'mkdir -p {work}; cd {work} && medusa-wrapper {sudo} medusa {config} ' \ | ||
'--fqdn=%s -vvv restore-node ' \ | ||
'{in_place} {keep_auth} %s {verify} --backup-name {backup} --temp-dir {temp_dir} ' \ | ||
'{use_sstableloader} {keyspaces} {tables}' \ | ||
'{use_sstableloader} {keyspaces} {tables} {bucket_name} {prefix}' \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: following up on my other comment, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, like this:
|
||
.format(work=self.work_dir, | ||
sudo='sudo' if medusa.utils.evaluate_boolean(self.config.cassandra.use_sudo) else '', | ||
config=f'--config-file {self.config.file_path}' if self.config.file_path else '', | ||
|
@@ -438,7 +441,9 @@ def _build_restore_cmd(self): | |
temp_dir=self.temp_dir, | ||
use_sstableloader='--use-sstableloader' if self.use_sstableloader else '', | ||
keyspaces=keyspace_options, | ||
tables=table_options) | ||
tables=table_options, | ||
bucket_name=f'--bucket-name {self.bucket_name}' if self.bucket_name is not None else '', | ||
prefix=f'--prefix {self.prefix}' if self.prefix is not None else '') | ||
|
||
logging.debug('Preparing to restore on all nodes with the following command: {}'.format(command)) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: these two new arguments are creating confusion with the ones defined in the global settings (the ones that will be placed before the command name in the command line).
We'd rather need to remove these and expose both the bucket_name and the prefix as is in the storage object.
Then you can use them in your code through the storage object instead of having to pass it around the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly your suggestion, I should move bucket_name and prefix into the storage config.
Here:
StorageConfig = collections.namedtuple( 'StorageConfig', ['bucket_name', 'key_file', 'prefix', 'fqdn', 'host_file_separator', 'storage_provider', 'base_path', 'max_backup_age', 'max_backup_count', 'api_profile', 'transfer_max_bandwidth', 'concurrent_transfers', 'multi_part_upload_threshold', 'host', 'region', 'port', 'secure', 'aws_cli_path', 'kms_id', 'backup_grace_period_in_days', 'use_sudo_for_restore', 'k8s_mode'] )
If yes, I had another idea in this PR. Changing config everywhere for a single restore is overkill, in the config, we have a configuration for long-term usages(like a regular backup), and changing the config for restore is a potential problem for the backup procedure(you should return config setting back, you should don't have conflicts between operations, etc).