netstat: Update VMI interface status report unit tests #6951

EdDev · 2021-12-15T09:42:51Z

What this PR does / why we need it:

The existing unit tests which cover the VMI interface status report is missing some realistic overall scenarios about the existing data available.

This change unifies and tries to place some rules on what is the expected data provided to the report processing.

The current netstat unit tests have been updated to reflect the
following expected behavior:

When regular interfaces are added, this is the expected state/input:
- VMI spec interfaces and networks should be in sync with 1:1 relation.
- Domain spec is expect to reflect the VMI spec, i.e. having the same
  interfaces.
- The filesystem cache should reflect the same interfaces as the VMI
  spec.
- The volatile cache is optional, reflecting the same interface.
- The Guest Agent data is optional.
When SR-IOV interfaces are added, this is the expected state/input:
- VMI spec interfaces and networks should be in sync with 1:1 relation.
- The Guest Agent data is optional.
  Notes: Caches do not exist and the domain spec interfaces do not
  include the SR-IOV interfaces. These are under the hostdevice section.

Miscellaneous scenarios are also covered to reflect what happens when such cases occur. However, these are not expected to occur in a normal system operation. Future production changes may treat these scenarios explicitly to avoid undefined and nontrivial behavior.

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:
~~Depends on #6926~~
~~Only the last 6 commits are to be reviewed as part of this PR.~~

Release note:

NONE

EdDev · 2021-12-15T09:44:20Z

/uncc rmohr vladikr

EdDev · 2021-12-15T09:45:38Z

/cc @maiqueb @AlonaKaplan @ormergi @oshoval

EdDev · 2021-12-15T09:45:55Z

/sig network

EdDev · 2021-12-16T14:46:22Z

change: Added misc test scenarios

EdDev · 2021-12-23T09:25:13Z

change: Rebase

oshoval · 2021-12-26T08:23:51Z

pkg/network/setup/netstat_test.go

@@ -278,6 +278,59 @@ var _ = Describe("netstat", func() {
 			newVMIStatusIface(networkName, nil, ifaceMAC, guestIfaceName),
 		}), "the SR-IOV interface should be reported in the status, associated to the network")
 	})
+
+	Context("misc scenario", func() {


maybe worth to add in the Context that this is unrealistic scenarios ?
(misc word is too general, and usually we read the code and not going to the commit message / PR desc which does elaborate it in this case)

It is important to people won't think those are the expected flows
while we develop or even during review, it can cause noise if misunderstood

there are some tests here that should not happen at all (unless there is hot plug), so i am not sure it is good to include them, for example has interface in VMI spec but not in domain spec IIUC (i do understand the motivation and saw the PR desc)

Unrealistic to whom?
These are indeed edge cases which are the result of the unit API. We can argue that these scenarios have not been expected to occur, at least by their users. However, the unit does allow them to exist, just because the API supports them. Therefore, they should be covered to express what will happen in these cases.

Although we expect not to see these scenarios, they may occur due to bugs or changes to the callers of this unit. In the context of this unit, it has no knowledge on how it is called or by who, and it should not care.

Now that these scenarios are exposed here, a decision may be made to disallow them (e.g. raise an error) or change the output result. But this is not in the scope of this change.

Finally, regarding the name: I consider these miscellaneous scenarios and I did not find any better name for this goup of tests. I do not think they fit to the word "unrealistic" because that is in the context of a caller, not the unit itself.

oshoval

first pass

oshoval · 2021-12-26T09:23:43Z

pkg/network/setup/netstat_test.go

+	if !(vmiIface.Name == vmiNetwork.Name && vmiIface.Name == domainIface.Alias.GetName()) {
+		panic("network name must be the same")
+	}


why not simply ExpectWithOffset ?
(appears at least twice in the PR)
i understand that it is not runtime but more 'config' phase, but we aren't using panic usually,
and Expect will do the same result

To differentiate between the code-under-test failing and the test writer placing an unsupported config in place.

oshoval · 2021-12-28T07:37:50Z

pkg/network/setup/netstat_test.go

 			newVMIStatusIface(primaryNetworkName, []string{newGaIPv4, newGaIPv6}, origMAC, ""),
 		}), "the pod IP/s should be reported in the status")
 	})

-	It("should add a new interface based on the domain spec", func() {
+	It("should report SR-IOV interface when no guest-agent is active", func() {


Please consider adding a test with 2 interfaces (without guest agent)

Non SR-IOV

SR-IOV

Because the behavior would be different, as the main loop of UpdateStatus would override the interfaces added by the snippet
https://github.com/kubevirt/kubevirt/pull/6951/files#diff-c2b23c7cf6d14c6f6a9e12f1a63d6c8004ead588e9d4d6a369aeea15701ca086R79

EdDev · 2021-12-30T13:01:07Z

change: Rebase

EdDev · 2021-12-30T13:44:36Z

Change: Answered review: Added a SR-IOV test to show what happens when it is accompanied with a "regular" interface.

EdDev · 2022-01-09T06:42:20Z

change: Rebase
The reabse required the inclusion of infoSource to the unit tests.

AlonaKaplan · 2022-01-09T12:01:28Z

pkg/network/setup/netstat_test.go

@@ -244,37 +244,6 @@ var _ = Describe("netstat", func() {
 		}), "the pod IP/s should be reported in the status")
 	})

-	It("should add a new interface based on the domain spec", func() {


I don't understand the commit message. Why such option is not supported? What if there is no GA at all?

The VMI spec is not updated with any interfaces, so having only the domain defined but not the vmi spec is odd.

Later on in the PR, I re-introduces this case in a more direct manner. see here.

I tried to express it better in the commit message.

AlonaKaplan

Great work!

AlonaKaplan · 2022-01-09T13:16:04Z

pkg/network/setup/netstat_test.go

+	ifaceFSCacheFactory *interfaceCacheFactoryStatusStub
+
+	// There are two types of caches used: virt-launcher/pod filesystem & virt-handler in-memory (volatile).
+	// volatileCache flag marks is the setup should also populate the volatile cache when a network interface is added.


"marks that the setup"?

AlonaKaplan · 2022-01-09T13:39:55Z

pkg/network/setup/netstat_test.go

+		}), "the SR-IOV interface should be reported in the status, associated to the network")
+	})
+
+	It("should not report SR-IOV interface when guest-agent is inactive and a regular interface exists", func() {


This sounds like a bug. The appearance of the sriov interface shouldn't be effected by another interface existence.

Yes, SR-IOV reporting is buggy.
On one hand, it is reported sometimes even if there is no "proof" of it being there. On the other hand, it is not reported if at least one non-SRIOV interface is there.

None of these are correct, both represent a bug: When the guest has no guest-agent, SR-IOV interfaces are not reported correctly (either not at all, or based on false data).

But al least now we express this clearly through the tests.

I opened #7050 to track this.
Ref it also in the commit message.

Can you please add the ref to #7050 to the code as well? So the next developer that will see those tests won't think it is "by design" behaviour.

Sure, I'll add it to the tests.

In the current implementation, the domain spec is directly affected by the VMI spec (in the network interfaces scope). The scenario were the domain is configured with interfaces that do not appear in the VMI spec is no supported (or expected). Therefore, the test that checks such a scenario (in an implicit manner) is removed. Signed-off-by: Edward Haas <edwardh@redhat.com>

Tests should represent real scenarios which are to be expected. Undefined scenario have their own place, but those need to be explicit and show what may happen in such cases as exceptions. The current netstat unit tests have been updated to reflect the following expected behavior: - When regular interfaces are added, this is the expected state/input: - VMI spec interfaces and networks should be in sync with 1:1 relation. - Domain spec is expect to reflect the VMI spec, i.e. having the same interfaces. - The filesystem cache should reflect the same interfaces as the VMI spec. - The volatile cache is optional, reflecting the same interface. - The Guest Agent data is optional. - When SR-IOV interfaces are added, this is the expected state/input: - VMI spec interfaces and networks should be in sync with 1:1 relation. - The Guest Agent data is optional. Notes: Caches do not exist and the domain spec interfaces do not include the SR-IOV interfaces. These are under the hostdevice section. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2022-01-09T14:55:56Z

change: Answered review feedback.

When an SR-IOV interface is present in the VMI spec and the guest is not running the guest-agent, the interface is appearing in the status section with just the network name and no other data. On the other hand, if another non SR-IOV interface exists, the result changes and the SR-IOV interface will not appear in the report at all. The behavior observed is tracked through: kubevirt#7050 Signed-off-by: Edward Haas <edwardh@redhat.com>

The unit tests included in this change represent undefined behavior. These scenarios are explicitly represented through dedicated tests to show the expected result if such a case occurs. Future changes may suggest to change this output and replace it with something that fits better (e.g. do not show the interface entry). Signed-off-by: Edward Haas <edwardh@redhat.com>

The volatile cache needs to be populated based on the non-volatile cache. Signed-off-by: Edward Haas <edwardh@redhat.com>

AlonaKaplan · 2022-01-10T07:24:08Z

/approve

kubevirt-bot · 2022-01-10T07:24:21Z

[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 · 2022-01-10T11:41:03Z

/retest

EdDev · 2022-01-10T17:16:57Z

/retest

EdDev · 2022-01-10T19:00:06Z

/retest

kubevirt-commenter-bot · 2022-01-11T01:18:31Z

/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 · 2022-01-11T07:06:29Z

/retest

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/XXL labels Dec 15, 2021

kubevirt-bot requested review from rmohr and vladikr December 15, 2021 09:43

kubevirt-bot removed request for vladikr and rmohr December 15, 2021 09:44

kubevirt-bot requested review from AlonaKaplan, maiqueb, ormergi and oshoval December 15, 2021 09:45

kubevirt-bot added the sig/network label Dec 15, 2021

EdDev force-pushed the refactor-network-update-status-utests branch from f5bed9b to 1c5e61f Compare December 23, 2021 09:25

kubevirt-bot added size/XL and removed size/XXL labels Dec 23, 2021

oshoval reviewed Dec 26, 2021

View reviewed changes

oshoval reviewed Dec 28, 2021

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 28, 2021

EdDev mentioned this pull request Dec 30, 2021

netstat: Drop dependencies between status report iterations and simplify the data collection from the pod #6926

Merged

EdDev force-pushed the refactor-network-update-status-utests branch from 1c5e61f to 9fc55e8 Compare December 30, 2021 13:00

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

EdDev force-pushed the refactor-network-update-status-utests branch from 9fc55e8 to 05b3465 Compare December 30, 2021 13:43

kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 9, 2022

oshoval approved these changes Jan 9, 2022

View reviewed changes

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

AlonaKaplan reviewed Jan 9, 2022

View reviewed changes

EdDev added 2 commits January 9, 2022 16:18

EdDev mentioned this pull request Jan 9, 2022

SR-IOV interfaces are not reported correctly when the guest has no guest-agent. #7050

Closed

EdDev force-pushed the refactor-network-update-status-utests branch from 3920609 to 5e8f178 Compare January 9, 2022 14:54

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2022

EdDev mentioned this pull request Jan 10, 2022

network, netstat: Simplify status update #7051

Merged

EdDev added 3 commits January 10, 2022 09:06

netstat, test: Validate that the volatile cache is populated

0c5b65d

The volatile cache needs to be populated based on the non-volatile cache. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev force-pushed the refactor-network-update-status-utests branch from 5e8f178 to 0c5b65d Compare January 10, 2022 07:06

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2022

oshoval approved these changes Jan 10, 2022

View reviewed changes

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

kubevirt-bot merged commit e18d164 into kubevirt:main Jan 11, 2022

EdDev deleted the refactor-network-update-status-utests branch January 11, 2022 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netstat: Update VMI interface status report unit tests #6951

netstat: Update VMI interface status report unit tests #6951

EdDev commented Dec 15, 2021 •

edited

EdDev commented Dec 15, 2021

EdDev commented Dec 15, 2021

EdDev commented Dec 15, 2021

EdDev commented Dec 16, 2021

EdDev commented Dec 23, 2021

oshoval Dec 26, 2021 •

edited

EdDev Dec 27, 2021

oshoval left a comment

oshoval Dec 26, 2021 •

edited

EdDev Dec 26, 2021

oshoval Dec 28, 2021

EdDev Dec 30, 2021

EdDev commented Dec 30, 2021

EdDev commented Dec 30, 2021 •

edited

EdDev commented Jan 9, 2022

AlonaKaplan Jan 9, 2022 •

edited

EdDev Jan 9, 2022

EdDev Jan 9, 2022

AlonaKaplan left a comment

AlonaKaplan Jan 9, 2022

EdDev Jan 9, 2022

AlonaKaplan Jan 9, 2022

EdDev Jan 9, 2022

AlonaKaplan Jan 10, 2022

EdDev Jan 10, 2022

EdDev Jan 10, 2022

EdDev commented Jan 9, 2022 •

edited

AlonaKaplan commented Jan 10, 2022

kubevirt-bot commented Jan 10, 2022

EdDev commented Jan 10, 2022

EdDev commented Jan 10, 2022

EdDev commented Jan 10, 2022

kubevirt-commenter-bot commented Jan 11, 2022

EdDev commented Jan 11, 2022

netstat: Update VMI interface status report unit tests #6951

netstat: Update VMI interface status report unit tests #6951

Conversation

EdDev commented Dec 15, 2021 • edited

EdDev commented Dec 15, 2021

EdDev commented Dec 15, 2021

EdDev commented Dec 15, 2021

EdDev commented Dec 16, 2021

EdDev commented Dec 23, 2021

oshoval Dec 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshoval left a comment

Choose a reason for hiding this comment

oshoval Dec 26, 2021 • edited

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

EdDev commented Dec 30, 2021

EdDev commented Dec 30, 2021 • edited

EdDev commented Jan 9, 2022

AlonaKaplan Jan 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

EdDev commented Jan 9, 2022 • edited

AlonaKaplan commented Jan 10, 2022

kubevirt-bot commented Jan 10, 2022

EdDev commented Jan 10, 2022

EdDev commented Jan 10, 2022

EdDev commented Jan 10, 2022

kubevirt-commenter-bot commented Jan 11, 2022

EdDev commented Jan 11, 2022

EdDev commented Dec 15, 2021 •

edited

oshoval Dec 26, 2021 •

edited

oshoval Dec 26, 2021 •

edited

EdDev commented Dec 30, 2021 •

edited

AlonaKaplan Jan 9, 2022 •

edited

EdDev commented Jan 9, 2022 •

edited