Abstract Command Client #10992

xpivarc · 2024-01-09T13:09:57Z

What this PR does / why we need it:
This PR is abstracting two things.
The first being is the creation of a client for the Command Server. The following interface is introduced:

type ClientManager interface {
	Create(vmi *v1.VirtualMachineInstance) (LauncherClient, error)
	Socket(vmi *v1.VirtualMachineInstance) (string, error)
	IsNotResponsive(socketPath string) bool
}

This allows us to write simpler tests in the future and resue one implementation in multiple places (search for FindSocketOnHost).

The second thing that is handled here is the usage of FindPodDirOnHost which has the same drawbacks. For this, we simply extend PodIsolationDetector, see below:

type PodIsolationDetector interface {
        ...
	DetectPod(vmi *v1.VirtualMachineInstance) error
        ...

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:
Note: This PR doesn't convert everything into a new abstraction and does not leverage it for tests. This will be follow-up work. The implementation is almost identical to the old one. We use the real implementation for the tests and that ensures we do not break tests and production code as well.

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

kubevirt-bot · 2024-01-09T13:10:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from xpivarc. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

orelmisan · 2024-01-09T13:12:23Z

/cc

pkg/virt-handler/cmd-client/manager.go

fossedihelm

Thanks @xpivarc Some comments below

pkg/virt-handler/vm.go

pkg/virt-handler/cmd-client/manager.go

pkg/virt-handler/isolation/detector.go

fossedihelm · 2024-01-09T14:03:16Z

pkg/virt-handler/isolation/detector.go

@@ -50,6 +50,10 @@ type PodIsolationDetector interface {

 	DetectForSocket(vm *v1.VirtualMachineInstance, socket string) (IsolationResult, error)

+	// Detects if there is a Pod on the host for given vm
+	// Returns error otherwise
+	DetectPod(vmi *v1.VirtualMachineInstance) error


Shouldn't this return the socketPodDir, error otherwise?
Otherwise I would tend to switch to something like:
DoesPodExists(vmi *v1.VirtualMachineInstance) (bool, error)

The socketPodDir is unused. In fact FindPodDirOnHost is now only used once, here.
I thought about (bool, error) but I am not sure if there is any value. For now, I like that it preserves the behavior and I don't need to add additional coverage.

I am open to suggestions for names but I do reserve to keep the logic in this PR as close as it was and only follow up with improvements to the logic once we cover this with more unit tests.

It's ok to me to leave the things as you proposed, with the promise that FindPodDirOnHost will be refactored in the future to drop the socketPodDir, if is not needed at all.
IOW my only concern is that we will forget that FindPodDirOnHost is returning an unused value.

orelmisan · 2024-01-09T18:56:29Z

pkg/virt-handler/cmd-client/manager.go

+}
+
+func (cm *clientManager) Socket(vmi *v1.VirtualMachineInstance) (string, error) {
+	return FindSocketOnHost(vmi)


Could the socket path found inCreate() be saved as a member and returned here?

This would require also cleaning it later. I have reverted the first commit to a simpler solution, please revisit.

As of now we directly search the filesystem to find the socket and then initialize a client for the command server in the launcher. This is harder to test and change than it could be. This commit encapsulates this into a simple yet effective abstraction. The next step will be to use this abstraction in all places that require the command client. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

FindPodDirOnHost is a simple function that goes through the file system to determine if there is a Pod for a given VMI. This direct approach makes testing problematic and therefore we are extending the Isolation Detector to cover this logic instead. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc · 2024-01-09T20:02:01Z

@fossedihelm @orelmisan PTAL

fossedihelm

Thanks @xpivarc!
I like the new simplified logic in the first commit.
One small last nit :)

fossedihelm · 2024-01-09T22:28:31Z

pkg/virt-handler/cmd-client/manager.go

+ *
+ * Copyright 2023 The KubeVirt Authors.
+ *
+ */


nit: missing newline
(sorry to be pedantic) 🙂

I adjust if any changes are requested.

fossedihelm · 2024-01-09T22:41:06Z

Please, also, adjust the PR description with the new function name for ClientManager

xpivarc · 2024-01-10T09:32:42Z

Please, also, adjust the PR description with the new function name for ClientManager

Done

fossedihelm · 2024-01-10T09:36:11Z

/lgtm

orelmisan · 2024-01-10T15:21:29Z

/hold

@xpivarc and I had an offline discussion, we agreed that this PR could be simplified if rebased on top of #10837.
Furthermore we found some issues with the suggested ClientManager abstraction that are worth revisiting.

kubevirt-bot · 2024-01-20T14:45:53Z

PR needs rebase.

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.

kubevirt-bot · 2024-05-04T00:27:28Z

@xpivarc: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations`
pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations`
pull-kubevirt-e2e-k8s-1.29-sig-monitoring	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring`
pull-kubevirt-e2e-k8s-1.27-sig-performance	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.27-sig-performance`
pull-kubevirt-e2e-k8s-1.29-sig-performance	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-performance`
pull-kubevirt-e2e-k8s-1.30-sig-compute-serial	`be74906`	link	true	`/test pull-kubevirt-e2e-k8s-1.30-sig-compute-serial`

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. I understand the commands that are listed here.

kubevirt-bot · 2024-08-02T01:12:49Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubevirt-bot · 2024-09-01T01:28:02Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

kubevirt-bot · 2024-09-23T14:18:07Z

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

kubevirt-bot · 2024-09-23T14:18:11Z

@kubevirt-bot: The label(s) needs-approver-review cannot be applied, because the repository doesn't have them.

In response to this:

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

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-sigs/prow repository.

kubevirt-bot · 2024-10-23T15:16:56Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

kubevirt-bot · 2024-10-23T15:17:00Z

@kubevirt-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

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-sigs/prow repository.

kubevirt-bot · 2024-10-25T14:48:36Z

/remove-label needs-approver-review

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jan 9, 2024

kubevirt-bot requested review from lyarwood and stu-gott January 9, 2024 13:11

kubevirt-bot added the size/M label Jan 9, 2024

kubevirt-bot requested a review from orelmisan January 9, 2024 13:12

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jan 9, 2024

orelmisan reviewed Jan 9, 2024

View reviewed changes

pkg/virt-handler/cmd-client/manager.go Show resolved Hide resolved

pkg/virt-handler/cmd-client/manager.go Outdated Show resolved Hide resolved

pkg/virt-handler/cmd-client/manager.go Outdated Show resolved Hide resolved

fossedihelm suggested changes Jan 9, 2024

View reviewed changes

kubevirt-bot assigned fossedihelm Jan 9, 2024

orelmisan reviewed Jan 9, 2024

View reviewed changes

xpivarc added 2 commits January 9, 2024 20:56

xpivarc force-pushed the cmd-client branch from c76bc88 to be74906 Compare January 9, 2024 20:01

kubevirt-bot added size/L and removed size/M labels Jan 9, 2024

fossedihelm reviewed Jan 9, 2024

View reviewed changes

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

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2024

kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 2, 2024

kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 1, 2024

dhiller added the needs-approver-review Indicates that a PR requires a review from an approver. label Sep 23, 2024

kubevirt-bot closed this Oct 23, 2024

kubevirt-bot removed the needs-approver-review Indicates that a PR requires a review from an approver. label Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract Command Client #10992

Abstract Command Client #10992

xpivarc commented Jan 9, 2024 •

edited

Loading

kubevirt-bot commented Jan 9, 2024

orelmisan commented Jan 9, 2024

fossedihelm left a comment

fossedihelm Jan 9, 2024

xpivarc Jan 9, 2024

fossedihelm Jan 9, 2024

orelmisan Jan 9, 2024

xpivarc Jan 9, 2024

xpivarc commented Jan 9, 2024

fossedihelm left a comment

fossedihelm Jan 9, 2024

xpivarc Jan 10, 2024

fossedihelm commented Jan 9, 2024

xpivarc commented Jan 10, 2024

fossedihelm commented Jan 10, 2024

orelmisan commented Jan 10, 2024 •

edited

Loading

kubevirt-bot commented Jan 20, 2024

kubevirt-bot commented May 4, 2024

kubevirt-bot commented Aug 2, 2024

kubevirt-bot commented Sep 1, 2024

kubevirt-bot commented Sep 23, 2024

kubevirt-bot commented Sep 23, 2024

kubevirt-bot commented Oct 23, 2024

kubevirt-bot commented Oct 23, 2024

kubevirt-bot commented Oct 25, 2024

Abstract Command Client #10992

Abstract Command Client #10992

Conversation

xpivarc commented Jan 9, 2024 • edited Loading

kubevirt-bot commented Jan 9, 2024

orelmisan commented Jan 9, 2024

fossedihelm left a comment

Choose a reason for hiding this comment

fossedihelm Jan 9, 2024

Choose a reason for hiding this comment

xpivarc Jan 9, 2024

Choose a reason for hiding this comment

fossedihelm Jan 9, 2024

Choose a reason for hiding this comment

orelmisan Jan 9, 2024

Choose a reason for hiding this comment

xpivarc Jan 9, 2024

Choose a reason for hiding this comment

xpivarc commented Jan 9, 2024

fossedihelm left a comment

Choose a reason for hiding this comment

fossedihelm Jan 9, 2024

Choose a reason for hiding this comment

xpivarc Jan 10, 2024

Choose a reason for hiding this comment

fossedihelm commented Jan 9, 2024

xpivarc commented Jan 10, 2024

fossedihelm commented Jan 10, 2024

orelmisan commented Jan 10, 2024 • edited Loading

kubevirt-bot commented Jan 20, 2024

kubevirt-bot commented May 4, 2024

kubevirt-bot commented Aug 2, 2024

kubevirt-bot commented Sep 1, 2024

kubevirt-bot commented Sep 23, 2024

kubevirt-bot commented Sep 23, 2024

kubevirt-bot commented Oct 23, 2024

kubevirt-bot commented Oct 23, 2024

kubevirt-bot commented Oct 25, 2024

xpivarc commented Jan 9, 2024 •

edited

Loading

orelmisan commented Jan 10, 2024 •

edited

Loading