Eviction admitter: Use KubeVirt generated clientset #11897

orelmisan · 2024-05-12T18:38:46Z

What this PR does

Before this PR:
PodEvictionAdmitter:

Had exported (public) fields and could be instantiated incorrectly.
Used the old kubecli.KubevirtClient clientset.
Used gomock in unit tests.

After this PR:
PodEvictionAdmitter:

Has a factory function and only unexported (private) fields.
Uses kubernetes' official generated clientset.
Uses KubeVirt's generated clientset.
Uses KubeVirt's generated fake clientset in unit tests instead of gomock.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

This is a follow up to #11380.

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

Currently, the fields of `PodEvictionAdmitter` are exported (public), and are directly assigned by its users. This has the potential to incorrectly instantiate the struct. Introduce a factory function in order to: 1. Have a single way of instantiating `PodEvictionAdmitter`. 2. Make its fields unexported (private) in the next commit. Signed-off-by: Orel Misan <omisan@redhat.com>

Following the introduction of the factory function in the previous commit, it is now possible to unexport the fields of `PodEvictionAdmitter`. Signed-off-by: Orel Misan <omisan@redhat.com>

orelmisan · 2024-05-12T18:39:24Z

/cc @fossedihelm

Currently, `PodEvictionAdmitter` uses the `kubecli.KubevirtClient` interface, which contains K8s clientset interface and KubeVirt's manual clientset. In order to use KubeVirt's generated clientset and fake clientset in the following commit, separate the clientsets used by `PodEvictionAdmitter` to: 1. kubeClient - to fetch pods 2. virtClient - to get and patch VMIs In the unit tests, the fake K8s clientset is used directly. There is no longer a need for virtClient to return the fake K8s clientset. Signed-off-by: Orel Misan <omisan@redhat.com>

orelmisan · 2024-05-12T19:17:33Z

Change 1: Remove the permission of gomock to return the fake k8s clientset.
Change 2: Fix linter issue.

orelmisan · 2024-05-15T09:41:31Z

/sig code-quality

enp0s3

@orelmisan Thank you!
/approve

kubevirt-bot · 2024-05-21T07:28:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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~~ [enp0s3]

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

fossedihelm

Thank you @orelmisan!
Even if the generated client(kubevirt.Interface) provides the same interface of our custom KubevirtClient kubecli.KubevirtClient we need to keep in mind that it currently does not implement all the custom (_expansion file) methods such as Pause, Freeze, etc...
A step ahead in this direction is provided by #11928, but it still does not implement some methods that require clientConfig.
Here we only use VirtualMachineInstances.Get() so everything works.
But we must be aware that the generated client that we are using with this patch still does not cover all the methods provided by the custom KubevirtClient.
@enp0s3 FYI

enp0s3 · 2024-05-21T08:48:27Z

@fossedihelm Thanks for the heads up! shouldn't the test fail in case the code will try to use the custom API calls that aren't implemented by the current client set?

RamLavi · 2024-05-21T08:52:34Z

pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter_test.go

@@ -541,6 +486,26 @@ func newDeniedAdmissionResponse(message string) *admissionv1.AdmissionResponse {
 	}
 }

+func newExpectedPatch(vmi *virtv1.VirtualMachineInstance) testing.PatchActionImpl {


the name newExpectedPatch sounds generic, but the patch changes something very specific. Please consider renaming the function to be more specific, or add the patch as a input param.

Thank you for catching it.
I agree with you.
Done.

fossedihelm · 2024-05-21T09:01:57Z

@fossedihelm Thanks for the heads up! shouldn't the test fail in case the code will try to use the custom API calls that aren't implemented by the current client set?

@enp0s3 Currently, the generated client will not return any error in such cases.
I will add a panic in #11928 as suggested in previous PR #11928.
If you agree I will wait for it to be merged first.

Use the generated KubeVirt clientset interface, instead of the old manual one. This allows us to use the generated fakeclient in unit tests, instead of using gomock [1] (which has been archived). [1] https://github.com/golang/mock Signed-off-by: Orel Misan <omisan@redhat.com>

evictionStrategy was misspelled. Signed-off-by: Orel Misan <omisan@redhat.com>

orelmisan · 2024-05-21T10:32:38Z

Change: Address @RamLavi's comment.

RamLavi

/lgtm

kubevirt-commenter-bot · 2024-05-21T10:47:06Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

kubevirt-commenter-bot · 2024-05-22T02:47:39Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

orelmisan added 2 commits May 12, 2024 21:22

Eviction admitter: Unexport PodEvictionAdmitter fields

9835fa4

Following the introduction of the factory function in the previous commit, it is now possible to unexport the fields of `PodEvictionAdmitter`. Signed-off-by: Orel Misan <omisan@redhat.com>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L sig/buildsystem Denotes an issue or PR that relates to changes in the build system. labels May 12, 2024

kubevirt-bot requested review from mhenriks and xpivarc May 12, 2024 18:38

kubevirt-bot requested a review from fossedihelm May 12, 2024 18:39

orelmisan force-pushed the eviction-admitter-fake branch 2 times, most recently from d80218d to 24839d4 Compare May 12, 2024 19:15

kubevirt-bot added the sig/code-quality label May 15, 2024

enp0s3 reviewed May 21, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2024

fossedihelm reviewed May 21, 2024

View reviewed changes

RamLavi reviewed May 21, 2024

View reviewed changes

orelmisan added 2 commits May 21, 2024 13:29

Eviction admitter, tests: Fix a typo

edd8ed0

evictionStrategy was misspelled. Signed-off-by: Orel Misan <omisan@redhat.com>

orelmisan force-pushed the eviction-admitter-fake branch from ca710a5 to edd8ed0 Compare May 21, 2024 10:31

RamLavi approved these changes May 21, 2024

View reviewed changes

kubevirt-bot assigned RamLavi May 21, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024

kubevirt-bot merged commit 3d7053c into kubevirt:main May 22, 2024
38 checks passed

orelmisan deleted the eviction-admitter-fake branch May 22, 2024 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eviction admitter: Use KubeVirt generated clientset #11897

Eviction admitter: Use KubeVirt generated clientset #11897

orelmisan commented May 12, 2024 •

edited

orelmisan commented May 12, 2024

orelmisan commented May 12, 2024

orelmisan commented May 15, 2024

enp0s3 left a comment

kubevirt-bot commented May 21, 2024

fossedihelm left a comment

enp0s3 commented May 21, 2024

RamLavi May 21, 2024 •

edited

orelmisan May 21, 2024

fossedihelm commented May 21, 2024 •

edited

orelmisan commented May 21, 2024

RamLavi left a comment

kubevirt-commenter-bot commented May 21, 2024

kubevirt-commenter-bot commented May 22, 2024

Eviction admitter: Use KubeVirt generated clientset #11897

Eviction admitter: Use KubeVirt generated clientset #11897

Conversation

orelmisan commented May 12, 2024 • edited

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

orelmisan commented May 12, 2024

orelmisan commented May 12, 2024

orelmisan commented May 15, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

kubevirt-bot commented May 21, 2024

fossedihelm left a comment

Choose a reason for hiding this comment

enp0s3 commented May 21, 2024

RamLavi May 21, 2024 • edited

Choose a reason for hiding this comment

orelmisan May 21, 2024

Choose a reason for hiding this comment

fossedihelm commented May 21, 2024 • edited

orelmisan commented May 21, 2024

RamLavi left a comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented May 21, 2024

kubevirt-commenter-bot commented May 22, 2024

orelmisan commented May 12, 2024 •

edited

RamLavi May 21, 2024 •

edited

fossedihelm commented May 21, 2024 •

edited