Make master nodes schedulable by ravisantoshgudimetla · Pull Request #366 · openshift/api

ravisantoshgudimetla · 2019-07-03T21:28:14Z

Add field to make % of masters to be schedulable. This is part of RHHI.next effort, where we want master nodes to be schedulable. For more context, please refer to:

https://docs.google.com/document/d/1FPvz3TyKEagXDekHhKSo7sCqHTxWnITG5pBOG4yUJgA/edit

openshift/machine-config-operator#763 (comment)

ravisantoshgudimetla · 2019-07-03T21:29:56Z

/cc @soltysh @deads2k @derekwaynecarr @eparis @cgwalters

cgwalters · 2019-07-08T13:05:10Z

+	// if we have 3 running master nodes in the cluster, setting this value to 33% means, 1
+	// node of 3 available master nodes would become schedulable. The absolute value is calculated
+	// by rounding down. This field is optional
+	PercentageOfMastersSchedulable intstr.IntOrString `json:"percentageOfMastersSchedulable"`


I think you need
+optional
at the end of the doc text (just like the above DefaultNodeSelector).
And also make it *intstr.IntOrString so we can distinguish unset or empty.

The naming seems awkward; how about just MaxMastersSchedulable or so, a bit like RollingUpdateDeployment's MaxUnavailable?

I think you need
+optional
at the end of the doc text (just like the above DefaultNodeSelector).

I think it's not needed as long as we provide some text. I have seen both the variants but not sure which one is preferred

The naming seems awkward; how about just MaxMastersSchedulable or so, a bit like RollingUpdateDeployment's MaxUnavailable?

Yeah, I agree. I got to know about it while I was implementing, where it's possible to have both integer values and strings. Will change that in a bit along with comments.

intOrString for explicitly specified percentages is strange. Why not just making it a float?

And add minimum and maximum as OpenAPI specification.

We can add number of nodes as well, for example a user can explicitly tell 3 instead of 40%. This is the pattern we've been following in other apps.

deads2k · 2019-07-08T15:09:04Z

+	// node of 3 available master nodes would become schedulable. The absolute value is calculated
+	// by rounding down. This field is optional
+	// +optional
+	MaxMastersSchedulable *intstr.IntOrString `json:"percentageOfMastersSchedulable"`


I think this should be a bool. I don't see a reason to make this a percentage.

The reason is in future, say if we have 5 masters cluster. We can tell, I want only 2 to be schedulable, so that even if they're loaded and they go down, we can still maintain quorum(with 3 masters). The other use-case is developers telling, I want 2 nodes to be made worker nodes and let one run as master..

@soltysh had a concern about making this a bool, specifically having to change this interface later if we ever wanted to to allow the user to specify a subset of masters that should be scheduleable.

@deads2k thoughts on that?

@soltysh had a concern about making this a bool, specifically having to change this interface later if we ever wanted to to allow the user to specify a subset of masters that should be scheduleable.

@deads2k thoughts on that?

If that becomes a thing we want (and I think that's pretty unlikely), we can add another field that takes precedence over this one.

sttts · 2019-07-08T15:18:46Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// percentageOfMastersSchedulable allows a certain percentage of available master nodes to
+	// be schedulable. As of now, the supported values are 0 and 100. In future, we 


nit: in the future

sttts · 2019-07-08T15:21:32Z

+	// will allow a certain percentage of master nodes to be schedulable. For example,
+	// if we have 3 running master nodes in the cluster, setting this value to 33% means, 1
+	// node of 3 available master nodes would become schedulable. The absolute value is calculated
+	// by rounding down. This field is optional


rounding down means that 33% becomes zero master.

sttts · 2019-07-08T19:18:31Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// MastersSchedulable allows masters nodes to be schedulable. This means, if there
+	// are n no of master nodes in the cluster, all of them would be made schedulable.


why mention n? "This means that all of them would be made schedulable."

Changed it to 10 as suggested by @deads2k

deads2k · 2019-07-08T19:18:31Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// MastersSchedulable allows masters nodes to be schedulable. This means, if there
+	// are n no of master nodes in the cluster, all of them would be made schedulable.


use an actual number.

deads2k · 2019-07-08T19:18:42Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// MastersSchedulable allows masters nodes to be schedulable. This means, if there
+	// are n no of master nodes in the cluster, all of them would be made schedulable.


remove early period.

Done. Thanks :)

deads2k · 2019-07-08T19:19:01Z

+	// Important Note: Once the workload pods start running on the master nodes, there
+	// is a very good chance that performance of control plane components is impacted.
+	// Please turn on this field after doing due deligence.
+	// +optional


You need to indicate what the default is.

deads2k · 2019-07-08T19:21:15Z

+	// so that workload pods can run on them. 
+	// Important Note: Once the workload pods start running on the master nodes, there
+	// is a very good chance that performance of control plane components is impacted.
+	// Please turn on this field after doing due deligence.


run a spell check on your doc. diligence isn't it?

Yes it is. I changed it. Thanks for catching it.

and what do you use for spell checking? I haven't tried anything yet of that sort and something for fedora?

ravisantoshgudimetla · 2019-07-08T20:35:21Z

@deads2k and @sttts - Updated. PTAL

sttts · 2019-07-09T06:43:58Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// MastersSchedulable allows masters nodes to be schedulable. This means, if there
+	// are 10 master nodes in the cluster, all of them would be made schedulable, 


same old question: what does talking about the number of masters add to the contents here? It does not matter whether we 2, 10 or 100 masters. All is all :-)

sttts · 2019-07-09T06:44:18Z

+	// MastersSchedulable allows masters nodes to be schedulable. This means, if there
+	// are 10 master nodes in the cluster, all of them would be made schedulable, 
+	// so that workload pods can run on them. The default value for this field is false,
+	// meaning none of the master nodes are schedulable


sttts · 2019-07-09T06:44:39Z

+	// meaning none of the master nodes are schedulable
+	// Important Note: Once the workload pods start running on the master nodes, 
+	// extreme care must be taken to ensure that cluster-critical control plane components 
+	// are not impacted


sttts · 2019-07-09T13:19:00Z

 	// +optional
 	DefaultNodeSelector string `json:"defaultNodeSelector,omitempty"`
+	// MastersSchedulable allows masters nodes to be schedulable. When this flag is
+        // turned on, all the master nodes in the cluster would be made schedulable,


"would be made" or "will be made" ?

sttts · 2019-07-09T13:26:04Z

+        // are not impacted.
+        // Please turn on this field after doing due diligence.
+        // +optional
+        MastersSchedulable bool `json:"MastersSchedulable"`


sttts · 2019-07-09T14:25:03Z

/lgtm
/approve

openshift-ci-robot · 2019-07-09T14:25:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ravisantoshgudimetla, sttts

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:

~~OWNERS~~ [sttts]

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

By default, the MCO's kubelet configuration injects a taint to disable scheduling. This adds support in the MCO for watching the API recently added to allow the masters to be schedulable. openshift/api#366 Use cases here are for 3 node bare metal clusters (which have significant resources on the masters), as well as CodeReady Containers which wants to make a single node OpenShift as a VM. Closes: openshift#763

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 3, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan July 3, 2019 21:28

ravisantoshgudimetla changed the title ~~Add master schedulable~~ Make master nodes schedulable Jul 3, 2019

openshift-ci-robot requested review from cgwalters, deads2k, derekwaynecarr, eparis and soltysh July 3, 2019 21:29

cgwalters reviewed Jul 8, 2019

View reviewed changes

ravisantoshgudimetla force-pushed the add-masterSchedulable branch from 0612176 to 4a117e2 Compare July 8, 2019 15:03

deads2k reviewed Jul 8, 2019

View reviewed changes

sttts reviewed Jul 8, 2019

View reviewed changes

ravisantoshgudimetla force-pushed the add-masterSchedulable branch 2 times, most recently from 7d3f3d7 to c7db6e4 Compare July 8, 2019 19:14

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2019

sttts reviewed Jul 8, 2019

View reviewed changes

deads2k reviewed Jul 8, 2019

View reviewed changes

ravisantoshgudimetla force-pushed the add-masterSchedulable branch from c7db6e4 to fc38076 Compare July 8, 2019 19:32

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 8, 2019

sttts reviewed Jul 9, 2019

View reviewed changes

ravisantoshgudimetla force-pushed the add-masterSchedulable branch from fc38076 to a0a3fe6 Compare July 9, 2019 13:12

sttts reviewed Jul 9, 2019

View reviewed changes

ravisantoshgudimetla force-pushed the add-masterSchedulable branch from a0a3fe6 to f82d253 Compare July 9, 2019 13:24

sttts reviewed Jul 9, 2019

View reviewed changes

ravisantoshgudimetla added 2 commits July 9, 2019 09:50

Make masters schedulable

62280dd

Generated files

7d8974d

ravisantoshgudimetla force-pushed the add-masterSchedulable branch from f82d253 to 7d8974d Compare July 9, 2019 13:50

openshift-ci-robot assigned sttts Jul 9, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

openshift-merge-robot merged commit 1fc8159 into openshift:master Jul 9, 2019

ravisantoshgudimetla deleted the add-masterSchedulable branch July 9, 2019 15:10

ravisantoshgudimetla mentioned this pull request Jul 11, 2019

Watch scheduler CR and make masters schedulable accordingly openshift/machine-config-operator#937

Merged

Conversation

ravisantoshgudimetla commented Jul 3, 2019

Uh oh!

ravisantoshgudimetla commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Jul 9, 2019

Uh oh!

openshift-ci-robot commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants