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

Bug 2247735: Adding toleration to schedule CSI pods on master nodes #40

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

rchikatw
Copy link
Contributor

Adding toleration to schedule CSI pods on master nodes

Copy link

openshift-ci bot commented Nov 15, 2023

@rchikatw: This pull request references Bugzilla bug 2247735, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

2 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @nehaberry

In response to this:

Bug 2247735: Adding toleration to schedule CSI pods on master nodes

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.

Copy link

openshift-ci bot commented Nov 15, 2023

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: nehaberry.

Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@rchikatw: This pull request references Bugzilla bug 2247735, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

2 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @nehaberry

In response to this:

Bug 2247735: Adding toleration to schedule CSI pods on master nodes

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.

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.

@rchikatw rchikatw force-pushed the fusion-hci-4.14 branch 6 times, most recently from 8447256 to f995e64 Compare November 15, 2023 12:11
pkg/utils/k8sutils.go Outdated Show resolved Hide resolved
pkg/utils/placements.go Outdated Show resolved Hide resolved
}
if runOnMaster {
toleration := corev1.Toleration{
Key: "node-role.kubernetes.io/master",
Copy link
Member

Choose a reason for hiding this comment

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

it looks like this is deprecated and replaced with control-plane https://kubernetes.io/docs/reference/labels-annotations-taints/#node-role-kubernetes-io-master-taint, please check what we have in supported OCP versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked one of the fusion hci clusters the key was "node-role.kubernetes.io/master". we can go with this key

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

please test and change and see if its working as expected

pkg/utils/placements.go Outdated Show resolved Hide resolved
@leelavg
Copy link
Contributor

leelavg commented Nov 16, 2023

@Madhu-1 and @rchikatw

  • I'm wondering why not runCSIDaemonSetOnAllNodes and use Toleration{Operator: TolerationExists}

Reasoning being,

  • no need to find and set the Toleration.Key explicitly.
  • Only if workloads are scheduled on tainted nodes then only CSI pods does actual work (this is a con as well, they are always there consuming resources)
  • let the default be false (or not set) to conform to existing deployments and take the input from Subscription .spec.config.env (or what is the planned deployment model, iirc env will be reset if not set in CSV?)

@Madhu-1
Copy link
Member

Madhu-1 commented Nov 16, 2023

@Madhu-1 and @rchikatw

  • I'm wondering why not runCSIDaemonSetOnAllNodes and use Toleration{Operator: TolerationExists}

The requirement was to run daemonset pods also on master node but not the nodes which might have tainted for something else.

Reasoning being,

  • no need to find and set the Toleration.Key explicitly.
  • Only if workloads are scheduled on tainted nodes then only CSI pods does actual work (this is a con as well, they are always there consuming resources)
  • let the default be false (or not set) to conform to existing deployments and take the input from Subscription .spec.config.env (or what is the planned deployment model, iirc env will be reset if not set in CSV?)

👍🏻 default is true but it need to be added to env to avoid upgrade problem

@leelavg
Copy link
Contributor

leelavg commented Nov 20, 2023

when PVC is scheduled on the master node CSI pod fails to run

Signed-off-by: rchikatw <[email protected]>
@nb-ohad
Copy link
Contributor

nb-ohad commented Nov 20, 2023

/approve

Copy link

openshift-ci bot commented Nov 20, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leelavg, nb-ohad, rchikatw

The full list of commands accepted by this bot can be found 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

@nb-ohad nb-ohad merged commit e364edf into red-hat-storage:fusion-hci-4.14 Nov 20, 2023
12 checks passed
Copy link

openshift-ci bot commented Nov 20, 2023

@rchikatw: All pull requests linked via external trackers have merged:

Bugzilla bug 2247735 has been moved to the MODIFIED state.

In response to this:

Bug 2247735: Adding toleration to schedule CSI pods on master nodes

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants