-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Ovirt delete snapshots after x days #45551
Ovirt delete snapshots after x days #45551
Conversation
The test
The test
|
keep_days_old: | ||
description: | ||
- "Number of days after which should snapshot be deleted." | ||
- "It will check all snapshots of vm and delete them, if they are older." |
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.
s/vm/virtual machine
except Exception as e: | ||
module.fail_json(msg=str(e), exception=traceback.format_exc()) | ||
finally: | ||
if module.params.get('keep_days_old') is not 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.
In this case we wouldn't close the connection. Please create new method something like remove_old_snapshosts
and call it from state=='present' branch as it's default state. Also please document what will be returned in case this paramter is used.
@@ -112,6 +122,10 @@ | |||
at following url: http://ovirt.github.io/ovirt-engine-api-model/master/#types/snapshot." | |||
returned: On success if snapshot is found. | |||
type: dict | |||
snapshots: | |||
description: List of deleted snapshots |
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.
Please also document that it's returned only in case user specify keep_days_old
parameter.
@@ -222,6 +236,23 @@ def restore_snapshot(module, vm_service, snapshots_service): | |||
} | |||
|
|||
|
|||
def remove_old_snapshosts(module, snapshots_service): | |||
from datetime import datetime |
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.
Please do this import at the begging of the file with other imports.
changed = False | ||
date_now = datetime.now() | ||
for snapshot in snapshots_service.list(): | ||
if snapshot.vm and snapshot.vm.name == module.params.get('vm_name'): |
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.
I think it should be just fine to check if snapshot.vm is not None
because you are already using snapshots_service
of that vm, so you don't need to check if names are same.
snapshots_service.snapshot_service(snapshot.id).remove() | ||
deleted_snapshot.append(get_dict_of_struct(snapshot)) | ||
changed = True | ||
time.sleep(45) |
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.
We need to wait until snaphsot is removed and vm ready to remove another snapshot, not any random wait time.
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.
shipit
SUMMARY
Fixes #41500
ISSUE TYPE
COMPONENT NAME
ovirt
ANSIBLE VERSION
ADDITIONAL INFORMATION