network: Extract setup and status update logic from the virt-handler package #6781

EdDev · 2021-11-11T09:32:56Z

What this PR does / why we need it:

Decouple network setup and status logic out of the virt-handler.
The network setup is now handled by the NetConf object and the status updates by NetStat.

Production code and tests moved under the network package, leaving only a slim check that both setup and status are actually called in the overall flow.

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:
Following PR/s should continue and:

Simplify the NentConf/Setup signature by embedding DoNetNS into the setup itself.
Remove some other network related logic from vm.go (e.g. checkNetworkInterfacesForMigration).
Refactor and simplified network code further.

Release note:

NONE

EdDev · 2021-11-11T09:35:26Z

/sig network

EdDev · 2021-11-11T09:35:58Z

/cc @AlonaKaplan @qinqon @maiqueb

pkg/network/setup/controller.go

EdDev · 2021-11-11T13:34:34Z

/retest

EdDev · 2021-11-14T10:47:34Z

/test all

maiqueb · 2021-11-24T09:58:30Z

/assign @maiqueb

Internal logic of the network setup is decoupled from the virt-handler unit tests by using a stub. The existing tests can check now that the network setup is invoked as before, but without the need to know the internals of how the network setup works. Signed-off-by: Edward Haas <edwardh@redhat.com>

Signed-off-by: Edward Haas <edwardh@redhat.com>

Keep a test that checks the scenario where the network status update is called from the main flow. Signed-off-by: Edward Haas <edwardh@redhat.com>

For consistency, the (semi) reverse operation of the setup is now called teardown. The method rename is introduced with documentation to clarify that the teardown involves only the cache cleanup, and not the removal of the network entities (these are removed as part of the pod/container destruction). Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2021-12-14T12:47:51Z

change: Answered comment.

maiqueb · 2021-12-14T12:49:37Z

change: Answered comment.

/lgtm

thanks!

AlonaKaplan

Commit 1 review

AlonaKaplan · 2021-12-20T11:45:31Z

pkg/network/setup/controller.go

+}
+
+func (c *Controller) Teardown(vmi *v1.VirtualMachineInstance, do func() error) error {
+	c.setupCompleted.Delete(vmi.UID)


It seems to be safer to move the Delete to be after the do. Is there a special reason for the current order?

Once teardown is called, the cache should be cleared even if the operation partially fails. Otherwise, we will have leftovers.
But again, this is following the current logic.

AlonaKaplan · 2021-12-20T11:49:35Z

pkg/network/setup/controller.go

+		return fmt.Errorf("setup failed, err: %w", err)
+	}
+
+	c.setupCompleted.Store(id, struct{}{})


Since sync.Map is used I understand the code here should be thread safe. The suggested code is not thread safe. Multiple threads can perform the setup simultaneously.

This is mimicking the existing code, nothing logically changed.

The thread-safety if for the cache itself, as multiple threads can access that "database". The setup itself is thread safe as only one VMI can be treated at a time, so there is nothing special to protect here.

AlonaKaplan · 2021-12-20T12:16:36Z

pkg/virt-handler/vm.go

-			return neterrors.CreateCriticalNetworkError(fmt.Errorf("failed to set up vhost-net device, %s", err))
+	return d.networkController.Setup(vmi, isolationRes.Pid(), isolationRes.DoNetNS, func() error {
+		if requiresDeviceClaim {
+			if err := d.claimDeviceOwnership(rootMount, "vhost-net"); err != nil {


Why did you move rootMount := isolationRes.MountRoot() outside the if?

In order not to bind (called closure) the whole isolation object to the function, just the needed data. I wanted to take only the minimum needed.

AlonaKaplan · 2021-12-20T12:25:19Z

pkg/virt-handler/vm_test.go

@@ -591,7 +597,6 @@ var _ = Describe("VirtualMachineInstance", func() {
 			Expect(mockQueue.GetRateLimitedEnqueueCount()).To(Equal(0))
 			_, err := os.Stat(mockWatchdog.File(vmi))
 			Expect(os.IsNotExist(err)).To(BeFalse())
-			Expect(controller.phase1NetworkSetupCache.Size()).To(Equal(1))


Why didn't substitute with Expect(controller.networkController.SetupCompleted(vmi)).To(BeTrue())

I saw no reason why we do need to check the state of the network cache in this test.

Do you see any reason for it?

AlonaKaplan · 2021-12-20T12:25:44Z

pkg/virt-handler/vm_test.go

@@ -664,7 +669,6 @@ var _ = Describe("VirtualMachineInstance", func() {
 			mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any()).Return(nil)
 			controller.Execute()
 			testutils.ExpectEvent(recorder, VMIDefined)
-			Expect(controller.phase1NetworkSetupCache.Size()).To(Equal(1))


Same answer, I do not see why we need to check this in this test context. The creation of the domain should be checked, not if there network setup was invoked.
There are dedicated tests for networking.

AlonaKaplan · 2021-12-20T14:01:38Z

pkg/virt-handler/vm_test.go

@@ -2582,8 +2585,7 @@ var _ = Describe("VirtualMachineInstance", func() {
 				PodIP:  podIPs[0],
 				PodIPs: podIPs,
 			}
-			err = controller.networkCacheStoreFactory.CacheForVMI(vmi).Write(interfaceName, podCacheInterface)
-			Expect(err).ToNot(HaveOccurred())
+			controller.networkController.CachePodInterfaceVolatileData(vmi, interfaceName, podCacheInterface)


The original test used the non-volatile cache. Do we have unit test coverage for it?

The non-volatile was used to inject data to see if reports the right thing. I do not think the intention was to check this cache, but to check that the status update does its thing.

So replacing it with the volatile data is both faster and closer to the intention here.

Per what I saw, how the non-volatile cache is used is already covered by other tests, like the ones under https://github.com/kubevirt/kubevirt/blob/main/pkg/network/setup/network_test.go .

Looking at the reporting/status part, at the end of the refactoring some scenarios are checked against the volatile cache and some against the non-volatile one.
But I think this is a small part, as the non-volatile and volatile caches interactions is simple. Maybe it is worth adding a specific test that checks that the non-volatile cache is populated in the volatile cache on first usage.

I have added a test to cover the last point here.

AlonaKaplan · 2021-12-20T14:04:31Z

pkg/virt-handler/vm.go


-	c.networkController = netsetup.NewController(c.networkCacheStoreFactory)
+	c.networkController = netsetup.NewController(netcache.NewInterfaceCacheFactory())


Why do you pass the cache factory as a parameter to the NewController and not initialize it inside the controller?

Same reason it was done until now... it needs to be stubbed/mocked in the tests with one that does not use the filesystem. See here.

AlonaKaplan · 2021-12-21T10:32:49Z

/approve

kubevirt-bot · 2021-12-21T10:33:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

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

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

EdDev · 2021-12-21T10:41:18Z

/retest

EdDev · 2021-12-21T16:09:33Z

/retest

EdDev · 2021-12-21T18:48:40Z

/retest

kubevirt-commenter-bot · 2021-12-22T02:46:45Z

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

EdDev · 2021-12-22T04:20:59Z

/retest

EdDev · 2021-12-22T12:21:19Z

/retest

EdDev · 2021-12-22T14:34:54Z

/retest

kubevirt-commenter-bot · 2021-12-23T01:48:36Z

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

dhiller · 2021-12-23T06:14:30Z

/retest

kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 11, 2021

EdDev marked this pull request as draft November 11, 2021 09:33

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Nov 11, 2021

kubevirt-bot requested review from dhiller and zcahana November 11, 2021 09:33

kubevirt-bot added the sig/network label Nov 11, 2021

kubevirt-bot requested review from AlonaKaplan, maiqueb and qinqon November 11, 2021 09:35

maiqueb reviewed Nov 11, 2021

View reviewed changes

pkg/network/setup/controller.go Outdated Show resolved Hide resolved

EdDev force-pushed the refactor-network-setup branch from e593696 to c3e189a Compare November 14, 2021 10:46

EdDev force-pushed the refactor-network-setup branch from c3e189a to 796b942 Compare November 14, 2021 12:19

EdDev marked this pull request as ready for review November 14, 2021 12:20

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2021

EdDev force-pushed the refactor-network-setup branch from 796b942 to de6e3d3 Compare November 16, 2021 05:42

kubevirt-bot added size/XL and removed size/L labels Nov 16, 2021

EdDev force-pushed the refactor-network-setup branch from de6e3d3 to 5284191 Compare November 16, 2021 05:43

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL and removed size/XL labels Nov 24, 2021

EdDev force-pushed the refactor-network-setup branch from eb0f2a8 to 1d5a0b1 Compare November 24, 2021 09:50

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2021

EdDev added 8 commits December 14, 2021 14:44

network, setup: Move status SR-IOV unit tests from vm_test

3cc9c16

Signed-off-by: Edward Haas <edwardh@redhat.com>

network, setup: Move iface MTU status change unit tests from vm_test

f6a20d5

Signed-off-by: Edward Haas <edwardh@redhat.com>

network, setup: Move iface IP status change unit tests from vm_test

7a5a5ac

Signed-off-by: Edward Haas <edwardh@redhat.com>

network, setup: Move the addition of an iface unit tests from vm_test

1814a4d

Signed-off-by: Edward Haas <edwardh@redhat.com>

network, setup: Move iface name status change unit tests from vm_test

6c298d6

Signed-off-by: Edward Haas <edwardh@redhat.com>

virt-handler, vm_test: Test VMI network status update using a stub

d8af492

Keep a test that checks the scenario where the network status update is called from the main flow. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev force-pushed the refactor-network-setup branch from dadff7e to 75b0bd4 Compare December 14, 2021 12:47

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

EdDev mentioned this pull request Dec 19, 2021

netstat: Simplify pod interface volatile cache #6971

Merged

AlonaKaplan reviewed Dec 20, 2021

View reviewed changes

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

kubevirt-bot merged commit d01219d into kubevirt:main Dec 23, 2021

EdDev deleted the refactor-network-setup branch December 28, 2021 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: Extract setup and status update logic from the virt-handler package #6781

network: Extract setup and status update logic from the virt-handler package #6781

EdDev commented Nov 11, 2021 •

edited

Loading

EdDev commented Nov 11, 2021

EdDev commented Nov 11, 2021

EdDev commented Nov 11, 2021

EdDev commented Nov 14, 2021

maiqueb commented Nov 24, 2021

EdDev commented Dec 14, 2021

maiqueb commented Dec 14, 2021

AlonaKaplan left a comment

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

EdDev Dec 21, 2021

AlonaKaplan Dec 20, 2021

EdDev Dec 20, 2021

AlonaKaplan commented Dec 21, 2021

kubevirt-bot commented Dec 21, 2021

EdDev commented Dec 21, 2021

EdDev commented Dec 21, 2021

EdDev commented Dec 21, 2021

kubevirt-commenter-bot commented Dec 22, 2021

EdDev commented Dec 22, 2021

EdDev commented Dec 22, 2021

EdDev commented Dec 22, 2021

kubevirt-commenter-bot commented Dec 23, 2021

dhiller commented Dec 23, 2021


		c.networkController = netsetup.NewController(c.networkCacheStoreFactory)
		c.networkController = netsetup.NewController(netcache.NewInterfaceCacheFactory())

network: Extract setup and status update logic from the virt-handler package #6781

network: Extract setup and status update logic from the virt-handler package #6781

Conversation

EdDev commented Nov 11, 2021 • edited Loading

EdDev commented Nov 11, 2021

EdDev commented Nov 11, 2021

EdDev commented Nov 11, 2021

EdDev commented Nov 14, 2021

maiqueb commented Nov 24, 2021

EdDev commented Dec 14, 2021

maiqueb commented Dec 14, 2021

AlonaKaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlonaKaplan commented Dec 21, 2021

kubevirt-bot commented Dec 21, 2021

EdDev commented Dec 21, 2021

EdDev commented Dec 21, 2021

EdDev commented Dec 21, 2021

kubevirt-commenter-bot commented Dec 22, 2021

EdDev commented Dec 22, 2021

EdDev commented Dec 22, 2021

EdDev commented Dec 22, 2021

kubevirt-commenter-bot commented Dec 23, 2021

dhiller commented Dec 23, 2021

EdDev commented Nov 11, 2021 •

edited

Loading