gardener / kupid

Inject scheduling criteria into target pods orthogonally by policy definition.
Apache License 2.0
11 stars 19 forks source link

Fix merge logic for node affinities #59

Closed shreyas-s-rao closed 1 year ago

shreyas-s-rao commented 1 year ago

What this PR does / why we need it: This PR brings the following changes and improvements:

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

Special notes for your reviewer:

Release note:

Fix merge logic for node affinities, so now node affinities between policy and pod spec are "strategically" merged using cartesian product of the `NodeSelectorTerms`.
Add leader election capability to kupid controller manager so that it can be run in active-passive HA mode.
Use structured JSON logger, with ISO8601 time encoder.
Make development and deployment of kupid easier by including deploying/removal of [cert-manager](https://cert-manager.io/docs/) within `make [un]deploy-with-certmanager` Make target.
shreyas-s-rao commented 1 year ago

/assign @unmarshall @ashwani2k PTAL

timuthy commented 1 year ago

/assign

shreyas-s-rao commented 1 year ago

Note to reviewers to test the PR:

  1. Checkout the PR branch locally
  2. Target any Kubernetes cluster with k8s version >= v1.22
  3. Run make deploy-with-certmanager to deploy cert-manager as well as the kupid extension
  4. Watch the kupid pod logs to ensure that kupid reacts to changes to PSPs, CPSPs as well as other pod-group resources deployed in the cluster
  5. Deploy a ClusterPodSchedulingPolicy for mutating nodeAffinity for statefulset pods (sample resource provided here)
  6. Deploy a statefulset with nodeAffinity different from the policy, and ensure from kupid logs that it mutates the statefulset
  7. Ensure that the statefulset is mutated correctly

You may use the following examples to test the PR: NodeAffinity present on the pod spec:

nodeAffinity:
  requiredDuringSchedulingIgnoredDuringExecution:
    nodeSelectorTerms:
    - matchExpressions:
      - key: a
        operator: In
        values:
        - A
      - key: b
        operator: In
        values:
        - B
    - matchExpressions:
      - key: c
        operator: In
        values:
        - C

kupid-sts-test.txt (github doesn't allow me to upload YAML files, hence renamed to .txt)

NodeAffinity defined by the ClusterPodSchedulingPolicy:

nodeAffinity:
  requiredDuringSchedulingIgnoredDuringExecution:
    nodeSelectorTerms:
    - matchExpressions:
      - key: d
        operator: In
        values:
        - D
    - matchExpressions:
      - key: e
        operator: In
        values:
        - E
      matchFields:
      - key: metadata.name
        operator: In
        values:
        - F

kupid-policy-test.txt

Expected correctly merged nodeAffinity with this PR (note that the affinity terms are merged using cartesian product, which means that the policy specified by Kupid is truly enforced on the pod spec, so a true strategic merge patch happens here):

nodeAffinity:
  requiredDuringSchedulingIgnoredDuringExecution:
    nodeSelectorTerms:
    - matchExpressions:
      - key: d
        operator: In
        values:
        - D
      - key: a
        operator: In
        values:
        - A
      - key: b
        operator: In
        values:
        - B
    - matchExpressions:
      - key: e
        operator: In
        values:
        - E
      - key: a
        operator: In
        values:
        - A
      - key: b
        operator: In
        values:
        - B
      matchFields:
      - key: metadata.name
        operator: In
        values:
        - F
    - matchExpressions:
      - key: d
        operator: In
        values:
        - D
      - key: c
        operator: In
        values:
        - C
    - matchExpressions:
      - key: e
        operator: In
        values:
        - E
      - key: c
        operator: In
        values:
        - C
      matchFields:
      - key: metadata.name
        operator: In
        values:
        - F

Wrongly merged nodeAffinity, prior to this PR (observe here that the affinity rules are simply appended to each other, causing the scheduler to perform an OR amongst these NodeSelectorTerms and choose any one for scheduling the pod, which is incorrect):

nodeAffinity:
  requiredDuringSchedulingIgnoredDuringExecution:
    nodeSelectorTerms:
    - matchExpressions:
      - key: a
        operator: In
        values:
        - A
      - key: b
        operator: In
        values:
        - B
    - matchExpressions:
      - key: c
        operator: In
        values:
        - C
    - matchExpressions:
      - key: d
        operator: In
        values:
        - D
    - matchExpressions:
      - key: e
        operator: In
        values:
        - E
      matchFields:
      - key: metadata.name
        operator: In
        values:
        - F

Please also change the replicas count to 2 or 3 to test the HA mode of Kupid introduced in this PR with leader election capabilities.

In order to test the interplay of kupid webhook with other mutating webhooks such as GRM from gardener, please run kupid as an extension in the gardener local setup and deploy a HA seed and then a HA shoot and observe the effect on the etcd-main statefulset deployed as part of the shoot control plane.

timuthy commented 1 year ago

/unassign