deploy: update validation for deploymentconfigs

[0xmichalis]

Validate that the deployment config selector is matching the pod
template labels.

cc: @ironcladlou @smarterclayton @mfojtik

[liggitt]

What are the implications if existing DCs suddenly stop validating because of this?

[0xmichalis]

@liggitt those DCs are already broken because the same validation underpins replication controllers:
https://github.com/kubernetes/kubernetes/blob/7476d97781563b70e8b89a8bd3f99ea75ae6c290/pkg/api/validation/validation.go#L2248

[liggitt]

so they would have failed when actually deploying, but do we have any automated controllers that will try to update the now-invalid DC (to report status, bump versions to trigger deployments, etc), that will behave in a way we don't want (hot-looping, clogging workqueues, etc)?

[0xmichalis]

We are gating retries in both the deployment config and config change controller. I believe that for 1.3 we will switch to using caches and the ratelimiting queue so I think we are covered.

[liggitt]

what about client calls to trigger deployments? would they get a validation error now?

[0xmichalis]

Yes. Much better than silently starting an invalid deployment.

[liggitt]

Much better than silently starting an invalid deployment.

depends. it makes sense when creating the DC. it makes sense if updating the DC from a valid selector to an invalid selector. if you are not changing the selector, and are making an unrelated API call, a validation error about the selector is somewhat nonsensical

[0xmichalis]

If it was any other field, I would probably agree but isn't it also nonsensical to allow updates on a DC when it is not working? If I run an update on my (invalid) DC and it succeeded I would expect it to work. But it wouldn't...

[liggitt]

isn't it also nonsensical to allow updates on a DC when it is not working

No... different updates do different things, and if data was previously allowed in, it's not clear we should disallow unrelated updates in order to tighten validation. I'm not 100% opposed, but the bar needs to be very high for tightening validation like this, and we need to think through all the automated and manual update cases to make sure we're not going to cause bigger problems.

[0xmichalis]

It's not about tightening validation, it's about not letting people break themselves or at the very least, letting them know where are they broken, if they are already broken. Disallowing updates like scaling or other template changes that never worked doesn't sound that bad. Plus if someone had hit this already, we would probably know about it.

[liggitt]

It's not about tightening validation, it's about not letting people break themselves or at the very least, letting them know where are they broken, if they are already broken.

Yes, by tightening validation :)

Disallowing updates like scaling or other template changes that never worked doesn't sound that bad.

Depends how that is being done. If a script or automated system is scaling and is not equipped to understand validation errors (because validation errors were not possible before), this isn't helpful. Like I said, I'm not 100% opposed, but we have to be super careful when tightening validation, because it introduces the potential to make unrelated operations fail.

[0xmichalis]

Depends how that is being done. If a script or automated system is scaling and is not equipped to understand validation errors (because validation errors were not possible before), this isn't helpful. Like I said, I'm not 100% opposed, but we have to be super careful when tightening validation, because it introduces the potential to make unrelated operations fail.

I can't think of such a system that wouldn't be already broken. Two cases: 1) scaling events (either by an autoscaler or another automated system or script) = already broken. 2) Updates to the deployment config that will cause[1] a redeployment (either automatic or manual) = already broken.

[1] most if not all of the time you want to redeploy your latest config. Not sure if there is any value in having an updated deployment config that is not working. And I don't think there is an automated system out there that will fix the validation issue for the deployment config to work:)

[smarterclayton]

It would mean, for instance, that an oc label or oc annotate call would fail until you fixed the DC. Since the DC is broken, you'd at least know that the DC was broken.

If there is no case where a DC can exist today that fails this validation, but isn't broken, then this is ok. If there is a DC today that would be working but when we add this validation it breaks (can't trigger redeployment on image?) then I would be concerned. Can we conclusively prove that there is scenario where the DC covered by this new validation would be broken?

[0xmichalis]

DCs that will break by this change are already broken.
On master: http://pastebin.com/ZXxLd0WX
The previous deployment is working fine, but any update to the DC will never be reflected back to it.
The new replication controller fails validation so we will never have a new deployment if the validation error is not fixed

From the server logs

E0615 15:01:41.539233    1980 factory.go:69] couldn't create deployment for deployment config broken/deployment-simple: ReplicationController "deployment-simple-2" is invalid: spec.template.metadata.labels: Invalid value: {"deployment":"deployment-simple-2","deploymentconfig":"deployment-simple","name":"deployment-simple-broken"}: `selector` does not match template `labels`

[smarterclayton]

Then I'm ok with the change - Jordan, do yo need more convincing?

[0xmichalis]

@liggitt ping

[0xmichalis]

@liggitt ping - better get this in for 1.3 than not?

[liggitt]

yeah, I think this is ok

[0xmichalis]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8386/) (Image: devenv-rhel7_4917)

[0xmichalis]

quota [merge]

[mfojtik]

@Kargakis while you are here, can you also fix #10570 ?

[0xmichalis]

@mfojtik that issue needs some discussion

#8427 [merge]

[0xmichalis]

[test]

[openshift-bot]

Evaluated for origin test up to 7ade0ff

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8386/)

[0xmichalis]

[merge]

[0xmichalis]

#8427 [merge]

[openshift-bot]

Evaluated for origin merge up to 7ade0ff