-
Notifications
You must be signed in to change notification settings - Fork 197
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
Fix tags not being applied on AWS EBS Snapshots #1007
base: master
Are you sure you want to change the base?
Conversation
7cabf0f
to
e5995e9
Compare
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.
Tested alongside #1005 and is working as expected!
ping |
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.
Thanks for the patch. There's a small fix which needs to be addressed as it's currently breaking snapshot backups via standard barman server.
Besides that, this looks good in general. I would just suggest a few things so it gets more in accordance with the standards:
- Write a commit body message detailing the issue a little bit more.
- Run
black
on the changed files to have them well-formatted.
The code works as expected when run with the barman-cloud-backup
script:
- Using the modified code.
$ which barman-cloud-backup
~/barman/venv/bin/barman-cloud-backup
- Do a snapshot backup with
barman-cloud-backup
passing some tags.
$ barman-cloud-backup -h 127.0.0.1 -d postgres -U postgres --snapshot-instance=test-snapshot-tags --snapshot-disk=test-snapshot-tags-non-root --tags environment,production project,my-project service,barman --aws-region us-east-1 s3://barman-testing-snapshot-tags pg-server
- The tags are available in the snapshot created.
$ aws ec2 describe-snapshots --owner <account-id>
{
"Snapshots": [
{
"Tags": [
{
"Key": "Name",
"Value": "test-snapshot-tags-non-root-20241022t124114"
},
{
"Key": "environment",
"Value": "production"
},
{
"Key": "project",
"Value": "my-project"
},
{
"Key": "service",
"Value": "barman"
}
],
"StorageTier": "standard",
"SnapshotId": "snap-08ac5e6b84264cdd8",
"VolumeId": "vol-0e4fc46b267a3ecce",
"State": "completed",
"StartTime": "2024-10-22T12:41:14.813000+00:00",
"Progress": "100%",
"OwnerId": "account-id",
"Description": "",
"VolumeSize": 10,
"Encrypted": false
}
]
}
It will still not be possible to use tags when doing snapshot backups via standard barman server, but we will work on that internally.
@@ -253,6 +254,7 @@ def get_snapshot_interface_from_server_config(server_config): | |||
server_config.aws_profile, | |||
server_config.aws_region, | |||
server_config.aws_await_snapshots_timeout, | |||
server_config.tags, |
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.
This needs to be removed. This is the server config object and there's currently no tags
option available in the server parameters. This function is used in the context of snapshot backups via the barman standard server (when backup_method = snapshot
) and there's currently no way to pass tags in there.
You indeed get an error when attempting a snapshot backup via barman server with your code:
$ barman backup snapshot-server
WARNING: No backup strategy set for server 'snapshot-server' (using default 'concurrent_backup').
ERROR: Error initialising snapshot provider aws: 'ServerConfig' object has no attribute 'tags'
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.
server_config.tags, |
@@ -463,7 +463,7 @@ class AwsCloudSnapshotInterface(CloudSnapshotInterface): | |||
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-creating-snapshot.html | |||
""" | |||
|
|||
def __init__(self, profile_name=None, region=None, await_snapshots_timeout=3600): | |||
def __init__(self, profile_name=None, region=None, await_snapshots_timeout=3600, tags=None): |
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.
Let's also add a description for tags
in the docstring.
This PR resolves an issue where tags specified via command-line arguments were not being correctly applied to snapshots.
Consider the following example:
Previously, the tags
environment
,project
andservice
were not included in the snapshot tags; only a single tag (Name
) was set by default. This omission made it difficult to filter snapshots based on tags, which is crucial for maintaining security and managing resources effectively.