Not related to matchLabelSelectors, but we decided not to have e2e test in kubernetes/kubernetes#116065.

Can this use case be generalized? I'd like this case to be explained as JobSet-like, but not restricted to JobSet only.

For example: when M Pods and N Pods share the same PodTemplate. The orchestrator spins up the two set of Pods at runtime, distinguish at their common key with different value: one can be foo=abc, the other is foo=xyz. If we want to place M pods in a topology, while keep N pods away from that topology, how can we achieve that?

OK, I'll try to have another example here to explain the same usecase.

As you suggested, I changed this example to be more general, not using jobset. PTAL.

I didn't follow your example because I feel like users can define LabelSelector of foo=abc / foo=xyz directly in Pod affinity / anti affinity. I think the point of the usecase of matchLabelKey is that "users want to use it in PodAffinity, but they cannot know the label value because, for example, the label value is applied from another system automatically" (like jobset controller for job-name, the deployment controller for pod-template-hash, generated in the manifest manager, etc)

I'm not sure I like this design. Look at this yaml - it specifies labelSelector and matchLabelSelector in a podAntiAffinity term, so can a user really understand its intents from first glance?

As an alternative, can we implement the semantics in the operator of podAffinityTerm? so we don't need matchLabels or matchLabelSelectors - all semantics are described using podAffinityTerm.

• (new) ExistsWithSameValue - means "key" Exists and the match value is identical
• (new) ExistsWithDifferentValue- means "key" Exists and the matched value is different

Could you provide how your proposal looks via example yaml or something?

I would love to see another proposal for the API, I agree that it duplicates labelSelector, but I didn't have better suggestions other than perhaps changing LabelSelector API itself to allow for implicitly selecting the label from the object, which is not applicable in all cases and can be confusing in the cases where it isn't.

@Huang-Wei I am also not following, which operator are you referring to? a new one we add to podAffinityTerm? an example would be great indeed.

Here are some examples:

a set of Pods A doesn't want to co-exist with other set of Pods, but want the set of Pods A co-located

    spec:
      affinity:
        podAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: pod-set
                operator: ExistsWithSameValue
            topologyKey: kubernetes.io/hostname
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: pod-set
                operator: ExistsWithDifferentValue
            topologyKey: kubernetes.io/hostname

smooth rolling upgrade for PodAntiAffinity:

    spec:
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
          - labelSelector:
              matchExpressions:
              - key: app
                operator: In
                values:
                - pause
            topologyKey: kubernetes.io/hostname
          - labelSelector:
              matchExpressions:
              - key: pod-template-hash
                operator: ExistsWithSameValue
            topologyKey: kubernetes.io/hostname

@Huang-Wei I am also not following, which operator are you referring to?

(The term "operator" is misleading...) I meant LabelSelectorOperator. And see ^^ for examples.

Generally adding a new enum to a set is problematic in the API. The readers in this case are the scheduler and any external management tools (descheduler, etc). Does anyone else read this field for enforcement? The kubelet?

Does anyone else read this field for enforcement? The kubelet?

no one else in k/k will read it. (As you said, some external tools like descheduler or cluster autoscaler will)

but also, this is generally consumed via the labels pkg: https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/apimachinery/pkg/labels; so if the new enums are handled there, most readers will just get them.

Can you list the enum alternative in the "Alternatives" section of the KEP?

Sure, added this idea as an alternative for now.
Who can give the final decision on whether the enum addition is allowed or not? (API reviewer?)

About the previous discussion of adding a new enum to label selector, I don't think we should do that, as existing clients are not prepared to handle that. Additionally, that new enum would make label selectors contextual, which would mean all code paths taking label selectors and returning matchers would then need to take label selectors AND a set of the labels of the containing object... that's a super invasive change I don't think we would ever successfully ripple out to all label selector consumers.

Having a separate spot in the API with key/operator fields that explicitly means "this label selector but with the value pulled from the pod's label" seems better to me than changing semantics of the existing label selector field.

I do wonder if it would make sense to have the API server actually populate the peer label selector field with the pod's label values on create. For example, creating this pod:

metadata:
  labels:
    tenant: foo
affinity:
  podAffinity:      # ensures the pods of this tenant land on the same node pool
    requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: otherlabel
            operator: In
            values: ["othervalue"]
        matchLabelSelectors:
          - key: tenant
            operator: In

would result in this pod being persisted:

 metadata:
   labels:
     tenant: foo 
 affinity:
   podAffinity:      # ensures the pods of this tenant land on the same node pool
     requiredDuringSchedulingIgnoredDuringExecution:
       - labelSelector:
           matchExpressions:
           - key: otherlabel
             operator: In
             values: ["othervalue"]
+          - key: tenant
+            operator: In
+            values: ["foo"]
         matchLabelSelectors:
           - key: tenant
             operator: In

That would mean anything reading pods and understanding how they are scheduled wouldn't have to learn about this new field, and the podAffinity / podAntiAffinity fields would continue to contain all the scheduling info

+1 on doing this in apiserver.
But then we should do the same in https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/3243-respect-pod-topology-spread-after-rolling-upgrades

(note that populating/merging with user specified fields on write in the server is usually not a great idea, but could be ok here specifically on pods since the field is immutable after create… also need to consider what would happen with an old client writing pods that didn't know about the new field and maybe copy the old value to the new pod in prepareforupdate)

That would simplify the implementation, but wouldn't users be surprised to see that duplicated in the labelSelector when reading back the spec?

would need to game it out for sure, but users would also be surprised to see their affinity/antiAffinity rules get ignored, which is what all existing API clients (alternate schedulers, autoscalers, etc) making scheduling decisions on pods would do (until updated to understand this new field, which might never happen)

also, those clients could be very confused (or maybe even broken) if they encounter apparently incomplete/invalid affinity/antiAffinity stanzas in pods

Sorry, missed this comment.
But, I prefer the idea of keeping matchLabelKeys + adding notMatchLabelKeys to matchLabelSelectors because

the API field is similar with what we introduced in PodTopologySpread.

makes both easier for users to understand instead of making a difference (matchLabelKeys vs matchLabelSelectors) between them to specify a similar thing.

@alculquicondor @ahg-g What do you think?

I'll defer to @ahg-g and @msau42

@ahg-g @msau42 kindly ping. 🙏

If we can find a more intuitive name for notMatchLabelKeys, then I am onboard. Otherwise I would stick with the key+operator approach.

• distinctLabelKeys
• ignoreSameValueLabelKeys
• ...? (My English skill can put out only them 😓

should we validate that only in and notin are supported? are there other relevant operators?

it'd be nice to add concrete examples here.

should we validate that only in and notin are supported?

+1. I don't think we will have supports for other fields. I added the mention that in and notin are valid.

Maybe emphasize a bit that the value of tenant is unknown when composing the yaml.

make the indents consistent? (you used tab below)

Format the comments (to have same empty spaces)

it'd be nice to add concrete examples here.

Can we define the allowed values? Are you planning only In and NotIn or sth else too?

Sure.
Yes, only In and NotIn are expected.

Done in #4069

Actually - it will ignore (drop) setting this field for newly added objects.
But existing objects will continue to exist.

So I'm assuming that kube-scheduler will also be performing the actual computations under feature-gate, right?

This is also touching on
"How can this feature be enabled / disabled in a live cluster?" in PRR.

existing objects will continue to exist.

Ah, yes correct. This PR changes MatchLabelSelectors to be handled in kube-apiserver when Pod is created.
So, existing objects will continue to have the labelselector made by MatchLabelSelectors even after the feature gate is turned off.

What is the ideal action here? OK to leave the existing objects when turning off the feature gate (as long as we state in KEP), or we somehow need to ignore the labelselector made by MatchLabelSelectors (gonna be tricky though..)?