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

Improve HelmRepository type switching from default to oci #1016

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Feb 1, 2023

Depends on fluxcd/pkg#458

When a HelmRepository with "default" spec.type is switched to "oci", the
existing HelmRepository is processed by HelmRepositoryReconciler by
running reconcileDelete() which removes all the previous status
information and allows the HelmRepositoryOCIReconciler to process the
object and add its own status data. But at times, when
HelmRepositoryOCIReconciler starts processing a HelmRepository with
stale status data from the client cache, it contains the stale
conditions that are owned only by HelmRepositoryReconciler and isn't
managed by HelmRepositoryOCIReconciler. This results in situations where
Ready is marked as True with the latest generation of the object and the
unmanaged stale conditions remain in the previous generation, resulting
in unexpected status conditions.

In the observed flaky tests,
TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter would
fail because of the stale ArtifactInStorage condition with previous
generation value.

--- FAIL: TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter (0.94s)
    helmrepository_controller_test.go:1333: 
        [Check-FAIL]: [Ready condition must be False when any of the status condition's ObservedGeneration is less than the object Generation: [ArtifactInStorage], The status conditions' ObservedGenerations must be equal to the root ObservedGeneration when Ready condition is True: [ArtifactInStorage]]

This change adds a check in the HelmRepositoryOCIReconciler to start
processing the object only once the stale unmanaged conditions have been
removed.

@darkowlzz darkowlzz added the area/testing Testing related issues and pull requests label Feb 1, 2023
@darkowlzz darkowlzz force-pushed the condn-checker-with-t branch 4 times, most recently from 8fb5a1e to 481a36a Compare February 2, 2023 21:39
@darkowlzz darkowlzz changed the title [Draft] test: Use condition checker with gomega WithT Improve HelmRepository type switching from default to oci Feb 2, 2023
@darkowlzz darkowlzz added area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests labels Feb 2, 2023
@darkowlzz darkowlzz marked this pull request as ready for review February 2, 2023 22:36
@darkowlzz darkowlzz force-pushed the condn-checker-with-t branch 3 times, most recently from 206f751 to b87ac82 Compare February 6, 2023 10:43
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Feb 6, 2023

I've added log and event for when HelmRepositoryOCIReconciler encounters an object with unmanaged conditions. This will make the scenario more visible for users and administrators if and when this happens.
Log:

{"level":"info","ts":"2023-02-06T16:10:12.432+0530","msg":"object contains conditions managed by other reconciler","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","HelmRepository":{"name":"example","namespace":"test-1"},"namespace":"test-1","name":"example","reconcileID":"40a0dd49-0cf1-44b0-88bb-e912fc4c272d"}

Events:

LAST SEEN   TYPE     REASON                       OBJECT                   MESSAGE
42s         Normal   NewArtifact                  helmrepository/example   stored fetched index of size 40.9kB from 'https://stefanprodan.github.io/podinfo'
11s         Normal   IncompleteTransition         helmrepository/example   object contains conditions managed by other reconciler
11s         Normal   GarbageCollectionSucceeded   helmrepository/example   garbage collected artifacts for deleted resource
10s         Normal   Succeeded                    helmrepository/example   Helm repository is ready

Introduced a new event reason IncompleteTransition. Please recommend any better name if it comes to mind. Also, if the error message should be more specific to be more helpful.

This allows using the condition checker as a test helper with proper
test like assertion failure and stacktrace.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
When a HelmRepository with "default" spec.type is switched to "oci", the
existing HelmRepository is processed by HelmRepositoryReconciler by
running reconcileDelete() which removes all the previous status
information and allows the HelmRepositoryOCIReconciler to process the
object and add its own status data. But at times, when
HelmRepositoryOCIReconciler starts processing a HelmRepository with
stale status data from the client cache, it contains the stale
conditions that are owned only by HelmRepositoryReconciler and isn't
managed by HelmRepositoryOCIReconciler. This results in situations where
Ready is marked as True with the latest generation of the object and the
unmanaged stale conditions remain in the previous generation, resulting
in unexpected status conditions.

In the observed flaky tests,
`TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter` would
fail because of stale ArtifactInStorage condition with previous
generation value.

This change adds a check in the HelmRepositoryOCIReconciler to start
processing the object only once the stale unmanaged conditions have been
removed.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz darkowlzz merged commit d18988e into main Feb 8, 2023
@darkowlzz darkowlzz deleted the condn-checker-with-t branch February 8, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests area/testing Testing related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants