KEP-3973: Consider Terminating Pods in Deployments

[atiratree]

• One-line PR description: Deployment should consider terminating pods when performing new rollouts and scaling.

• Issue link: Consider Terminating Pods in Deployments #3973

• Other comments:
  • supersedes WIP: KEP-3973: Consider Terminating Pods in replica sets #3974
  • similar feature for Jobs: Allow for recreation of pods once fully terminated in the job controller #3939

[kannon92]

/cc

[kannon92]

Looks really good! I have a few minor comments. We should get someone from api-review to decide on the name but otherwise, I think it looks good.

We should also consider metrics for this feature even if we don't implement them in alpha.

[kannon92]

The mix of past and current tense I find to be a bit odd..

Maybe instead of Terminated, we could use Terminal?

[kannon92]

IMO I would maybe ask in api-review to see if we can get some kind of consensus on name.

[atiratree]

I only saw Terminal being used in reference to the phases. I think Terminated is less ambiguous.

But for sure, let's bring this up in the api review.

[knelasevero]

Maybe another option is to explicitly name this SucceededOrFailed? Was this already discussed in api review? Job has TerminatingOrFailed in their case, right?

I actually like Terminated, just thought about bringing this up

[atiratree]

the PR still has to go through API review

• IIRC for Jobs, failed has a different meaning than just the pod status. The pod may fail or may not according to the PodFailurePolicy or if it started terminating prematurely (has deletionTimestamp). So it does not always correlate with the pod status.
• jobs are also using different combinations: TerminatingOrFailed, which is not applicable for Deployments

Since deployments do not care about the final phase of the pod, it might be simpler to call this Terminated. And to have less confusion when comparing this API to the Jobs API.

[soltysh]

I will add my two cents, that the distinction between terminating and terminated might be confusing. It should not block the KEP, since this can be decided at the API review phase during implementation and updated here afterwards. Still, I'd probably try to use 2 distinct words, maybe terminated and final?

[atiratree]

I am not sure if we want to introduce a new terminology (final), but rather have some term that can be associated with the pod lifecycle.

I agree, we can revisit this discussion in the implementation phase.

[jpbetz]

Naming idea:

podReplacementPolicy: TerminationStarted
podReplacementPolicy: TerminationComplete

(I agree this does not need to be decided until implementation)

[atiratree]

+1 for TerminationStarted/TerminationComplete. Still, it is a bit different from the Jobs API, but I suppose if we are already incompatible, we can do the naming a bit differently.

[atiratree]

I have updated the KEP to the suggestion above.

[kannon92]

If the pod is removed from etcd, does that mean that we no longer track them?

[atiratree]

We will stop tracking them once they reach the terminal phase. There is a small delay before the pod gets removed from etcd though. Not sure if it is important to mention, but some people might find it weird, that the Pod count is higher in this situation (although not running ones!).

[dejanzele]

As I worked on the promotion to Beta for this feature in Job controller, there were a couple of caveats we encountered along the way when wanting to force the pods into terminating state for some amount of time in order to assert status, so I'll post the PR here kubernetes/kubernetes#121491, as it might save somebody's time

[atiratree]

I expect the deployment tests to be a bit different, but it might come handy. Thanks!

[kannon92]

@atiratree any traction on getting this queued for 1.30 features?

[atiratree]

@kannon92 yes, we went over this one in the planning

[atiratree]

the PR should be ready for PRR
/assign @wojtek-t

[jpbetz]

Did an early pass on API review for this KEP. This is non-blocking, we can merge the KEP and finish API review during implementation, but I figured I get started on feedback.

[jpbetz]

This looks right, but just to confirm, +optional indicates that this will be an omitempty field?

[jpbetz]

Curious about relationships between counters here. I see "non-terminated" used in comments, I assume that is different than what "terminatingReplicas" will count but it would be best if we state it as clearly as possible.

Also, do any of the Replicas, UpdatedReplicas, ... counts include the TerminatingReplicas count? If so we should document clearly. Is there any relationship between the counts (e.g. does ReadyReplicas + TerminatingReplicas = Replicas?)

	// Total number of non-terminated pods targeted by this deployment (their labels match the selector).
	// +optional
	Replicas int32

	// Total number of non-terminated pods targeted by this deployment that have the desired template spec.
	// +optional
	UpdatedReplicas int32

	// Total number of ready pods targeted by this deployment.
	// +optional
	ReadyReplicas int32

	// Total number of available pods (ready for at least minReadySeconds) targeted by this deployment.
	// +optional
	AvailableReplicas int32

	// Total number of unavailable pods targeted by this deployment. This is the total number of
	// pods that are still required for the deployment to have 100% available capacity. They may
	// either be pods that are running but not yet available or pods that still have not been created.
	// +optional
	UnavailableReplicas int32

Which counters (if any) also count terminatingReplicas?

[atiratree]

This looks right, but just to confirm, +optional indicates that this will be an omitempty field?

Yes. I have added the tags for clarity.

Curious about relationships between counters here. I see "non-terminated" used in comments, I assume that is different than what "terminatingReplicas" will count but it would be best if we state it as clearly as possible.

I have also added the other relevant fields to the KEP.

I have replaced non-terminated with non-terminating as I think it describes that better. It should include pods that do not have a .metadata.deletionTimestamp field set.

Terminating pods have a .metadata.deletionTimestamp field set.

I have tried to make the fields clearer. But having non-terminating (no .metadata.deletionTimestamp) everywhere seems too verbose, so I have added the full explanation only to the TerminatingReplicas field

Also, do any of the Replicas, UpdatedReplicas, ... counts include the TerminatingReplicas count?
Which counters (if any) also count terminatingReplicas?

No, they do not. And they cannot - to be backwards compatible.

Is there any relationship between the counts (e.g. does ReadyReplicas + TerminatingReplicas = Replicas?)

There is not. Basically the amount of terminating replicas is unlimited. For example if you have Terminating policy and do multiple rollouts in a row and have a long TerminationGracePeriodSeconds.

[jpbetz]

Naming idea:

podReplacementPolicy: TerminationStarted
podReplacementPolicy: TerminationComplete

(I agree this does not need to be decided until implementation)

[jpbetz]

As the number of replica status counters increases, I'd like to make sure the relationships between the counts are clear. We have:

	// Replicas is the number of actual replicas.
	Replicas int32

	// The number of pods that have labels matching the labels of the pod template of the replicaset.
	// +optional
	FullyLabeledReplicas int32

	// The number of ready replicas for this replica set.
	// +optional
	ReadyReplicas int32

	// The number of available replicas (ready for at least minReadySeconds) for this replica set.
	// +optional
	AvailableReplicas int32

Does FullyLabeledReplicas , Replicas or ReadyReplicas count terminating replicas?

Are we guaranteed ReadyReplicas + TerminatingReplicas = Replicas ? Any other relationships between counters we should clarify?

[atiratree]

Same answer as #4357 (comment).

Strange thing, not sure why are the comments a bit different in the /api/ and /apis/ types?

Also the style of each field is a bit different.

Can we go over the fields and make the documentation consistent when we do the PR in k/k?

[jpbetz]

Can we go over the fields and make the documentation consistent when we do the PR in k/k?

That would be great.

[atiratree]

The policy naming has been updated as follows:

Terminating -> TerminationStarted
Terminated -> TerminationComplete

[soltysh]

/lgtm

[atiratree]

@wojtek-t can you please take a look?

[wojtek-t]

Small comments from PRR pov.

[wojtek-t]

But you can have skew between kube-apiserver and kube-controller-manager. Everything should work fine, but can you please briefly describe what happens if FG is enabled in one of those and not the other?

[atiratree]

added an explanation

[wojtek-t]

So if the FG is disabled, the ReplicaSet/Deployment controllers will actually set the TerminatingReplicas to 0? Or rather will not touch it at all?

[In particular the controller will certainly not touch it if rolled back to a version that doesn't support the field]

[atiratree]

the status will be reconciled in normal deployment/replicaset flow and the value will not be preserved - it will be omitted (0)

[wojtek-t]

If FG in kube-controller-manager is enabled, this isn't true right?
The controller will continue to use the new behavior...

[atiratree]

I was thinking about an apiserver that does not support the field. But yes it will function if it supports the field.

[wojtek-t]

/lgtm
/approve PRR

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh, wojtek-t

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

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-apps/OWNERS [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment