Fix PodAntiAffinity issues in case of multiple affinityTerms

[Huang-Wei]

• rename podMatchesAffinityTermProperties to podMatchesAllAffinityTermProperties
• add podMatchesAnyAffinityTermProperties which should be used in some PodAntiAffinity cases
• add unit tests

What this PR does / why we need it:

There are 2 phases to verify symmetry of PodAntiAffinity:

• verify labels of incoming pod not violates antiaffinity of existing pods
• verify antiaffinity of incoming pod not violates labels of existing pods

This PR is to fix issue in second step.

Currently logic of second step tries to invoke podMatchesAffinityTermProperties when checking both affinity and antiaffinity.

It's not that accurate, as podMatchesAffinityTermProperties is a "match all" semantics, say for each term, ensure it matches target selector/labels, and return true if all term passes check. This is good for affinity, but not antiaffinity.

Regarding antiaffinity, it's a "match any" semantics - i.e. if found any term match target selector/labels, return true.

So without this fix, unit tests (which is added in this PR) failed on following cases:

• a) incoming pod's anti-affinity has more than one term - existing pod violates all terms
• b) incoming pod's anti-affinity has more than one term - existing pod violates partial term - case 1
• c) incoming pod's anti-affinity has more than one term - existing pod violates partial term - case 2

Case a) is guarded in before-mentioned step 1 of symmetric logic. So we don't see it in a running k8s cluster. Case b) and c) are real problems and specifically case c) is what issue #68101 reported.

Which issue(s) this PR fixes:
Fixes #68101

Special notes for your reviewer:
After reviewing the correctness of this PR, bsalamat and k82cn can decide if it needs to go into 1.12 release cycle.

Release note:

Fix PodAntiAffinity issues in case of multiple affinityTerms.

[Huang-Wei]

/sig scheduling

[Huang-Wei]

/cc @bsalamat @k82cn @ravisantoshgudimetla

[Huang-Wei]

/retest

[resouer]

You used nil above instead of make(), should be consistent :D

btw, I think nil is fine.

[Huang-Wei]

Good catch, lol...

getPodsMatchingAffinity simply return nil if it doesn't have affinity/antiaffinity:

kubernetes/pkg/scheduler/algorithm/predicates/metadata.go

Lines 376 to 382 in 058b26f

	func getPodsMatchingAffinity(pod *v1.Pod, nodeInfoMap map[string]*schedulercache.NodeInfo) (affinityPods map[string][]*v1.Pod, antiAffinityPods map[string][]*v1.Pod, err error) {
	allNodeNames := make([]string, 0, len(nodeInfoMap))
	affinity := pod.Spec.Affinity
	if affinity == nil || (affinity.PodAffinity == nil && affinity.PodAntiAffinity == nil) {
	return nil, nil, nil
	}

But Mr. reflect.DeepEqual is unhappy to treat "nil" as equality of a "nil map" - so in some cases, I put nil, while some I put nil map there to make it happy...

BTW: apiequality.Semantic.DeepEqual is considerate to say yes to "nil equals to nil map". But maybe not worth introducing an extra package dependency.

[resouer]

Ah I see, seems like using apiequality.Semantic.DeepEqual is a approach. I noticed that function in other test files.

[Huang-Wei]

Hmm... Have to rebase code as PR #67788 refactored the data structure.

Now it's all consistent. PTAL.

[resouer]

This is a very detailed test, thanks!

Maybe I am verbose, but I am thinking if you can add a test case to verify this bug is fixed in predicates.go as well?

Because we still have many logics there after we got the antiAffinityPods from metadata, so it would be great if we can ensure this change does work and will not break anything in predicates.go.

Sounds reasonable?

[Huang-Wei]

@resouer Thanks for the detailed review!

Sure, it's totally reasonable. Do you think is it good enough to add additional UTs for predicates.go#satisfiesPodsAffinityAntiAffinity()? (step 2 of symmetry check this PR tries to fix)

[resouer]

Yeah, should be.

Or, I think you can just add one more test case to verify this in TestInterPodAffinity? which seems quicker?

[Huang-Wei]

Added one testcase - which only passed with this PR. PTAL.

[resouer]

The idea is valid to me, while some nits need to be fixed first :-)

Also, I would suggest a test case for predicates.go to verify this fix solves the reported issue.

[k82cn]

did we check InterPodAffinityMatches ?

[Huang-Wei]

yes, implicitly - InterPodAffinityMatches calls satisfiesPodsAffinityAntiAffinity, satisfiesPodsAffinityAntiAffinity needs "meta", and "meta" is generated in GetMetadata where calls getPodsMatchingAffinity.

[Huang-Wei]

@resouer @k82cn Code is rebased and added testcase in TestInterPodAffinity().

Please take another look. Thanks!

[rphillips]

/xref kubernetes-retired/bootkube#1001

The changes in this PR are causing affinity sections to appear in checkpoint files causing the Kubelet to not restore pods. The affinity block within the checkpoint manifest errors with the following on kubelet restart:

predicate.go:133] Predicate failed on Pod: pod-checkpointer-l9cgg-172.17.4.101_kube-system(231cd4bc9c8f63b3131c1ec25716fe91), for reason: Predicate MatchNodeSelector failed

Checkpoint manifest json includes:

"affinity": {
      "nodeAffinity": {
        "requiredDuringSchedulingIgnoredDuringExecution": {
          "nodeSelectorTerms": [
            {
              "matchExpressions": null
            }
          ]
        }
      }
    },

Removing the affinity section within the checkpoint manifest fixes the restore issue. I'll be investigating the issue further, but am commenting to see if the upstream authors have any thoughts. This issue is a breaking change on upgrades and new installs for 1.12.x.

[rphillips]

I suspect setting the affinity to nil when matchExpressions == null/nil would preserve the prior behavior.

[Huang-Wei]

@rphillips thanks for bringing this up.

If I understand correctly, you're suspicious that after applying this PR, scheduler rejects a pod which has null matchExpression in nodeAffinity?

[Huang-Wei]

Considering this PR doesn't go to v1.11.3, I did a test on v1.11.3:

kind: Pod
apiVersion: v1
metadata:
  name: pause
spec:
  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions: null
  containers:
  - name: pause
    image: k8s.gcr.io/pause:3.1

The above pod is also rejected. In other words, this PR seems not to break behavior of null matchExpressions.

$ k get po
NAME      READY     STATUS    RESTARTS   AGE
pause     0/1       Pending   0          2m
$ k describe po pause | tail -3
  Type     Reason            Age               From               Message
  ----     ------            ----              ----               -------
  Warning  FailedScheduling  1m (x25 over 3m)  default-scheduler  0/3 nodes are available: 3 node(s) didn't match node selector.

[rphillips]

@Huang-Wei Thank you for the prompt reply. The concern is with a static pod the kubelet will restore on restart. Bootkube, Tectonic, and other distributions may use a checkpointer to save the state in a manifest.

core@c1 /etc/kubernetes/manifests $ jq < kube-system-pod-checkpointer-q8zww.json 
{
  "kind": "Pod",
  "apiVersion": "v1",
  "metadata": {
    "name": "pod-checkpointer-q8zww",
    "namespace": "kube-system",
    "creationTimestamp": null,
    "labels": {
      "controller-revision-hash": "6775765d66",
      "k8s-app": "pod-checkpointer",
      "pod-template-generation": "1",
      "tier": "control-plane"
    },
    "annotations": {
      "checkpointer.alpha.coreos.com/checkpoint-of": "pod-checkpointer-q8zww"
    },
    "ownerReferences": [
      {
        "apiVersion": "v1",
        "kind": "Pod",
        "name": "pod-checkpointer-q8zww",
        "uid": "55c42d8e-cd79-11e8-9476-080027385a9b",
        "controller": true
      }
    ]
  },
  "spec": {
    "volumes": [
      {
        "name": "kubeconfig",
        "hostPath": {
          "path": "/etc/kubernetes/checkpoint-configmaps/kube-system/pod-checkpointer-q8zww/kubeconfig-in-cluster"
        }
      },
      {
        "name": "etc-kubernetes",
        "hostPath": {
          "path": "/etc/kubernetes",
          "type": ""
        }
      },
      {
        "name": "var-run",
        "hostPath": {
          "path": "/var/run",
          "type": ""
        }
      },
      {
        "name": "pod-checkpointer-token-ljvdc",
        "hostPath": {
          "path": "/etc/kubernetes/checkpoint-secrets/kube-system/pod-checkpointer-q8zww/pod-checkpointer-token-ljvdc"
        }
      }
    ],
    "containers": [
      {
        "name": "pod-checkpointer",
        "image": "quay.io/coreos/pod-checkpointer:9dc83e1ab3bc36ca25c9f7c18ddef1b91d4a0558",
        "command": [
          "/checkpoint",
          "--lock-file=/var/run/lock/pod-checkpointer.lock",
          "--kubeconfig=/etc/checkpointer/kubeconfig",
          "--checkpoint-grace-period=5m"
        ],
        "env": [
          {
            "name": "NODE_NAME",
            "valueFrom": {
              "fieldRef": {
                "apiVersion": "v1",
                "fieldPath": "spec.nodeName"
              }
            }
          },
          {
            "name": "POD_NAME",
            "valueFrom": {
              "fieldRef": {
                "apiVersion": "v1",
                "fieldPath": "metadata.name"
              }
            }
          },
          {
            "name": "POD_NAMESPACE",
            "valueFrom": {
              "fieldRef": {
                "apiVersion": "v1",
                "fieldPath": "metadata.namespace"
              }
            }
          }
        ],
        "resources": {},
        "volumeMounts": [
          {
            "name": "kubeconfig",
            "mountPath": "/etc/checkpointer"
          },
          {
            "name": "etc-kubernetes",
            "mountPath": "/etc/kubernetes"
          },
          {
            "name": "var-run",
            "mountPath": "/var/run"
          },
          {
            "name": "pod-checkpointer-token-ljvdc",
            "readOnly": true,
            "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount"
          }
        ],
        "terminationMessagePath": "/dev/termination-log",
        "terminationMessagePolicy": "File",
        "imagePullPolicy": "Always"
      }
    ],
    "restartPolicy": "Always",
    "terminationGracePeriodSeconds": 30,
    "dnsPolicy": "ClusterFirst",
    "nodeSelector": {
      "node-role.kubernetes.io/master": ""
    },
    "nodeName": "172.17.4.101",
    "hostNetwork": true,
    "securityContext": {},
    "affinity": {
      "nodeAffinity": {
        "requiredDuringSchedulingIgnoredDuringExecution": {
          "nodeSelectorTerms": [
            {
              "matchExpressions": null
            }
          ]
        }
      }
    },
    "schedulerName": "default-scheduler",
    "tolerations": [
      {
        "key": "node-role.kubernetes.io/master",
        "operator": "Exists",
        "effect": "NoSchedule"
      },
      {
        "key": "node.kubernetes.io/not-ready",
        "operator": "Exists",
        "effect": "NoExecute"
      },
      {
        "key": "node.kubernetes.io/unreachable",
        "operator": "Exists",
        "effect": "NoExecute"
      },
      {
        "key": "node.kubernetes.io/disk-pressure",
        "operator": "Exists",
        "effect": "NoSchedule"
      },
      {
        "key": "node.kubernetes.io/memory-pressure",
        "operator": "Exists",
        "effect": "NoSchedule"
      },
      {
        "key": "node.kubernetes.io/unschedulable",
        "operator": "Exists",
        "effect": "NoSchedule"
      },
      {
        "key": "node.kubernetes.io/network-unavailable",
        "operator": "Exists",
        "effect": "NoSchedule"
      }
    ],
    "priority": 0
  },
  "status": {}
}

[rphillips]

Note: We do not create the checkpointer pod with an affinity, so I'm not sure where it is being added in.

[Huang-Wei]

We do not create the checkpointer pod with an affinity, so I'm not sure where it is being added in.

Agree, this is the key point.