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

dont update localbuild resource status when shutting down #140

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

nimakaviani
Copy link
Contributor

updating the resource when shutting down can create a race against a closed context which would throw the following undesirable error.

The PR checks whether we are shutting down before trying to update the localbuild resource. The assumption is that the necessary update to the status should have already happened.

1.706661106363861e+09   ERROR   controller.localbuild   Failed to update resource status after reconcile {"reconciler group": "idpbuilder.cnoe.io", "reconciler kind": "Localbuild", "name": "localdev", "namespace":
 "", "error": "client rate limiter Wait returned an error: context canceled"}
github.com/cnoe-io/idpbuilder/pkg/controllers/localbuild.(*LocalbuildReconciler).Reconcile
        /Users/nkaviani/workspaces/git/cnoe/idpbuilder/pkg/controllers/localbuild/controller.go:76
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
        /Users/nkaviani/.gvm/pkgsets/go1.20/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:114
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
        /Users/nkaviani/.gvm/pkgsets/go1.20/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:311
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
        /Users/nkaviani/.gvm/pkgsets/go1.20/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
        /Users/nkaviani/.gvm/pkgsets/go1.20/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227

updating the resource when shutting down can create a race
against a closed context which would throw an undesirable error

Signed-off-by: Nima Kaviani <[email protected]>
@nimakaviani nimakaviani requested a review from nabuskey January 31, 2024 01:27
@nimakaviani
Copy link
Contributor Author

I also think showing an error message for the missing cnoe.io/last-observed-cli-start-time annotation is confusing and misleading. Why I added another commit to prevent the reconciler from shutting down but also logging what we would otherwise see as an error message, as a logger.Info message

1.706675874321422e+09   ERROR   controller.localbuild   Reconciler error        {"reconciler group": "idpbuilder.cnoe.io", "reconciler kind":
"Localbuild", "name": "localdev", "namespace": "", "error": "expected annotation, cnoe.io/last-observed-cli-start-time, not found"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem                                                              /Users/nkaviani/.gvm/pkgsets/go1.20/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2```

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

LGTM

@nabuskey nabuskey merged commit 4806dd3 into main Feb 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants