Hmmm... I thought it was ok for "PreferredDuringScheduling" to have empty topology key. Empty topology key means "all topologies" for "PreferredDuringScheduling". TopologyKey is not allowed to be empty for
RequiredDuringScheduling.

if you have existing pods like this, tightening validation can prevent them from being able to be deleted

We need to do some more research on this. I'll try to get @wojtek-t to chime in. I can't remember the detailed history of this and unfortunately don't have time right now to dig through it.

The API comment supports what @bsalamat said, and moreover it is also what I recall as being the desired behavior. Also there is a comment in the code that supports it.

But then the actual implementation doesn't seem to do the right thing for empty TopologyKey.

IIRC, what we wanted to do is to:

• make it still possible to have empty topology key for alpha version (represented in annotations)
• disable it in Beta (represented in object fields).
  It makes impossible to migrate your alpha pods to Beta ones, but I think that was a conscious decision.

Empty topology key (or to be more specific, allowing multiple keys, which it is behond the scenes) is a disaster from performance point of view and we should get rid of it.

Thanks, @wojtek-t ! In that case it sounds like this PR is correct, an also we should update the two comments I mentioned in my last reply, that talk about empty TopologyKey.

@davidopp updated PTAL.

@liggitt This behavior is already in 1.7 (https://github.com/kubernetes/kubernetes/blob/release-1.7/pkg/api/validation/validation.go#L2432). This PR fixes 1.8 to avoid regression.

@liggitt This behavior is already in 1.7 (https://github.com/kubernetes/kubernetes/blob/release-1.7/pkg/api/validation/validation.go#L2432). This PR fixes 1.8 to avoid regression.

looks like if they enabled the AffinityInAnnotations feature gate in 1.7, they could have existing pods with empty TopologyKey fields, right? just need to determine whether it was possible for someone to get data like that persisted in 1.7, and make sure adding this validation doesn't make that bad data from 1.7 impossible to remove or fix.

looks like if they enabled the AffinityInAnnotations feature gate in 1.7, they could have existing pods with empty TopologyKey fields, right? just need to determine whether it was possible for someone to get data like that persisted in 1.7, and make sure adding this validation doesn't make that bad data from 1.7 impossible to remove or fix.

If someone is enabling an Alpha feature (AffinityInAnnotation) in 1.7 and the feature is already deprecated, they should know that they are supposed to update their stuff in 1.7 before moving to 1.8.

I think I am more worried about when AffinityInAnnotation is disabled (which is by default) in 1.7 and then there is behavior change between 1.7 and 1.8 pod affinity beta fields, which this PR fixes.

If someone is enabling an Alpha feature (AffinityInAnnotation) in 1.7 and the feature is already deprecated, they should know that they are supposed to update their stuff in 1.7 before moving to 1.8.

Can you add a release note indicating this is no longer allowed, and that clusters with the alpha AffinityInAnnotation feature enabled must ensure pods with empty TopologyKey fields must be removed before upgrading to 1.8?

@liggitt updated release note, PTAL.

thanks

@bsalamat @davidopp @lavalamp could you give lgtm /approval as i dont think anything is left to do in this pr as its been lying for long and it really fixes an important issue?

/lgtm

but I cannot approve

/approve

/approve no-issue

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, bsalamat, davidopp, wojtek-t

Associated issue requirement bypassed by: wojtek-t

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

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Automatic merge from submit-queue (batch tested with PRs 50531, 50853, 49976, 50939, 50607)

Sorry, I didn't see this.

Next time please make sure it gets the action required label.

This doesn't look like a safe way to accomplish the goal. It's fine to have action required but there should be a path to fixing things if the action isn't followed correctly. How does one recover in this case?