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

Change handler toleration to "operator: exists" #755

Merged
merged 1 commit into from
May 26, 2021

Conversation

cybertron
Copy link
Collaborator

Is this a BUG FIX or a FEATURE ?:

Uncomment only one, leave it on its own line:

/kind bug

/kind enhancement

What this PR does / why we need it:

Since nmstate is to be used for configuration of the network
infrastructure, we want the handler to run on all nodes regardless
of taint. If a network config should not run on a given node, the
NNCP nodeSelector field can be used to accomplish that.

For example, a node with a taint of:

[map[effect:NoSchedule key:node.ocs.openshift.io/storage value:true]]

will currently keep the nmstate handler pod from running there.
However, that is not desirable since it prevents the use of nmstate
to configure networking on that storage node.

This changes the handler toleration to "operator: exists", which
will allow the handler to run on all nodes. The webhook toleration
is left alone since there is no need for that to be running on
nodes with a NoSchedule taint.

Special notes for your reviewer:

Release note:

Handler pods now have the "operator: exists" toleration so they will run on all nodes. This allows the use of nmstate when configuring networking on NoSchedule tainted nodes.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. kind/bug labels May 20, 2021
@kubevirt-bot kubevirt-bot requested review from phoracek and qinqon May 20, 2021 15:15
@kubevirt-bot
Copy link
Collaborator

Hi @cybertron. Thanks for your PR.

I'm waiting for a nmstate member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kubevirt-bot kubevirt-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS labels May 20, 2021
Since nmstate is to be used for configuration of the network
infrastructure, we want the handler to run on all nodes regardless
of taint. If a network config should not run on a given node, the
NNCP nodeSelector field can be used to accomplish that.

For example, a node with a taint of:

[map[effect:NoSchedule key:node.ocs.openshift.io/storage value:true]]

will currently keep the nmstate handler pod from running there.
However, that is not desirable since it prevents the use of nmstate
to configure networking on that storage node.

This changes the handler toleration to "operator: exists", which
will allow the handler to run on all nodes. The webhook toleration
is left alone since there is no need for that to be running on
nodes with a NoSchedule taint.

Signed-off-by: Ben Nemec <[email protected]>
@cybertron cybertron force-pushed the handler-toleration branch from 6f4fd73 to 390ea95 Compare May 20, 2021 15:17
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels May 20, 2021
@qinqon
Copy link
Member

qinqon commented May 25, 2021

/ok-to-test

@kubevirt-bot kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2021
@cybertron
Copy link
Collaborator Author

/retest

I'm not able to see the logs for some reason, so I'm not sure why the jobs failed. e2e tests were working for me locally.

@qinqon
Copy link
Member

qinqon commented May 26, 2021

/retest

failing test feels related, this was not flaky.

E2E Test Suite: [rfe_id:3503][crit:medium][vendor:[email protected]][level:component]NodeSelector when policy is set with node selector not matching any nodes and we add the label to the node and remove the label again should remove the not matching enactment

@qinqon
Copy link
Member

qinqon commented May 26, 2021

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2021
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021
@kubevirt-bot kubevirt-bot merged commit b2bccf9 into nmstate:main May 26, 2021
cybertron added a commit to cybertron/kubernetes-nmstate that referenced this pull request Jun 8, 2021
…mstate#755)

Since nmstate is to be used for configuration of the network
infrastructure, we want the handler to run on all nodes regardless
of taint. If a network config should not run on a given node, the
NNCP nodeSelector field can be used to accomplish that.

For example, a node with a taint of:

[map[effect:NoSchedule key:node.ocs.openshift.io/storage value:true]]

will currently keep the nmstate handler pod from running there.
However, that is not desirable since it prevents the use of nmstate
to configure networking on that storage node.

This changes the handler toleration to "operator: exists", which
will allow the handler to run on all nodes. The webhook toleration
is left alone since there is no need for that to be running on
nodes with a NoSchedule taint.

Signed-off-by: Ben Nemec <[email protected]>
(cherry picked from commit b2bccf9)
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/kubernetes-nmstate that referenced this pull request Jun 9, 2021
…mstate#755)

Since nmstate is to be used for configuration of the network
infrastructure, we want the handler to run on all nodes regardless
of taint. If a network config should not run on a given node, the
NNCP nodeSelector field can be used to accomplish that.

For example, a node with a taint of:

[map[effect:NoSchedule key:node.ocs.openshift.io/storage value:true]]

will currently keep the nmstate handler pod from running there.
However, that is not desirable since it prevents the use of nmstate
to configure networking on that storage node.

This changes the handler toleration to "operator: exists", which
will allow the handler to run on all nodes. The webhook toleration
is left alone since there is no need for that to be running on
nodes with a NoSchedule taint.

Signed-off-by: Ben Nemec <[email protected]>
(cherry picked from commit b2bccf9)
creydr pushed a commit to creydr/kubernetes-nmstate that referenced this pull request Jun 7, 2022
…mstate#755)

Since nmstate is to be used for configuration of the network
infrastructure, we want the handler to run on all nodes regardless
of taint. If a network config should not run on a given node, the
NNCP nodeSelector field can be used to accomplish that.

For example, a node with a taint of:

[map[effect:NoSchedule key:node.ocs.openshift.io/storage value:true]]

will currently keep the nmstate handler pod from running there.
However, that is not desirable since it prevents the use of nmstate
to configure networking on that storage node.

This changes the handler toleration to "operator: exists", which
will allow the handler to run on all nodes. The webhook toleration
is left alone since there is no need for that to be running on
nodes with a NoSchedule taint.

Signed-off-by: Ben Nemec <[email protected]>
(cherry picked from commit b2bccf9)
(cherry picked from commit de92fbc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants