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

Introduce Progressive status #974

Merged
merged 9 commits into from
Jan 10, 2023
Merged

Introduce Progressive status #974

merged 9 commits into from
Jan 10, 2023

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Dec 5, 2022

Depends on fluxcd/pkg#422

This change introduces progressive status across all the reconcilers. It uses the helpers and checks proposed in fluxcd/pkg#422 to implement and verify the result of the change.

Mid-reconciliation status

Before this change, the reconcilers patched the target object of reconciliation only once, at the end of the reconciliation to report the final result of the reconciliation. The change in the status that took place during reconciliation were not reflected on the object. As described in #936, this resulted in stale status reported on the object. This also resulted in not reporting the progressive status of the reconciliation, which made it hard to understand if an object is undergoing reconciliation or not, refer #935 .

In order to perform multiple status updates on the object during reconciliation, a serial patcher was introduced in fluxcd/pkg#379 . This patcher is a drop-in replacement for the previous patch helper. It keeps track of the previously patched object and constructs subsequent patch based on the previously patched object.

To perform mid-reconciliation patching, all the sub-reconcilers need an instance of the patcher. This change extends the sub-reconciler function signature to allow passing the patcher.

When reconciliation of an object begins, the object is immediately marked with Reconciling=True. If it's a new object generation or reconciliation is manually requested using the reconcile annotation value, the reconciling status is patched and reported on the object.

In the reconcileStorage() phase, if there's no artifact in the storage or an existing artifact disappeared from the storage, it is considered as a drift and is immediately reported on the object by patching the status.

In the reconcileSource() phase, if a drift is detected in the artifact or the included artifact, it is again considered as a drift and is immediately reported on the object by patching the status.

All the sub-reconciler tests have been updated accordingly with in-progress status condition check.

Reconciled status

In order to indicate that a reconciliation has resulted in a failure and reconciliation will be retried, the Reconciling=True condition is updated with ProgressingWithRetry reason. Kustomize-controller already does this.

After a full reconciliation, if the reconciliation didn't succeed and Reconciling is still True, the ComputeReconcileResult() helper is updated to add the Reconciling=True reason with ProgressingWithRetry.
Subsequent reconciliation updates the Reconciling=True reason with Progressing to indicate that a reconciliation is in progress.

SummarizeAndPatch() has been updated to replace the previous patch helper with the serial patcher.

Fixes #936, #935


Snippets showing the result of this change with status output:

Except for HelmRepository OCI reconciler, all the other reconcilers have similar implementations. Their status results should be similar.

@darkowlzz darkowlzz added the enhancement New feature or request label Dec 5, 2022
Update pkg/runtime for progressive status tooling.

Signed-off-by: Sunny <[email protected]>
Replace the patch Helper with SerialPatcher which is used for
progressive status patching.

Update the tests to use progressive status reasons in tests.

Add ProgressingWithRetry Reconciling reason for failed
reconciliation result to indicate a finished failure operation.

Signed-off-by: Sunny <[email protected]>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

This all looks sensible to me, thank you @darkowlzz 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Artifact is wrongly reported in storage after a restart
3 participants