bug 2005901: staticpod builder: initialize operand label selector separately by ingvagabund · Pull Request #1281 · openshift/library-go

ingvagabund · 2022-01-10T14:52:25Z

Not every controller consumes operand pod label selector when the StartMonitor is enabled.
Also, the operand pod label selector is now required by the guard
controller since not every operator has the static pod name
equal to the app label. E.g.

operand	label	static pod name
kube-scheduler	app=openshift-kube-scheduler	openshift-kube-scheduler
kube-controller-manager	app=kube-controller-manager	kube-controller-manager
kube-apiserver	app=openshift-kube-apiserver	kube-apiserver

In case of KS and KCM both app label and the static pod name are equal.
Whereas in the case of KA, the app label differs from the static pod
name. Thus, it is more practical to provide the operand pod label selector rather than rely on a specific label.

Required by openshift/cluster-kube-apiserver-operator#1275.

openshift-ci · 2022-01-10T14:52:31Z

@ingvagabund: This pull request references Bugzilla bug 2005901, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @kasturinarra

Details

In response to this:

bug 2005901: staticpod builder: initialize operand label selector separately

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ingvagabund · 2022-01-10T14:53:08Z

/hold
waiting until tests in openshift/cluster-kube-apiserver-operator#1275 go green

openshift-ci · 2022-01-10T14:54:49Z

@ingvagabund: An error was encountered querying GitHub for users with public email (knarra@redhat.com) for bug 2005901 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

bug 2005901: staticpod builder: initialize operand label selector separately

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

p0lyn0mial · 2022-01-10T15:26:33Z

pkg/operator/staticpod/controllers.go

 	WithInstaller(command []string) Builder
 	WithMinReadyDuration(minReadyDuration time.Duration) Builder
-	WithStartupMonitor(enabledStartupMonitor func() (bool, error), operandPodLabelSelector labels.Selector) Builder
+	WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder


so it will become:

staticPodControllers, err := staticpod.NewBuilder(operatorClient, kubeClient, kubeInformersForNamespaces). ... WithStartupMonitor(startupmonitorreadiness.IsStartupMonitorEnabledFunction(configInformers.Config().V1().Infrastructures().Lister(), operatorClient)). WithOperandPodLabelSelector(labels.Set{"apiserver": "true"}.AsSelector())

p0lyn0mial · 2022-01-10T15:27:21Z

pkg/operator/staticpod/controllers.go

-		b.eventRecorder,
-	), 1)
+	if b.operandPodLabelSelector.Empty() {
+		eventRecorder.Warning("OperandPodLabelMissing", "not enough information provided, not all functionality is present")


I think we should return an error since we need a way to find the operand.

actually, we could do without the selector. At the moment it is used only in the staticPodFallbackConditionController. In the worst-case scenario, we would loop over all pods in a given namespace.

As I understand the staticPodFallbackConditionController is narrowly focused on the static pods/operands. So it needs to ignore any other pod. It could incorrectly set the degraded condition otherwise.

not sure why do we want to fire an event in that case? It will make troubleshooting harder. We should either return an error or let it fall through (previous behavior).

I do not consider the controller to be crucial. I'd rather set no condition than set it incorrectly. Though, I might be wrong here.

ingvagabund · 2022-01-11T11:36:54Z

/hold cancel

ingvagabund · 2022-01-11T11:53:19Z

@p0lyn0mial I like the idea of removing the code. Unfortunately, the guard controller needs to target only the operands and ignore everything else. I will not oppose removing the pod label selector from the staticPodFallbackConditionController as I am not fully familiar with the expected functionality and whether the restriction can be relaxed or replaced by what is already wired to the controller. Nevertheless, the guard controller is mirroring the operand pods and it needs to know exactly the difference as each operand pod can have any combination of labels.

I might rely on the operand prefix and match the pods based on the prefix. However, using a pod label selector is more intuitive.

Not every controller consumes operand pod label selector when the StartMonitor is enabled. Also, the operand pod label selector is now required by the guard controller since not every operator has the static pod name equal to the app label. E.g. | operand | label | static pod name | | ------- | ----- | --------------- | | kube-scheduler | app=openshift-kube-scheduler | openshift-kube-scheduler | | kube-controller-manager | app=kube-controller-manager | kube-controller-manager | | kube-apiserver | app=openshift-kube-apiserver | kube-apiserver | In case of KS and KCM both app label and the static pod name are equal. Whereas in the case of KA, the app label differs from the static pod name. Thus, it is more practical to provide the operand pod label selector rather than rely on a specific label.

p0lyn0mial · 2022-01-11T12:51:49Z

/lgtm
/approve

openshift-ci · 2022-01-11T12:52:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, p0lyn0mial

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/operator/staticpod/OWNERS~~ [p0lyn0mial]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-01-11T12:57:10Z

@ingvagabund: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-01-11T12:59:12Z

@ingvagabund: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/cluster-kube-apiserver-operator#1275 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 2005901 has not been moved to the MODIFIED state.

Details

In response to this:

bug 2005901: staticpod builder: initialize operand label selector separately

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 10, 2022

openshift-ci bot requested a review from kasturinarra January 10, 2022 14:52

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2022

openshift-ci bot requested review from p0lyn0mial and sttts January 10, 2022 14:55

p0lyn0mial reviewed Jan 10, 2022

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2022

ingvagabund force-pushed the staticpod-with-operand-label-selector branch from c911d23 to 4c466c2 Compare January 11, 2022 12:49

openshift-ci bot assigned p0lyn0mial Jan 11, 2022

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 11, 2022

openshift-merge-robot merged commit 7f25b9c into openshift:master Jan 11, 2022

ingvagabund deleted the staticpod-with-operand-label-selector branch January 11, 2022 12:59

This was referenced Jan 11, 2022

bug 2005901: Bump library-go openshift/cluster-kube-scheduler-operator#396

Merged

bug 2005901: Bump library-go openshift/cluster-kube-controller-manager-operator#588

Merged

openshift-ci bot mentioned this pull request Jan 12, 2022

bug 2005901: Guard controller pdb openshift/cluster-kube-apiserver-operator#1275

Merged

openshift-ci bot mentioned this pull request Jan 24, 2022

bug 2005901: Sync library go openshift/cluster-kube-controller-manager-operator#591

Merged

Comments

Conversation

ingvagabund commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jan 10, 2022

Uh oh!

ingvagabund commented Jan 10, 2022

Uh oh!

openshift-ci bot commented Jan 10, 2022

Uh oh!

p0lyn0mial Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund commented Jan 11, 2022

Uh oh!

ingvagabund commented Jan 11, 2022

Uh oh!

p0lyn0mial commented Jan 11, 2022

Uh oh!

openshift-ci bot commented Jan 11, 2022

Uh oh!

openshift-ci bot commented Jan 11, 2022

Uh oh!

openshift-ci bot commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ingvagabund commented Jan 10, 2022 •

edited

Loading