Skip to content
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

Transform KubernetesComputeResourcesPatch "Push" statuses into "Pull" statuses #107

Merged
merged 16 commits into from
Aug 29, 2024

Conversation

michaeldmitry
Copy link
Contributor

@michaeldmitry michaeldmitry commented Aug 20, 2024

Issue

The way how v0 KubernetesComputeResourcesPatch currently behaves enforces charms using the library to implement a push status pattern to keep track of failures that might have happened during the patching. Even the existing function is_ready doesn't have enough info to determine whether the patch is not ready because it has failed or because its still in progress. That introduces an overhead of managing that "push" status and probably maintaining a StoredState in the charm.

Solution

Introduce a new API function get_status that gets the current status of the patch operation.
get_status will return one of 3 possible states:
1- succeeded: a patch operation has completed successfully or the patch operation itself has not started yet.
2- failed: using dry-run mechanism, detect if the patch operation would've failed.
3- in_progress: rollout status-like logic to detect if the patch is still being applied and not fully ready.

Testing Instructions

A tandem PR for loki for using the new version of the library and test existing functionality as normal.

Upgrade Notes

Charms can continue using this new version of the library same way as before relying on is_ready as a push status. If charms wish to transform that as a pull status, they should start using get_status in collect-unit-status and depending on the state of the patch returned, set the desired state of the charm.

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

A few thoughts and doubts about the public API.
Didn't do an in-depth review and didn't try it out yet. Could you open a tandem PR in some charm where you replace the old lib with the new one, so we can clone that, deploy it, and see it in action?

Wondering if it makes sense to only move the files before merging but not while in the review stage? Having this large diff makes it really hard to look at the code.

@michaeldmitry
Copy link
Contributor Author

@PietroPasotti reverted the new files to ease the review and this PR is for testing the above changes in an existing codebase.

Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

Partial review for now, sorry :)

Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

Great job!
The is_in_progress is difficult to grok/utest, and could probably be better to have in lightkube, but that's not a blocker for me.

@michaeldmitry michaeldmitry merged commit 47a4142 into main Aug 29, 2024
13 checks passed
@michaeldmitry michaeldmitry deleted the OPENG-2205 branch August 29, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants