Support pods with a non-empty .pod.nodeName by replacing with node affinity

[iholder101]

What this PR does / why we need it:
Creating pods with both scheduling gates and a non-empty .spec.nodeName is forbidden in Kubernetes. This causes an issue since AAQ's webhook adds scheduling gates for certain pods, which might have a non-empty nodeName set.

With this PR's changes, the pod's .spec.nodeName would be dropped and instead a node affinity targeting the same node would be added.

For example, a pod with the following spec:

spec:
  nodeName: node01

would be changed to:

spec:
  # nodeName: node01 --> this field is now dropped
  affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchFields:
          - key: metadata.name
            operator: In
            values:
            - node01

This trick allows AAQ to support pods with a target node set.

Note to reviewer:
In the first commit, I've imported kubevirt's patch package to ensure the patching code remains elegant.

Release note:

Support pods with a non-empty .pod.nodeName by replacing with node affinity

[Barakmor1]

Thanks a lot for your PR! 😊 I'm really grateful for your help.

I'd prefer not to add kubevirt/kubevirt as a requirement if possible. It could mean doing some extra steps, like asking for changes in kubevirt's code, making a new version of kubevirt, and then updating our project here. Let's try to keep things simple and make sure we can use the new stuff easily. Thanks again for your work!

[iholder101]

Thanks a lot for your PR! 😊 I'm really grateful for your help.

I'd prefer not to add kubevirt/kubevirt as a requirement if possible. It could mean doing some extra steps, like asking for changes in kubevirt's code, making a new version of kubevirt, and then updating our project here. Let's try to keep things simple and make sure we can use the new stuff easily. Thanks again for your work!

Thanks for reviewing :)
Done. PTAL

[Barakmor1]

Thanks for the important improvement :)
I wonder if kubernetes folks could somehow provide support for .spec.nodeName with schedulingGate.
@vladikr WDYT?

/approve

We can track this here

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Barakmor1

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:

• OWNERS [Barakmor1]

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

[alculquicondor]

Why were you setting a nodeName in the first place?

Setting a nodeName complete bypasses the scheduler, which could lead to over commitment of nodes.

There is no point in supporting nodeName and scheduling gates, as the whole point of scheduling gates is to prevent scheduling. Whereas setting a nodeName is the equivalent of scheduling.

[iholder101]

Why were you setting a nodeName in the first place?

Setting a nodeName complete bypasses the scheduler, which could lead to over commitment of nodes.

There is no point in supporting nodeName and scheduling gates, as the whole point of scheduling gates is to prevent scheduling. Whereas setting a nodeName is the equivalent of scheduling.

Hey @alculquicondor! Thanks for your reply!

Firstly, I agree that setting .spec.nodeName is a bad idea. I'm guessing that this field is very old and being kept around for backward compatibility reasons, since using affinity is always a better idea (not even talking about the fact that it gets populated when the pod schedules, as if it's part of the pod's status..).

Secondly, it wasn't me who set the nodeName. This operator is being installed on various environments, some of them are running pods with a nodeName set. While you're right that it's probably a bad idea to set this field, I still want AAQ to be able to run on such environments. I think you'd agree that if such a pod is being created, then being mutated by our hook, then the creation fails, is not a great behavior from our end.

Lastly, I fail to understand why it is restricted to set a nodeName alongside scheduling gates in the first place. This basically means that dynamic quota management (which is the only use-case specified in the scheduling gates KEP) is not possible for pods that set a nodeName. It seems to me that it could have been solved easily by saying "if there's a scheduling gate - the pod is not ready for scheduling - whether it has or doesn't have a nodeNamde set".

WDYT?

[vladikr]

/lgtm

[Barakmor1]

/cherry-pick release-v1.1

[kubevirt-bot]

@Barakmor1: new pull request created: #31

In response to this:

/cherry-pick release-v1.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.