Clients shouldn't mutate DC status

[0xmichalis]

oc deploy currently bumps the dc version (dc.Status.LatestVersion) to trigger a deployment. LatestVersion, being a part of the Status of the DeploymentConfig, shouldn't be mutated by clients.

Ref #6246 (comment)

cc: @ironcladlou @smarterclayton @deads2k

[deads2k]

Since that comment may disappear:

It seems like we'd want to restructure this so that a client requests a redepoyment in the spec and a controller creates one.

@openshift/api-review This is a pre-existing problem and would be an API change that has consequences for backwards compatibility, but this decision here means that we can't trust this Status object to be accurate until we stop users from tweaking it.

[ironcladlou]

This is another case where we mistakenly added logic to the client which should live on the server. I believe it was discussed long ago, but there should be some sort of endpoint (sub resource?) to instantiate a new deployment.

[liggitt]

at the very least, there should be split spec/status versions, so someone (a client, controller, etc) can indicate the desire for a new deployment, and something else (the deployment controller) can tell whether that was fulfilled or not. I could see a subresource that just bumps the spec.version as well

[smarterclayton]

We should be using generation for this - like RC. There should be one in
status and one in spec. updating spec is the minimal trigger to force a
new deployment of the same thing.

On Wed, Dec 16, 2015 at 9:52 AM, Jordan Liggitt [email protected]
wrote:

at the very least, there should be split spec/status versions, so someone
(a client, controller, etc) can indicate the desire for a new deployment,
and something else (the deployment controller) can tell whether that was
fulfilled or not. I could see a subresource that just bumps the
spec.version as well

—
Reply to this email directly or view it on GitHub
#6337 (comment).

[0xmichalis]

We should be using generation for this - like RC. There should be one in status and one in spec. updating spec is the minimal trigger to force a new deployment of the same thing.

Upstream are planning to add ObservedGeneration to Deployments so we have one more reason to do this:)

[0xmichalis]

We should be using generation for this - like RC. There should be one in status and one in spec. updating spec is the minimal trigger to force a new deployment of the same thing.

Note that the generation for RCs changes when the spec changes (eg. number of replicas) and not only when podTemplate changes occur.

[0xmichalis]

So my question here is how are we going to differentiate between generation bumps that are latestVersion bumps and generation bumps where the template hasn't changed or the user hasn't triggered manually?

[smarterclayton]

Does that distinction matter? If a deployment happens due to a trigger it should increment the generation (like we increment version). Version should be completely replaced by generation.

[0xmichalis]

Each new version results in a new replication controller. Generation is RCs is incremented just by scaling the controller. If we do the same for deployments, then each generation would result in a new controller? Or shouldn't generation be incremented when a deployment is scaled (why generation was added in rcs: kubernetes/kubernetes#19581 (comment)) ?

[smarterclayton]

Generation is intended to help convey the delta between desired state and
whether the cluster believes the state has been converged. Scale is
relevant - the cluster is acknowledging that it saw and accepted the
request to change the scale, regardless of whether the actual scale at that
point in time matches the desired scale.

On Sun, Jan 24, 2016 at 5:25 PM, Michail Kargakis [email protected]
wrote:

Each new version results in a new replication controller. Generation is
RCs is incremented just by scaling the controller. If we do the same for
deployments, then each generation would result in a new controller? Or
shouldn't generation be incremented when a deployment is scaled (why
generation was added in rcs: kubernetes/kubernetes#19581 (comment)
kubernetes/kubernetes#19581 (comment))
?

—
Reply to this email directly or view it on GitHub
#6337 (comment).

[0xmichalis]

We cannot rely then on generation numbers for triggering deployments like we do with LatestVersion in oc deploy.

[smarterclayton]

Not sure what you mean - generation should usually flow from the spec, having a spec field that contains something mutable but insignificant is sufficient. Having user set generation usually feels wrong. In image streams I allow spec tag generation to be explicitly set to zero, which does trigger a new state. But spec tag gen is a pointer so we can distinguish between unset and default, and Kube gen is not nillable.

[0xmichalis]

Let me rephrase my earlier question:

How will the dc controller be able to distinguish between a generation bump that comes from oc deploy --latest or oc rollout and a generation bump that comes from the registry update hook due to a change in the spec? In the first case we want a new deployment of the same thing to run, in the second case we track whether the controller has observed the latest state of the dc.

[smarterclayton]

oc deploy --latest has to bump the spec somehow. Usually that's "force
deployed by bob @ 10:34pm because REASON"

On Fri, Jan 29, 2016 at 2:28 PM, Michail Kargakis [email protected]
wrote:

Let me rephrase my earlier question:

How will the dc controller be able to distinguish between a generation
bump that comes from oc deploy --latest

origin/pkg/cmd/cli/cmd/deploy.go

Line 223 in 9431c7e

config.Status.LatestVersion++

or oc rollout and a generation bump that comes from the registry update
hook due to a change in the spec? In the first case we want a new
deployment of the same thing to run, in the second case we track whether
the controller has observed the latest state of the dc.

—
Reply to this email directly or view it on GitHub
#6337 (comment).

[0xmichalis]

Generation is not what we want here. How about a spec.version field? And maybe have it updated via a subresource? @liggitt and @ironcladlou already mentioned those, not sure why I didn't process them until now

[smarterclayton]

Ultimately both types of clients should mutate the DC (or invoke an endpoint like instantiate) that masks this. We can't drop support for old clients mutating the client - so we have to continue to have a field that works with the existing call patterns. However, we can take control of the edit and say "the presence of a status.Version that is not zero and differs from the current value counts as a mutation to spec.version in the DC". However, both types of updates should result in spec.version being updated, which is effectively generation. So I don't know why we wouldn't have generation.

[smarterclayton]

If updating status.version did the same thing as updating metadata.generation, and that was the initial value that was used to convey changes to the effective deployment spec (the part that triggers mutation on config), then an instantiate / rollout endpoint can be added.

[smarterclayton]

Basically: can't drop back compat. Need to handle it during strategy update. Should use generation. Should not let the user increment by more than 1. Need to figure out how this translates to the deployment controller. Better to have endpoint to trigger equivalent mutation when necessary.

[0xmichalis]

Ah, now it's clearer to me. I will start working on this asap since it's blocking a bunch of other issues.

[smarterclayton]

You can see an example of generation in image stream - that was different from here because we needed per tag generation, and old clients didn't set it, so we had to treat nil as "don't care" and 0 as "new". I think here though we want to treat status.LatestVersion as "may only increment by one or be equal, otherwise this is treated as a conflict". However, we'd need to do backcompat tests to validate that.