add nodeselector and annotation build pod overrides and defaulters #11380

bparees · 2016-10-14T23:12:19Z

This PR allows cluster admins to set override nodeselectors and annotations(primarily intended to be used for tolerations) that will be applied to build pods.

Admins can also set default nodeselectors/annotations which will be applied to the build pod only if the pod does not contain those selector/annotation keys already.

Lastly it also allows users to set nodeselectors on their buildconfig, those nodeselectors will be applied to the build pod (but be overridden by the admin-configured override values)

fixes #9971

fixes bug 1277432

bparees · 2016-10-14T23:12:27Z

[test]

bparees · 2016-10-14T23:12:51Z

[testextended][extended:core(builds)]

bparees · 2016-10-17T16:16:29Z

@openshift/api-review this is ready for review.

@deads2k it could also use your review of how i'm initializing the config+controller now with this change since it's not actually an admission controller anymore, but it's still reading/using the admission plugin config.

liggitt · 2016-10-17T17:32:19Z

pkg/build/admission/defaults/admission.go

 )

-func init() {
-	admission.RegisterPlugin("BuildDefaults", func(c clientset.Interface, config io.Reader) (admission.Interface, error) {


to continue allowing old configs to run, do we need to register a no-op plugin with this name that logs a warning that the admission plugin is deprecated and returns nil? same question for BuildOverrides?

I don't think so? we're still honoring the admission plugin config, we're just using it in a different codepath instead of as an admission controller.

@deads2k, did we want to remove direct control of the admission chain in 1.4? if so, then no, I don't think we need to maintain the names as dummy plugins

@deads2k, did we want to remove direct control of the admission chain in 1.4? if so, then no, I don't think we need to maintain the names as dummy plugins

I don't strictly object to doing it 1.4, but I hadn't set a timeframe. I'm not entirely sure what the validator will do on admission config for a mismatched name.

Also, it's pretty weird to drive config for a controller from the admission config. If we do end up with split processes, this is going to be a problem. This would have instantly broken.

it's the only way I can see to make this change and maintain backwards compatibility unless you've got another idea, and @smarterclayton insisted the admission controllers should go away.

i'm going to need you guys to elaborate on this admission chain issue, i don't know what you're referring to.

liggitt · 2016-10-17T17:36:52Z

pkg/build/admission/defaults/admission.go

-	}
-	if !buildadmission.IsBuildPod(attributes) {
+// ApplyDefaults applies configured build defaults to a build pod
+func (b BuildDefaults) ApplyDefaults(pod *kapi.Pod) error {


can we take the build as an arg instead of parsing from the annotation? looks like all callers have the build handy (same question applies to ApplyOverrides, I guess)

are the possible mutations to the build in applyBuildDefaults() things we want persisted? if not, we could pass in buildCopy

Right now we don't mutuate the actual build object in the admission controller, only the build as it is defined in the pod, so no, the mutations are not things we want persisted.

we can't just take the build as an arg because we also need the version info that is part of the serialized form of the build in the annotation. we have to keep the serialized version consistent before/after mutating the value.

(also taking the build as an arg would mean warning callers that the object they pass in is being mutated but should not be persisted after that mutation, to retain the existing behavior, or we'd have to make a copy within this method)

liggitt · 2016-10-17T17:50:24Z

pkg/build/admission/overrides/api/types.go

@@ -17,4 +19,10 @@ type BuildOverridesConfig struct {
 	// If user provided a label in their Build/BuildConfig with the same name as one in this
 	// list, the user's label will be overwritten.
 	ImageLabels []buildapi.ImageLabel
+
+	// nodeSelector is a selector which must be true for the build pod to fit on a node
+	NodeSelector map[string]string


it looks like these defaults are applied key-by-key? so if I have a build with "nodeSelector": {"foo":"bar"} and defaults of "nodeSelector":{"baz":"qux"}, I'll get a pod with "nodeSelector":{"foo":"bar","baz":"qux"}. Is that what we want for the defaulting?

yes and yes. the intent is that an admin might set a default nodeSelector of "targetNode:defaultBuildNode" but the user might want to say "targetNode:highMemoryBuildNode" instead.

That works as long as a build never needs a selector that omits one of the default keys. I'm not sure what level of granularity is needed.

my view of the default nodeselector was "must be at least this specific".

there are probably competing use cases. but my assumption is most admins are going to set their system up with node labels like "buildNode=foo", and a defaulter that assigns the buildNode nodeselector key, which ensures that all builds will land on a node with at least the "buildNode" key, of some value.

liggitt · 2016-10-17T17:52:59Z

pkg/build/admission/overrides/admission.go

-	if build.Spec.Strategy.SourceStrategy != nil {
-		glog.V(5).Infof("Setting source strategy ForcePull to true in build %s/%s", build.Namespace, build.Name)
-		build.Spec.Strategy.SourceStrategy.ForcePull = true
+	for k, v := range b.config.NodeSelector {


do we want to do the same thing as the project nodeSelector and error if the build already specifies a conflicting key/value, or just stomp it?

well obviously i want to stomp it :)

if they create a build with a nodeSelector, is it more confusing to fail build pod creation with errors about disallowed elements in the nodeSelector, or to create build pods that apparently ignore the nodeSelector they set? I'm really not sure...

i'm not sure either. i kind of prefer the path where the build succeeds for them, but i agree they aren't necessarily getting what they expected nor will they necessarily understand why (assuming they even notice that it didn't run on the node they expected)

liggitt · 2016-10-17T17:53:21Z

pkg/build/admission/overrides/admission.go

-	return nil
+	for k, v := range b.config.Annotations {
+		glog.V(5).Infof("Adding override annotation %s=%s to build pod %s/%s", k, v, pod.Namespace, pod.Name)
+		pod.Annotations[k] = v


same question about erroring or stomping on conflicting key/value

same answer :)

smarterclayton · 2016-10-18T01:16:39Z

It was probably a mistake to assume that policy config related to things that happened in admission config would continue to be implemented the same way. Admission is an implementation detail. When we change our process model we will already have to deal with existing config continuing working for some period of time. In any case we should not break config on disk just because implementation changed.

bparees · 2016-10-18T15:17:31Z

flake #11240

bparees · 2016-10-18T15:30:36Z

@smarterclayton @liggitt @deads2k

The open questions on this PR appear to be:

should override stomp on the user's value, or result in an error/failure (and is this conflict on a key by key level, or is it an error for the user to set any key, if the override is also going to set some key(s))
should default keys merge with the user's keys, or get ignored if the user set any keys

And these questions may have different answers for nodselectors vs annotations (it's hard to imagine we'd want to fail all user builds that have an annotation set, when a non-conflicting override annotation is defined. Ditto for ignoring default annotations just because the user set some random annotation on their build).

We need this questions resolved asap so this can land for 3.4 feature cut. Especially if the answer is that the current choices are not acceptable and the behavior needs to be changed.

bparees · 2016-10-18T17:46:22Z

flake #11240
[test]

pweil- · 2016-10-20T15:04:57Z

We need this questions resolved asap so this can land for 3.4 feature cut

@openshift/api-review bump for questions in #11380 (comment). I'd prefer the route that does not fail the build, especially if we can provide a message that their selection was overridden by cluster settings.

smarterclayton · 2016-10-20T21:54:18Z

Suggestions:

annotations
- overrides = stomp key by key
- defaults = default key by key
selector
- defaults = if selector is completely empty, default, otherwise do nothing
- overrides = for each label specified by the admin, update the value (this will be broken when label selectors are more complex)

liggitt · 2016-10-20T22:08:59Z

so I think the only change from the current state is making the nodeSelector defaulting all-or-nothing

follow up issue to try to surface stompage to the user would be nice but non-blocking

smarterclayton · 2016-10-20T22:11:30Z

Yeah definitely non-blocking.

bparees · 2016-10-20T22:11:32Z

yeah, working on that change right now.

bparees · 2016-10-21T04:31:46Z

flake #11477
flake #11094
[test]

bparees · 2016-10-21T04:32:05Z

default selector is now all or nothing. @csrwng ptal.

bparees · 2016-10-21T04:32:51Z

(and per @liggitt's suggestion, map{} results in no defaults being applied. nil results in defaults being applied. so users can avoid the defaults if they want)

bparees · 2016-10-21T15:57:23Z

flake #11452

bparees · 2016-10-21T15:58:35Z

@jupierce ptal

jupierce

LGTM.

jupierce · 2016-10-21T19:09:56Z

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

 	// build if the specified variables do not exist on the build
 	Env []kapi.EnvVar `json:"env,omitempty"`

-	// SourceStrategyDefaults are default values that apply to builds using the
+	// sourceStrategyDefaults are default values that apply to builds using the
 	// source strategy.
 	SourceStrategyDefaults *SourceStrategyDefaultsConfig `json:"sourceStrategyDefaults,omitempty"`

 	// ImageLabels is a list of docker labels that are applied to the resulting image.


You lowercased every attribute name in doc except this one.

result of rebase/merge. but yeah i'll lowercase it.

jupierce · 2016-10-21T20:21:50Z

pkg/build/admission/config.go

+	defer configData.Close()
+
+	configBytes, err := ioutil.ReadAll(configData)
+
 	err = configlatest.ReadYAMLInto(configBytes, config)


ReadYAMLFileInto instead?

thanks. i just moved this code from somewhere else but it makes sense to update it to use that instead.

jupierce · 2016-10-21T20:23:14Z