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.

Sign up for GitHub

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

Jump to bottom

Fix NodeConfigPod controller to exclusively enqueue and sync Scylla Pods #1131

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Fix NodeConfigPod controller to exclusively enqueue and sync Scylla Pods #1131

merged 2 commits into from
Feb 13, 2023

Conversation

rzetelskik
Copy link
Member

Description of your changes:
As described in #1119, currently the NodeConfigPod controller removes ownerReferences from any ConfigMap that has a controllerRef to any Pod. This is undesired as the controller shouldn't affect any resources that the operator doesn't manage. This PR adds a condition verifying whether the owner of the ConfigMap is a Scylla Pod.

Which issue is resolved by this Pull Request:
Resolves #1119

@rzetelskik rzetelskik added the kind/bug Categorizes issue or PR as related to a bug. label Dec 27, 2022
@rzetelskik rzetelskik added this to the v1.8 milestone Dec 27, 2022
@rzetelskik rzetelskik modified the milestones: v1.8, v1.9 Jan 4, 2023
@rzetelskik rzetelskik closed this Jan 9, 2023
@rzetelskik rzetelskik reopened this Jan 9, 2023
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 2 times, most recently from 54f405b to e0dbb02 Compare January 9, 2023 16:53
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 4 times, most recently from 89fedb9 to fea7b6c Compare January 11, 2023 14:31
@rzetelskik rzetelskik requested a review from zimnx January 11, 2023 14:32
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 2 times, most recently from ca5987f to 977dc83 Compare January 11, 2023 15:42
@rzetelskik rzetelskik mentioned this pull request Jan 11, 2023
Closed
zimnx
zimnx previously approved these changes Jan 11, 2023
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

tnozicka
tnozicka reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

I think we should also add the check to sync too. Handlers are more about optimization, which is good as well, but there should be a single point of enforcement.

pkg/controller/nodeconfigpod/controller.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfigpod/controller.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfigpod/controller.go Outdated Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Outdated Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Outdated Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Outdated Show resolved Hide resolved
@tnozicka tnozicka added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 23, 2023
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 4 times, most recently from 6cf3570 to 456fc7a Compare January 24, 2023 17:33
@rzetelskik rzetelskik changed the title Fix NodeConfigPod controller to only enqueue Scylla Pods Fix NodeConfigPod controller to exclusively enqueue and sync Scylla Pods Jan 24, 2023
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 4 times, most recently from f4a728f to 0143df8 Compare January 25, 2023 09:35
zimnx
zimnx previously approved these changes Feb 6, 2023
View reviewed changes
tnozicka
tnozicka reviewed Feb 8, 2023
View reviewed changes
pkg/controller/nodeconfigpod/sync.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfigpod/sync.go Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Show resolved Hide resolved
pkg/controllerhelpers/handlers.go Outdated Show resolved Hide resolved
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 3 times, most recently from 0779f18 to 9929ca6 Compare February 8, 2023 14:50
tnozicka
tnozicka reviewed Feb 9, 2023
View reviewed changes
pkg/naming/labels.go Outdated Show resolved Hide resolved
@rzetelskik rzetelskik force-pushed the configmaps-patch branch 4 times, most recently from 62efd19 to 6893036 Compare February 9, 2023 13:23
tnozicka
tnozicka previously approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@rzetelskik
Copy link
Member Author

rebased on current master, @zimnx please approve

@zimnx zimnx merged commit c6a6e05 into scylladb:master Feb 13, 2023
@rzetelskik rzetelskik deleted the configmaps-patch branch February 13, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimnx zimnx zimnx approved these changes

@tnozicka tnozicka tnozicka approved these changes

Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scylla is patching unrelated config maps
3 participants
@rzetelskik @zimnx @tnozicka