-
Notifications
You must be signed in to change notification settings - Fork 183
cmd-coreos-prune: Ensure action completion of each build #3950
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
Conversation
src/cmd-coreos-prune
Outdated
| if "images" not in policy_cleanup: | ||
| policy_cleanup["images"] = True | ||
| for action, completed_arches in action_completion_status.items(): | ||
| if set(completed_arches) == set(build["arches"]): # Ensure all arches are completed |
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'm trying to think of a case we would get to this code where not all architectures were completed.. If that were the case wouldn't the code have thrown an exception?
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.
That's correct; technically, we could bypass considering the angle of architectures in these changes. However, assuming that pruning is complete for a build_id after processing just one architecture felt inconsistent.
I could revise the implementation to operate that way, and it would technically work.
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.
Something like this when we run pruning on the first arch should suffice.
action_completion_status[action] = True
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.
SGTM
ffb1b48 to
9738adf
Compare
src/cmd-coreos-prune
Outdated
| for build in reversed(builds): | ||
| build_id = build["id"] | ||
| build_date = parse_fcos_version_to_timestamp(build_id) | ||
| action_completion_status = {action: False for action in policy[stream]} |
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.
optional: this could probably just be a list at this point
| action_completion_status = {action: False for action in policy[stream]} | |
| actions_completed = [] |
Refactor policy-cleanup updates to ensure action completion across all arches. Added action_completion_status to track completed actions per arch. This bug came after we refactored the code to to iterate by build, arch, then action.
9738adf to
be1ba06
Compare
dustymabe
left a comment
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.
LGTM
Refactor policy-cleanup updates to ensure action completion across all arches. Modified to track completed actions
per arch. This bug came after we refactored the code to to iterate by build, arch, then action.
With the current code, it tries to update the builds.json for every build in the stream having the respective action in
gc-policy.yamlnot taking into consideration the datetime for eg. if the script will prune all containers that are 2weeks ago, the script will try to update every build in that stream with"containers": trueno matter if the pruning ran on that build or not.