Drop feature gate in descheduler support design proposal

[fossedihelm]

The descheduler support design proposal specifies that a feature gate will be added. The feature gate can be dropped, since no work is added other than to add annotations. The feature gate drop also has the advantage of saving work to the cluster admins; with the feature gate, if a cluster admin wants to enable the descheduler, they have to install it and add the feature gate to KubeVirt. Without the feature gate they only have to install it.

Also, this feature gate only enables a support for the descheduler, which should always be enabled, and there are no advantages to not have it.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR. Approvers are expected to review this list.

• [ ] Design: A design document was considered and is present (link) or not required
• [ ] PR: The PR description is expressive enough and will help future contributors
• [ ] Code: Write code that humans can understand and Keep it simple
• [ ] Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• [ ] Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• [ ] Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• [ ] Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• [ ] Community: Announcement to kubevirt-dev was considered

Release note:

NONE

/cc @acardace @Barakmor1 @iholder101 @jean-edouard @xpivarc

[iholder101]

From my perspective, the most important thing here is that the descheduler is not a Kubevirt feature, hence it shouldn't be guarded with a feature gate.

If we want to provide a way of avoiding these extra annotations, we can always provide a configuration for doing so, but using a feature gate for it is wrong IMO.

However, I currently don't see a reason to provide such a configuration as a few possibly redundant annotations shouldn't cause any harm that I can see.

/lgtm

[acardace]

/lgtm

+1 Totally agree with this.

[fossedihelm]

/cc @fabiand @aburdenthehand

[Barakmor1]

/lgtm

[fabiand]

Agree with @iholder101 - I tend to say that we want a cluster level way to opt out of setting the annotations. Main reason: Some people might not want these annotations.

[fabiand]

Actually @fossedihelm when are we adding the annotations? unconditionally or under certain conditions?

[iholder101]

Agree with @iholder101 - I tend to say that we want a cluster level way to opt out of setting the annotations. Main reason: Some people might not want these annotations.

Thanks @fabiand!

Just to clarify: If we want this to be configurable, then a cluster-wide configuration is the way to go rather than a feature gate. Saying that, I'm not sure we need to add such a configuration.

We're talking about pod-level annotations, which are considered as an "implementation detail" of our VM/VMI objects.

Do you think there's a reason for users to opt-out of these annotations?

[fossedihelm]

Actually @fossedihelm when are we adding the annotations? unconditionally or under certain conditions?

It depends on the annotation:

• descheduler.alpha.kubernetes.io/request-evict-only will be added for every virt-launcher pod
• descheduler.alpha.kubernetes.io/eviction-in-progress to the source virt-launcher pod only when an evacuation migration is triggered (not every migration).

IOW, from a practical point of view, we are going to add only the first one because the second one will be added to the pod that (eventually) will be Completed.

To me, a single annotation to the virt-launcher pod (not VM nor vmi) should not disturb users.

[fabiand]

Ok, thanks. Reasonable.

So, is descheduler.alpha.kubernetes.io/evict https://github.com/kubernetes-sigs/descheduler/blob/17af29afe418d56aff1954ee4ee0392e29b3fd5e/README.md?plain=1#L947 still required today as well?

[fossedihelm]

Ok, thanks. Reasonable.

So, is descheduler.alpha.kubernetes.io/evict https://github.com/kubernetes-sigs/descheduler/blob/17af29afe418d56aff1954ee4ee0392e29b3fd5e/README.md?plain=1#L947 still required today as well?

@fabiand I don't think I fully understand. We are not adding descheduler.alpha.kubernetes.io/evict and not managing it at all. Am I missing something? Thank you

[fabiand]

/approve

according to all previous positive reviews.

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabiand

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](https://github.com/kubevirt/community/blob/main/OWNERS)~~ [fabiand] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment