Thinking about how TemplateGeneration is used in DaemonSets, I have updated the proposal with what I think as an ok solution for using TemplateGeneration for Deployments and migrating away from hashing PodTemplateSpec.

Thinking about how TemplateGeneration is used in DaemonSets, I have updated the proposal with what I think as an ok solution for using TemplateGeneration for Deployments and migrating away from hashing PodTemplateSpec.

Idea:

• Make the rollback endpoint unset TemplateGeneration iff the revision we are rolling back to has a pod-template-generation.
• Make the Deployment controller set the TemplateGeneration on a Deployment based on pod-template-generation of the matching ReplicaSet. If there is no matching ReplicaSet, list all, and set TemplateGeneration to the highest found + 1.
• The only problem with this flow, is what happens when a user rolls back from foo-6 to foo-5 and his next update is different from what's in foo-6. Should we rewrite foo-6 or advance foo to 7?

@Kargakis thanks for the proposal. It looks great, and I have some thoughts on this:

If I understand this proposal correctly, we want to replace template hash with templateGeneration, mainly for the following reasons:

• Hash collisions (which cause replica set names and label/selector collisions)
• Hash value changed across Kubernetes versions

Upgrading Kubernetes

When we upgrade from a previous Kubernetes version, we need to do something about the old Deployments without templateGeneration:

1. First, update deployment.spec.templateGeneration=N
2. Add label to new RS template.labels: templateGeneration=N
3. Add label to new RS’s pods’ labels: templateGeneration=N
4. Add label to new RS’s labels and selectors: templateGeneration=N
5. Remove label from new RS’s labels and selectors: template-hash=xxx
6. Remove label from new RS’s template.labels: template-hash=xxx
7. Remove label from new RS’s pods’labels: template-hash=xxx
8. Do these for other RSes of the deployment, except that templateGeneration != N and should be unique

We need to remove template-hash labels so that the hash collision won’t affect us.
N can be calculated from the total number of RSes, and each RS has a unique number from 1~N. We can sort RSes by timestamp.

Adoption

• If there are only bare pods to be adopted, we just adopt them blindly (based on labels, since we can’t compare template against pod spec) by creating a new RS with label, template label, and selector templateGeneration=deployment.spec.templateGeneration. Then label the pod with the same label. (Updated: Deployments don't deal with bare pods today and is out of scope in this proposal)
• If there are only one bare RS and some of its pods, we adopt them by doing step 1~4 in the upgrading Kubernetes case where N = 1
• If there are more than one RSes, also doing step 1~4 in the upgrading Kubernetes case

Rollback

As described in the proposal, every time we rollback, we’ll increase templateGeneration by 1, and we’ll need to update new RS label/selector/template label, as well as its pods’ labels.

kubectl version skew

Since we’ll still use revisions for rollback, kubectl should continue to work.

Naming of RSes and pods

Having 2 naming styles is a bit ugly, but I like its simplicity.
If it's required to rename RSes that were created before templateGeneration is introduced, we can delete them non-cascadingly and then recreate them with a different name. However, we can’t rename their pods since that’ll cause unnecessary restarts.

Other things to consider

• We should not allow users to update templateGeneration
• Make sure that templateGeneration won’t collide (no two RSes from the same Deployment will have the same templateGeneration label value at the same time)
• Will templateGeneration and revision cause user confusion?
• This change will complicate deployment...

@janetkuo thanks for the comment

If there are only bare pods to be adopted, we just adopt them blindly (based on labels, since we can’t compare template against pod spec) by creating a new RS with label, template label, and selector templateGeneration=deployment.spec.templateGeneration. Then label the pod with the same label.

Deployments so far don't deal with bare pods and I don't want to deal with this problem in this proposal (probably deserves a separate discussion).

The only problem with switching to TemplateGeneration is that we make adoption harder because the adopted RS will need to match to a TemplateGeneration number in order to separate its Pods from other ReplicaSets owned by the Deployment. Owner references help in that regard but don't necessarily solve the issue[1] so I think we will always need the additional label. How about instead of labeling by using pod-template-generation we switch to use the ReplicaSet name? This could be something that each RS is doing on its own, so we stop doing the relabeling we have been doing in the Deployment controller and get separate non-overlapping sets of labels for free. This probably overlaps with the selector generation proposal.

[1] If we relied just on owner references, orphaning would be a nightmare because all the existing history of a Deployment would be able to adopt any orphaned pods and I am not sure this is desirable hence we will always need the additional label.

Thanks @Kargakis, I read both proposal (this and #477). Using templateGeneration is more seamless from user's perspective but complicates the system; on the other hand, switching hashing algorithm seems to be much less complex, but the users need to manually migrate to use the new hash. I’m leaning a bit towards using the new hash, but I’m not sure if the new hash will bring us new problems. Do you have a preference between the two?

I have another idea, and would love to hear your thoughts. How about avoiding the hash collision by just appending some suffix upon collision? Do you think this would work?

Situation:

1. The known problem is hash collisions. Hash value is used as ReplicaSet selector and name, so it has to be unique per template.
2. We can’t avoid comparing templates of ReplicaSets.

Solution:

1. Make hash value “unique” by appending a suffix on collision. The suffix can be:
   • The hash of timestamp
   • An incremental number
   • A random string
2. That “unique” hash is used as ReplicaSet selector and name.
3. We only need this when the controller tries to create a new ReplicaSet.

If this works, we don't need migration, and we don't need to add more complexity to the controller.

We can’t avoid comparing templates of ReplicaSets.

This is the only source of truth for knowing the new RS for a deployment so I don't think we want to avoid it:)

I think I prefer using the new hashing algorithm because adoption is simpler. With templateGeneration we can still end up running a RS with a name other than the expected because the RS may be adopted. The fact that the hash can change between releases wasn't a problem up to the point we broke the wait of the controller.

Make hash value “unique” by appending a suffix on collision.

I am not sure this is any less complex to achieve (we now have to programmatically find out hash collisions) vs staging a path for migrating to a stable hashing algorithm. The migration step is really temporary and we can force admins to run it between releases.

We can’t avoid comparing templates of ReplicaSets.

This is the only source of truth for knowing the new RS for a deployment so I don't think we want to avoid it:)

Yeah this is a statement of current situation rather than a problem we want to solve.

I intentionally didn't comment on #477 since I think this is better approach and it should be pursued.

@janetkuo I thought about this more and after reviewing #503 I think we want to move forward with TemplateGeneration for Deployments too. I thought this out and the pros outweigh the cons:

1. No more hash collisions
2. We don't need to stage a migration
3. Consistent with the rest of the workload APIs

By following what @kow3ns is proposing in #503:

CurrentTemplateGeneration and UpdatedTemplateGeneration help us in:

4. stop using the revision annotation which has been a PITA to maintain. I always wanted to move it in the API for validation. I think we want to map the annotation to UpdatedTemplateGeneration.
5. Combined with CurrentTemplateGeneration we can gate more the rollout process which will simplify a lot the implementation for ProgressDeadlineSeconds (right now it's ugly).

We can and should use GenerationPartition from #503 as well but in Deployments it can also be a percentage. We will need some math here for calculating rollout percentages but it should be fun to write.

We can and should use GenerationPartition from #503

For canary, custom, and A/B deployments

cc: @mfojtik @smarterclayton

stop using the revision annotation which has been a PITA to maintain.

@Kargakis Would you elaborate more on why it's a PITA?

Just thought about this issue more. I don't think the migration step is necessary, if we switch to the new / more stable hash function. After switching to the new hash, no unnecessary new RS will be created, since we'll look at current RS template before creating a new one and we don't compare hash value directly.

@Kargakis Would you elaborate more on why it's a PITA?

See fixes like kubernetes/kubernetes#42535 or kubernetes/kubernetes#42869

Just thought about this issue more. I don't think the migration step is necessary, if we switch to the new / more stable hash function. After switching to the new hash, no unnecessary new RS will be created, since we'll look at current RS template before creating a new one and we don't compare hash value directly.

What if a hash produced with the new hashing algo collides with a hash produced with the old hashing algo?

What if a hash produced with the new hashing algo collides with a hash produced with the old hashing algo?

True, but can we guarantee that the new hashing algo will never collide? If not, we can uniquify the hash value when a collision happens. It should also be idempotent so that the Deployment controller won’t create more new RS than required.

So we can do this by:

1. First, switch to the new hash algo (without migration), and then
2. Add a new field to Deployment: uniquifier (tentative name, perhaps under status)
   • This value will only increase
   • This value will be updated when there’s hash collision
3. When the Deployment template is updated (either rollback or rollout):
   • The Deployment controller first searches for current RSes and compares their template with the deployment’s (current behavior)
   • If found a match, that RS is the new RS (current behavior)
   • If no matches found, create a new RS, with hash{template+uniquifier} (use this hash value for RS name and label selectors and pod labels)
     • If the creation failed because the RS already exists, check the existing RS template with Deployment template to determine if it’s a collision or it’s a new RS the controller just created (but haven’t observed the creation)
     • If the new RS is already created, no op (mission completed)
     • If it’s a collision, increase uniquifier by 1, and try again in the next loop
     • If the creation failed for any other reasons, the controller will retry in the next loop, and the hash value will be idempotent because we use the same template and the same uniquifier

Please let me know what you think about this.

@janetkuo I like the idea but the uniquifier now changes the generated names every time a hash collision is detected, which means that it also increases the probability for hash collisions. I think it is very hard to programmatically know when a hash collision has happened w/o making the underlying ReplicaSets read-only otherwise they may change by users in subtle ways that is impossible for us to detect. Read-only children is a problem on its own though and I am going to open a separate issue for discussion.

Read-only children issue: kubernetes/kubernetes#44237

Updated the proposal with my latest thoughts on selector generation for ReplicaSets and rollbacks.

Closing in favor of #477