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

reconcile: Set observed gen only when conditions exist #729

Merged
merged 1 commit into from
May 25, 2022

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented May 24, 2022

The observed generation must be set only when actual observation is
made. When an actual observation is made, some conditions are set on the
object. Introduce a helper function
addPatchOptionWithStatusObservedGeneration() to set the patcher option
WithStatusObservedGeneration only when there's any condition in the
status.

Updates the existing tests that depended on this behavior.

This fixes the issue where the observed generation is set by the patcher
when a reconciler does an early return for setting the finalizers only.
With this, the observed generation will be updated only when some
observations are made on the object based on the usual rules of success
result, no error, ignore error and stalled condition.

After finalizer setting reconcile -

Before this change:

status:
  observedGeneration: 1

After this change:

status:
  observedGeneration: -1

Ref: fluxcd/flux2#2736

@darkowlzz darkowlzz added the enhancement New feature or request label May 24, 2022
@darkowlzz
Copy link
Contributor Author

Looks like it's minio again:

*** Test killed with quit: ran too long (11m0s).
FAIL	github.com/fluxcd/source-controller/pkg/minio	660.006s

@stefanprodan
Copy link
Member

IMO this is a regression bug fix not an enhancement

@darkowlzz
Copy link
Contributor Author

I haven't verified that it fixes the CLI issue. Couldn't reproduce it locally by creating a GitRepo and Kustomization rapidly based on the example in https://github.com/fluxcd/flux2-multi-tenancy/blob/50ad87d14542a09249bfe761b441078f12b4053f/.github/workflows/e2e.yaml#L43 . Need to try it in flux2-multi-tenancy CI.
But the CLI code that fails, maybe https://github.com/fluxcd/flux2/blob/938f2570efb05f26ff83b7684f950389b7d3f60b/cmd/flux/create_source_helm.go#L245-L250 , doesn't use the status.observedGeneration, but Ready condition's observedGeneration.
I believe that's very different from what this change does and may not be directly related.

@hiddeco hiddeco added bug Something isn't working and removed enhancement New feature or request labels May 25, 2022
@hiddeco
Copy link
Member

hiddeco commented May 25, 2022

This is indeed a bug and not an enhancement as it was not working as intended, even if this would not pose an issue to the CLI.

The observed generation must be set only when actual observation is
made. When an actual observation is made, some conditions are set on the
object. Introduce a helper function
addPatchOptionWithStatusObservedGeneration() to set the patcher option
WithStatusObservedGeneration only when there's any condition in the
status.

Updates the existing tests that depended on this behavior.

This fixes the issue where the observed generation is set by the patcher
when a reconciler does an early return for setting the finalizers only.
With this, the observed generation will be updated only when some
observations are made on the object based on the usual rules of success
result, no error, ignore error and stalled condition.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@makkes makkes merged commit 6e768b3 into main May 25, 2022
@makkes makkes deleted the observed-gen-no-condition branch May 25, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants