Ability to specify default tolerations via the buildconfig defaulter #17955

coreydaley · 2018-01-02T04:13:30Z

Trello: https://trello.com/c/LNxlMjjU/1435-5-ability-to-specify-default-tolerations-via-the-buildconfig-defaulter-builds

coreydaley · 2018-01-02T04:14:57Z

(5) Ability to specify default tolerations via the buildconfig defaulter [builds]

gabemontero · 2018-01-02T14:26:48Z

ah - just saw the WIP tag @coreydaley - only looked cause the bot assigned me and I was on auto pilot

coreydaley · 2018-01-02T21:07:39Z

@openshift/sig-developer-experience ptal

bparees · 2018-01-02T21:12:53Z

pkg/build/controller/build/defaults/api/v1/types.go

+
+	// tolerations is a list of default Tolerations that are applied to build pods
+	// so that they can be placed on build-specific nodes
+	Tolerations []kapi.Toleration `json:"tolerations,omitempty"`


@deads2k should this whole file not have moved into the api repo?

@deads2k should this whole file not have moved into the api repo?

The admission ones need to move. Found it late last month. Is this still configured via admission? Try to fix that in 3.9, so we can fix the config in 3.10.

coreydaley · 2018-01-02T21:15:52Z

@openshift/api-review ptal

bparees · 2018-01-02T21:15:58Z

pkg/build/controller/build/defaults/api/types.go

+
+	// tolerations is a list of default Tolerations that are applied to build pods
+	// so that they can be placed on build-specific nodes
+	Tolerations []kapiv1.Toleration


internal api objects should not be using versioned k8s api types.

bparees · 2018-01-02T21:17:15Z

pkg/build/controller/build/defaults/defaults.go

@@ -100,6 +100,12 @@ func (b BuildDefaults) applyPodDefaults(pod *v1.Pod) {
 			}
 		}
 	}
+
+	if pod.Spec.Tolerations == nil && len(b.config.Tolerations) != 0 {


if pod.Spec.Tolerations is nil, you should be creating a new array and populating it.

(actually I think w/ arrays you can just always append, you don't need to explicitly instantiate the empty array first)

but regardless you should be adding the default tolerations to the array. the defaulting should be done on a key by key basis. Same way the annotations are being done currently.

Nil and zero length are equivalent here, and you should check zero length

bparees · 2018-01-02T21:18:05Z

pkg/build/controller/build/defaults/defaults.go

+
+	if pod.Spec.Tolerations == nil && len(b.config.Tolerations) != 0 {
+		for _, toleration := range b.config.Tolerations {
+			pod.Spec.Tolerations = append(pod.Spec.Tolerations, toleration)


since we're calling this a defaulter, you should make sure the config.Toleration.Key does not exist in pod.Spec.Tolerations before appending it. (there's no reason to expect it to today, but that could change someday).

coreydaley · 2018-01-03T17:03:07Z

@bparees feedback addressed, ptal

bparees · 2018-01-03T17:45:45Z

pkg/build/controller/build/defaults/defaults.go

@@ -100,6 +103,14 @@ func (b BuildDefaults) applyPodDefaults(pod *v1.Pod) {
 			}
 		}
 	}
+
+	// Apply default Tolerations
+	if len(pod.Spec.Tolerations) == 0 && len(b.config.Tolerations) != 0 {


I think we decided you should append these even if pod.Spec.Tolerations already contains some tolerations?

bparees · 2018-01-03T17:49:33Z

pkg/build/controller/build/defaults/defaults.go

+	tol := v1.Toleration{}
+
+	if err := kapiv1.Convert_core_Toleration_To_v1_Toleration(&t, &tol, nil); err != nil {
+		panic(err)


you can't call panic, that would crash the api server. util.HandleErr to log it, and ultimately you need to bubble this error up to ApplyDefaults so it can be returned from there.

coreydaley · 2018-01-03T18:13:22Z

@bparees comments addressed

bparees · 2018-01-03T18:52:17Z

pkg/build/controller/build/defaults/defaults.go

 	tol := v1.Toleration{}

 	if err := kapiv1.Convert_core_Toleration_To_v1_Toleration(&t, &tol, nil); err != nil {
-		panic(err)
+		utilruntime.HandleError(fmt.Errorf("Unable to convert core.Toleration to v1.Toleration: %v", err))


you should return the same error you're logging.

bparees · 2018-01-03T18:52:42Z

pkg/build/controller/build/defaults/defaults.go

@@ -218,18 +226,20 @@ func addDefaultAnnotation(k, v string, annotations map[string]string) {
 	}
 }

-func addDefaultToleration(t kapi.Toleration, tolerations *[]v1.Toleration) {
+func addDefaultToleration(t kapi.Toleration, tolerations *[]v1.Toleration) error {
 	tol := v1.Toleration{}

 	if err := kapiv1.Convert_core_Toleration_To_v1_Toleration(&t, &tol, nil); err != nil {


@deads2k sanity check for the right way to do this.

coreydaley · 2018-01-04T00:44:49Z

/retest

coreydaley · 2018-01-04T15:19:48Z

/retest

bparees · 2018-01-04T15:21:52Z

@openshift/api-review bump.

liggitt · 2018-01-04T15:52:01Z

pkg/build/controller/build/defaults/api/v1/types.go

@@ -43,6 +43,10 @@ type BuildDefaultsConfig struct {

 	// resources defines resource requirements to execute the build.
 	Resources kapi.ResourceRequirements `json:"resources,omitempty"`
+
+	// tolerations is a list of default Tolerations that are applied to build pods
+	// so that they can be placed on build-specific nodes


needs good doc about what happens in the following cases:

no tolerations on the pod, tolerations set here (assume these are all added verbatim)

unrelated tolerations on the pod already, tolerations set here (assume these are appended)

some of these toleration keys exist on the pod already, but with different values/effects (do these override or preserve what is already set on the pod?)

@liggitt As of now, the only way that tolerations are set on build pods is through the defaulter, so there would be no other tolerations. If/when there comes a time that users (or whoever) can add tolerations, then all of these things should be considered and addressed, but for now it might be overkill?
@bparees thoughts?

for now it makes sense to state that these are appended to any existing tolerations on the build pod.

if there are multiple tolerations with the same key but different values, i would expect us to forgo adding the default toleration entry.

(note that even the key is optional in a Toleration, so it's quite a mess deciding when two tolerations should replace each other vs both be added)

@bparees updated in next push

@bparees tolerations seem to be processed as a combination of the key/value/effect exactly equaling the taint, or the key/effect existing, and the value being blank, so it seems like skipping a toleration based on the key matching would not be the best option.

@bparees updated description based on your feedback

bparees · 2018-01-04T20:13:13Z

pkg/build/controller/build/defaults/api/types.go

@@ -46,8 +46,7 @@ type BuildDefaultsConfig struct {
 	// resources defines resource requirements to execute the build.
 	Resources kapi.ResourceRequirements

-	// tolerations is a list of default Tolerations that are applied to build pods
-	// so that they can be placed on build-specific nodes
+	// tolerations is a list of default Tolerations that are appended to build pods


are appended to the build pod's set of tolerations.

updated in current push

coreydaley · 2018-01-04T22:04:50Z

flake #17995

coreydaley · 2018-01-05T00:24:34Z

/retest

coreydaley · 2018-01-05T01:53:45Z

/retest

coreydaley · 2018-01-05T12:26:16Z

/retest

coreydaley · 2018-01-05T14:56:26Z

@openshift/api-review ptal at the api changes and let me know if they look ok

bparees · 2018-01-05T15:02:51Z

It does raise the question, why is the node selector even in the defaults at all?

it made more sense when it actually operated as an admission controller since other things might have put nodeselector values on the pod before the defaulter+overrider saw the pod object.

And given that nodeselector does exist as both an override and a default, that would seem to further the argument that the Toleration annotation could reasonably be placed in either (it could also be placed in both, if you really want it, but it would be pointless).

liggitt · 2018-01-05T15:15:00Z

And given that nodeselector does exist as both an override and a default, that would seem to further the argument that the Toleration annotation could reasonably be placed in either (it could also be placed in both, if you really want it, but it would be pointless).

Thinking about how a user would have to configure this to get build isolation, it's just odd to make them configure both defaults (containing tolerations) and overrides (containing selectors).

bparees · 2018-01-05T15:26:52Z

@openshift/api-review isn't going to approve this w/o moving the Tolerations value into the overrides config (which means changing the behavior such that the override Tolerations array completely replaces the existing Tolerations array on the pod, if any). @coreydaley please rework the PR accordingly so we can land this for devcut.

coreydaley · 2018-01-05T17:02:44Z

@bparees ptal
migrated the code over to overrides

coreydaley · 2018-01-05T18:06:25Z

/retest

bparees · 2018-01-05T18:17:37Z

pkg/build/controller/build/overrides/overrides.go

+		for _, toleration := range b.config.Tolerations {
+			t := v1.Toleration{}
+
+			if err := kapiv1.Convert_core_Toleration_To_v1_Toleration(&toleration, &t, nil); err != nil {


@deads2k is this an ok way to convert from the internal toleration object to external?

bparees · 2018-01-05T18:18:35Z

test/integration/buildpod_admission_test.go

+	for i, toleration := range tolerations {
+		tol := v1.Toleration{}
+		if err := kapiv1.Convert_core_Toleration_To_v1_Toleration(&toleration, &tol, nil); err != nil {
+			panic(err)


while you "can" panic here, it's not going to make it easier to investigate the failure. just fail the test w/ the error.

bparees · 2018-01-05T18:19:39Z

@openshift/api-review location of the tolerations field has been moved. please review.

Trello: https://trello.com/c/LNxlMjjU/1435-5-ability-to-specify-default-tolerations-via-the-buildconfig-defaulter-builds

liggitt · 2018-01-05T18:39:50Z

config api change lgtm

bparees · 2018-01-05T18:43:33Z

/lgtm

openshift-ci-robot · 2018-01-05T18:43:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, coreydaley

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apps/OWNERS~~ [bparees]
~~pkg/build/OWNERS~~ [bparees]
~~test/integration/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

coreydaley · 2018-01-05T20:27:06Z

/retest

coreydaley · 2018-01-06T00:27:35Z

/retest

openshift-ci-robot · 2018-01-06T02:12:53Z

@coreydaley: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_builds	`bf3275f`	link	`/test extended_builds`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-merge-robot · 2018-01-06T11:30:35Z

Automatic merge from submit-queue.

…t_tolerations_via_buildconfig_defaulter Automatic merge from submit-queue. Ability to specify default tolerations via the buildconfig defaulter

…ons_via_buildconfig_defaulter Automatic merge from submit-queue. Ability to specify default tolerations via the buildconfig defaulter Trello: https://trello.com/c/LNxlMjjU/1435-5-ability-to-specify-default-tolerations-via-the-buildconfig-defaulter-builds Dependent on: openshift/origin#17955

openshift-ci-robot requested review from abhgupta and gabemontero January 2, 2018 04:13

openshift-merge-robot added the needs-api-review label Jan 2, 2018

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 2, 2018

coreydaley changed the title ~~Ability to specify default tolerations via the buildconfig defaulter~~ [WIP] Ability to specify default tolerations via the buildconfig defaulter Jan 2, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 2, 2018

gabemontero removed their request for review January 2, 2018 14:27

This was referenced Jan 2, 2018

Ability to specify default tolerations via the buildconfig defaulter openshift/openshift-docs#6974

Merged

Ability to specify default tolerations via the buildconfig defaulter openshift/openshift-ansible#6579

Merged

coreydaley changed the title ~~[WIP] Ability to specify default tolerations via the buildconfig defaulter~~ Ability to specify default tolerations via the buildconfig defaulter Jan 2, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 2, 2018

bparees reviewed Jan 2, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 3, 2018

bparees requested changes Jan 3, 2018

View reviewed changes

bparees reviewed Jan 3, 2018

View reviewed changes

liggitt reviewed Jan 4, 2018

View reviewed changes

bparees reviewed Jan 4, 2018

View reviewed changes

bparees reviewed Jan 5, 2018

View reviewed changes

Ability to specify default tolerations via the buildconfig defaulter

bf3275f

Trello: https://trello.com/c/LNxlMjjU/1435-5-ability-to-specify-default-tolerations-via-the-buildconfig-defaulter-builds

liggitt added the api-approved label Jan 5, 2018

openshift-ci-robot assigned bparees Jan 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2018

coreydaley removed the request for review from abhgupta January 5, 2018 21:53

openshift-merge-robot merged commit c8c51ba into openshift:master Jan 6, 2018

Ability to specify default tolerations via the buildconfig defaulter #17955

Ability to specify default tolerations via the buildconfig defaulter #17955

Conversation

coreydaley commented Jan 2, 2018

coreydaley commented Jan 2, 2018

gabemontero commented Jan 2, 2018

coreydaley commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jan 4, 2018

coreydaley commented Jan 4, 2018

bparees commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jan 4, 2018

coreydaley commented Jan 5, 2018

coreydaley commented Jan 5, 2018

coreydaley commented Jan 5, 2018

coreydaley commented Jan 5, 2018

bparees commented Jan 5, 2018

liggitt commented Jan 5, 2018

bparees commented Jan 5, 2018

coreydaley commented Jan 5, 2018

coreydaley commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jan 5, 2018 • edited by coreydaley Loading

liggitt commented Jan 5, 2018

bparees commented Jan 5, 2018

openshift-ci-robot commented Jan 5, 2018

coreydaley commented Jan 5, 2018

coreydaley commented Jan 6, 2018

openshift-ci-robot commented Jan 6, 2018 • edited Loading

openshift-merge-robot commented Jan 6, 2018

bparees commented Jan 5, 2018 •

edited by coreydaley

Loading

openshift-ci-robot commented Jan 6, 2018 •

edited

Loading