Skip to content

EFS: Added backup option #1502

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/1502-efs-add-backup-option.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- efs - adds ``backup`` option to enable/disable automatic backups on the file system (https://github.com/ansible-collections/community.aws/issues/1266).
54 changes: 54 additions & 0 deletions plugins/modules/efs.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@
description:
- If the throughput_mode is provisioned, select the amount of throughput to provisioned in Mibps.
type: float
backup:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you call it backup_policy to be consistent with AWS's naming.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, I will change this

description:
- Changes the file system backup policy. Use this option to start or stop automatic backups of the file system.
- By default, automatic backup of the file system is disabled.
choices: ['enabled', 'disabled']
type: str
wait:
description:
- "In case of 'present' state should wait for EFS 'available' life cycle state (of course, if current state not 'deleting' or 'deleted')
Expand Down Expand Up @@ -274,6 +280,7 @@ class EFSConnection(object):
STATE_AVAILABLE = 'available'
STATE_DELETING = 'deleting'
STATE_DELETED = 'deleted'
BACKUP_DISABLED = 'DISABLED'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the constant increase the readability.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would BACKUP_POLICY_DISABLED or BACKUP_POLICY_STATUS_DISABLED improve on readability?


def __init__(self, module):
self.connection = module.client('efs')
Expand Down Expand Up @@ -413,6 +420,24 @@ def get_provisioned_throughput_in_mibps(self, **kwargs):
))
return info.get('ProvisionedThroughputInMibps', None)

def get_backup_policy(self, fs_id):
"""
Returns the status of Filesystem backup policy
"""
try:
info = self.connection.describe_backup_policy(FileSystemId=fs_id)
return info['BackupPolicy']['Status']
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
# PolicyNotFound error returned whenever default backup policy is in effect
# In this case, we return disabled status
# By default, the automatic backup is disabled
if e.response['Error']['Code'] == 'PolicyNotFound':
return self.BACKUP_DISABLED
else:
self.module.fail_json_aws(e, msg="Unable to get file system backup policy.")

return self.BACKUP_DISABLED

def create_file_system(self, name, performance_mode, encrypt, kms_key_id, throughput_mode, provisioned_throughput_in_mibps):
"""
Creates new filesystem with selected name
Expand Down Expand Up @@ -502,6 +527,30 @@ def update_lifecycle_policy(self, name, transition_to_ia):
changed = True
return changed

def change_backup_policy(self, name, status):
"""
Enable/disable automatic backup policy of the FileSystem.
"""
changed = False
state = self.get_file_system_state(name)
if state in [self.STATE_AVAILABLE, self.STATE_CREATING]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If state doesn't match the condition, we silently ignore it and return "unchanged" even if the value is not the one expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you mean by this, isn't the "unchanged" when the changed= False?

wait_for(
lambda: self.get_file_system_state(name),
self.STATE_AVAILABLE,
self.wait_timeout
)
fs_id = self.get_file_system_id(name)
current_backup_policy = self.get_backup_policy(fs_id)
params = dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is valide, but params in general is already used for the module parameters. Can you use a different name for your variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

if status != current_backup_policy:
params['Status'] = status
try:
self.connection.put_backup_policy(FileSystemId=fs_id, BackupPolicy=params)
changed = True
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
self.module.fail_json_aws(e, msg="Unable to update backup policy status.")
return changed

def converge_file_system(self, name, tags, purge_tags, targets, throughput_mode, provisioned_throughput_in_mibps):
"""
Change attributes (mount targets and tags) of filesystem by name
Expand Down Expand Up @@ -726,6 +775,7 @@ def main():
transition_to_ia=dict(required=False, type='str', choices=["None", "7", "14", "30", "60", "90"], default=None),
throughput_mode=dict(required=False, type='str', choices=["bursting", "provisioned"], default=None),
provisioned_throughput_in_mibps=dict(required=False, type='float'),
backup=dict(required=False, type='str', choices=["enabled", "disabled"], default=None),
wait=dict(required=False, type="bool", default=False),
wait_timeout=dict(required=False, type="int", default=0)
)
Expand Down Expand Up @@ -754,6 +804,7 @@ def main():
transition_to_ia = module.params.get('transition_to_ia')
throughput_mode = module.params.get('throughput_mode')
provisioned_throughput_in_mibps = module.params.get('provisioned_throughput_in_mibps')
backup = module.params.get('backup')
state = str(module.params.get('state')).lower()
changed = False

Expand All @@ -767,6 +818,9 @@ def main():
throughput_mode=throughput_mode, provisioned_throughput_in_mibps=provisioned_throughput_in_mibps) or changed
if transition_to_ia:
changed |= connection.update_lifecycle_policy(name, transition_to_ia)
if backup:
backup = str(backup).upper()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backup is already a string, you don't need the str() cast.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this catch

changed = connection.change_backup_policy(name, backup)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should changed |=, not just =, this way we preserve the value if it's set True above.

e.g:

>>> changed = False
>>> changed |= False
>>> changed |= True
>>> changed |= False
>>> changed
True

result = first_or_default(connection.get_file_systems(CreationToken=name))

elif state == 'absent':
Expand Down
68 changes: 68 additions & 0 deletions tests/integration/targets/efs/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,74 @@
- efs_result is not changed

# ============================================================
- name: Efs enable automatic backup
efs:
state: present
name: "{{ efs_name }}-test-efs"
tags:
Name: "{{ efs_name }}-test-tag"
Purpose: file-storage
targets:
- subnet_id: "{{testing_subnet_a.subnet.id}}"
- subnet_id: "{{testing_subnet_b.subnet.id}}"
backup: 'enabled'
register: efs_result

- assert:
that:
- efs_result is changed

- name: Efs enable automatic backup - idempotency
efs:
state: present
name: "{{ efs_name }}-test-efs"
tags:
Name: "{{ efs_name }}-test-tag"
Purpose: file-storage
targets:
- subnet_id: "{{testing_subnet_a.subnet.id}}"
- subnet_id: "{{testing_subnet_b.subnet.id}}"
backup: 'enabled'
register: efs_result

- assert:
that:
- efs_result is not changed

- name: Efs disable automatic backup
efs:
state: present
name: "{{ efs_name }}-test-efs"
tags:
Name: "{{ efs_name }}-test-tag"
Purpose: file-storage
targets:
- subnet_id: "{{testing_subnet_a.subnet.id}}"
- subnet_id: "{{testing_subnet_b.subnet.id}}"
backup: 'disabled'
register: efs_result

- assert:
that:
- efs_result is changed

- name: Efs disable automatic backup - idempotency
efs:
state: present
name: "{{ efs_name }}-test-efs"
tags:
Name: "{{ efs_name }}-test-tag"
Purpose: file-storage
targets:
- subnet_id: "{{testing_subnet_a.subnet.id}}"
- subnet_id: "{{testing_subnet_b.subnet.id}}"
backup: 'disabled'
register: efs_result

- assert:
that:
- efs_result is not changed
# ============================================================
- name: Query unknown EFS by tag
efs_info:
tags:
Expand Down