KEP-3243: Update the design to mutate the label selector based on matchLabelKeys at api-server instead of the scheduler handling it

[mochizuki875]

• One-line PR description: As discussed #129480, I've updated this KEP related to
  labelSelector.
• Issue link: Respect PodTopologySpread after rolling upgrades #3243
• Other comments:

[mochizuki875]

/cc @sanposhiho

[sanposhiho]

/cc @alculquicondor
Do we need /lead-opt-in at #3243?

@wojtek-t (or @alculquicondor) do we need PRR review in this case? (I suppose Yes?)

[sanposhiho]

/retitle KEP-3243: Update the design to mutate the label selector based on matchLabelKeys at api-server instead of the scheduler handling it

[alculquicondor]

cc @dom4ha

[sanposhiho]

Update the content based on the conclusion kubernetes/kubernetes#129480 (comment)

[sanposhiho]

You need to update other sections such as Test Plan and other PRR questions.

[sanposhiho]

Also, please add the current implementation for Alternative section and describe why we decided to move to a new approach.

[mochizuki875]

I appreciate your comment.
I've addressed.

[sanposhiho]

/assign @wojtek-t

for a PRR part reviewing. Please assign another person if needed.

[wojtek-t]

for a PRR part reviewing. Please assign another person if needed.

Queued - although I will wait for SIG approval to happen first.

[mochizuki875]

@sanposhiho
Thank you for your comments.

I have a question.
I understand that kube-scheduler has two roles:

1. Read matchLabelKeys of cluster-level default constraints from scheduler configuration, get key-value labels corresponding to matchLabelKeys from pod labels, and logically combine them with labelSelector internally to schedule pod.
2. Check if the key-value labels corresponding to matchLabelKeys are set in labelSelector by kube-apiserver or users, and if not, logically combine them with labelSelector internally to schedule pod.

Are my understandings correct?
I'm concerned about 2.

For example, if the following Pod manifest is applied

apiVersion: v1
kind: Pod
metadata:
  name: sample
  labels:
    app: sample
...
  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector: {}
    matchLabelKeys:
    - app

kube-apiserver will usually merge key-value labels corresponding to matchLabelKeys into labelSelector, so kube-scheduler will not handle matchLabelKeys.

  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector:
      matchExpressions:
+      - key: app
+        operator: In
+        values:
+        - sample
    matchLabelKeys:
    - app

It is the same if user sets key-value labels corresponding to matchLabelKeys in labelSelector

apiVersion: v1
kind: Pod
metadata:
  name: sample
  labels:
    app: sample
...
  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector:
      matchExpressions:
      - key: app
        operator: In
        values:
        - sample
    matchLabelKeys:
    - app

However, if key-value labels corresponding to matchLabelKeys are not merged into labelSelector by kube-apiserver for some reason, kube-scheduler will get key-value labels from pod labels and logically combine them with labelSelector internally to schedule pod.(I'm wondering if this case should be considered.)

  topologySpreadConstraints:
  - maxSkew: 1
    topologyKey: kubernetes.io/hostname
    whenUnsatisfiable: DoNotSchedule
    labelSelector: 
      matchExpressions:
      - key: foo
        operator: In
        values:
        - bar
    matchLabelKeys:
    - app

[sanposhiho]

As my suggested changes above imply, the scheduler only has one role, which is to handle matchLabelKeys within the cluster wide default constraints. In other cases (like a user created a Pod with matchLabelKeys), we assume kube-apiserver handles it and kube-scheduler doesn't have to worry about matchLabelKeys, right?

... and you know what, I found I forgot about the update path issue that I raised myself while I'm writing this comment. 😅
I'll revisit my suggestions made above.

So, answering your questions,

However, if key-value labels corresponding to matchLabelKeys are not merged into labelSelector by kube-apiserver for some reason, kube-scheduler will get key-value labels from pod labels and logically combine them with labelSelector internally to schedule pod.(I'm wondering if this case should be considered.)

Usually, no. Like my above strikethrough-ed comment argued, we can just assume kube-apiserver handles all matchLabelKeys that users added. And, the scheduler has to only worry about the cluster wide default constraints.
HOWEVER, there's an exception which is during the cluster upgrade. We need to consider scenarios kube-apiserver not handling matchLabelKeys because, at the time running the cluster upgrade, there could be unscheduled pods that are made through the old kube-apiserver, and have to be handled by the new version kube-scheduler. In this case, kube-scheduler needs to handle matchLabelKeys even though those matchLabelKeys came from users, not from the cluster wide default constraints.
That being said, after one release with this new design, so basically at the next release cycle, we can change kube-scheduler to only handle matchLabelKeys from the cluster wide default constraints.

[mochizuki875]

@sanposhiho
I've addressed them and add description related to kubernetes/kubernetes#129480 (comment)

[mochizuki875]

@sanposhiho
Thank you, I've incorporated your suggestion.

[sanposhiho]

/lgtm

/cc @dom4ha @macsko
/assign @alculquicondor

[macsko]

Is it true? Even if the labelSelector no longer matches the pod itself, the topology spreading is not ignored and could still make the pod unschedulable (ofc selfMatchNum will be 0). If it's what you mean, I'd rephrase it.

[dom4ha]

Wouldn't simply the label change become ignored? Does the pod itself need to be covered by the selector? I think the bigger problem is that since we'd have both implementations (in api-server and scheduler), won't scheduler add new label ot the labelSelecor and make the pod completely unschedulable?

[sanposhiho]

Is it true? Even if the labelSelector no longer matches the pod itself, the topology spreading is not ignored and could still make the pod unschedulable (ofc selfMatchNum will be 0). If it's what you mean, I'd rephrase it.

Yeah, sorry it's my suggestion and you're right, "ignored" was misleading and we should rephrase; matchNum - minMatchNum could still be bigger than maxSkew and node(s) in some domains could be unschedulable, depending on the current number of matching pods there.

won't scheduler add new label ot the labelSelecor and make the pod completely unschedulable?

The scheduler adds, but the pod won't be completely unschedulable. Rather, the label addition in this case makes the pod easier to get scheduled because selfMatchNum would be 0.

[mochizuki875]

Thank you for your comments.
I've tried to rephrase this sentence.

[dom4ha]

Suggested change

	It means this feature doesn't support the label's update even though users, of course,
	It means this feature doesn't support the label's update even though a user
	could update the label that is specified at `matchLabelKeys` after a pod's creation.

[dom4ha]

Suggested change

	`TopologySpreadConstraint` which represent a set of label keys only.
	`TopologySpreadConstraint` which represents a set of label keys only.

[dom4ha]

I'd rather say that updating labels used is not recommended at all (instead of updating frequently), as it would break the original placement of such pods.

[dom4ha]

Wouldn't simply the label change become ignored? Does the pod itself need to be covered by the selector? I think the bigger problem is that since we'd have both implementations (in api-server and scheduler), won't scheduler add new label ot the labelSelecor and make the pod completely unschedulable?

[dom4ha]

Isn't it only a proposal? kubernetes/kubernetes#129198 (comment)

[dom4ha]

Suggested change

	So we decided to go with the current design that implements this feature within both
	So we decided to go with the design that implements this feature within both

[dom4ha]

Suggested change

	impossible to handle `MatchLabelKeys` within the cluster-level default constraints in the scheduler configuration.
	impossible to handle `MatchLabelKeys` within the cluster-level default constraints in the scheduler configuration in the future (see https://github.com/kubernetes/kubernetes/issues/129198).

[dom4ha]

Suggested change

	and those key-value labels are ANDed with `LabelSelector` to identify the group of existing pods over
	and those key-value labels will be merged with existing `LabelSelector` to identify the group of existing pods over

[dom4ha]

Suggested change

	kube-apiserver will use those keys to look up label values from the incoming pod
	At a pod creation, kube-apiserver will use those keys to look up label values from the incoming pod

[dom4ha]

Suggested change

	kube-scheduler will also handle it if the cluster-level default constraints have the one with `MatchLabelKeys`.
	Note that in case `MatchLabelKeys`is supported in the cluster-level default constraints (see https://github.com/kubernetes/kubernetes/issues/129198), kube-scheduler will also handle it separately.

[mochizuki875]

@dom4ha @macsko
Thank you for your comments.
I've addressed and please check them.

[macsko]

lgtm from me. Let's wait for @dom4ha

[alculquicondor]

/approve

Time is running out, so better approve now.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mochizuki875, sanposhiho

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/sig-scheduling/OWNERS [alculquicondor]

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

[alculquicondor]

/lgtm

[sanposhiho]

@wojtek-t We haven't modified PRR file and it's merged without your approval.
Please take a look and we can follow up accordingly if you have any suggestion.

[dom4ha]

No further concerns from my side. I have limited availability this week, so thanks @macsko and @alculquicondor for approvals