OCPEDGE-1309: feat: add arbiter node check #1882
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@eggfoobar: This pull request references OCPEDGE-1309 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
eggfoobar
force-pushed
the
add-arbiter-node-check
branch
from
f3b06ce to
778398c
Compare
eggfoobar
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
eggfoobar
force-pushed
the
add-arbiter-node-check
branch
from
778398c to
fb107ae
Compare
|
@eggfoobar: This pull request references OCPEDGE-1309 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
| @@ -67,6 +79,14 @@ func (c *NodeController) sync(ctx context.Context, syncCtx factory.SyncContext) | |||
| return err | |||
| } | |||
|
|
|||
| if c.arbiterNodeSelector != nil { | |||
| arbiterNodes, err := c.nodeLister.List(c.arbiterNodeSelector) | |||
There was a problem hiding this comment.
Could a new label requirement be part of an already existing selector? (line 77).
I think that a selector can accept multiple requirements.
There was a problem hiding this comment.
Ah, that's a good point, unfortunately, this won't work in this situation labels selectors by design do not support an or operation on keys (see). So this must be done as separate call, otherwise if we were going of values we could build an contains query.
There was a problem hiding this comment.
ok, could you add a comment explaining that the OR operation is not supported on keys and include a link to the issue you mentioned in the other comment?
eggfoobar
force-pushed
the
add-arbiter-node-check
branch
from
fb107ae to
9f0f3b9
Compare
| @@ -124,6 +131,11 @@ type Builder interface { | |||
| ToControllers() (manager.ControllerManager, error) | |||
| } | |||
|
|
|||
| func (b *staticPodOperatorControllerBuilder) WithExtraNodeSelector(extraNodeSelector labels.Selector) Builder { | |||
There was a problem hiding this comment.
What about other controllers potentially listing the master nodes? As I understand, we will only run the etcd member on the arbiter node. Is that correct?
There was a problem hiding this comment.
Right, exactly, so unless explicitly required the other controllers should run only on masters, but if they come to the conclusion they need to also run on the arbiter, we can have a path for them to do it
|
While reviewing this PR I found that the master nodes selector is being parsed on each sync for some controllers and decided to open #1910, PTAL. |
|
also, what is our e2e strategy for this feature? Is there a test in the origin that will add the arbiter node? |
add WithExtraNodeSelector flag to static pod builder passed down selector to guard and node controller to query for extra nodes as well as master nodes Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
add-arbiter-node-check
branch
from
9f0f3b9 to
7d2a658
Compare
Yup! We will have tests added into origin for OLA validation, as well as CI lanes that do the install. This feature will be TechPreview in 4.19 and GA in 4.20 if all the CI signaling goes to plan for the baremetal platform |
|
thanks! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@eggfoobar: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Per EP: openshift/enhancements#1674
This PR adds a check for arbiter node role to guard and node controller for static pods, this is exposed via a
WithArbiterbuilder function to the static pod builder. This should allow folks to decide if their component is designed for the arbiter or not.