virt-handler, Add infoSource field to vmi status #6793

oshoval · 2021-11-14T15:44:10Z

What this PR does / why we need it:

In case of MAC change in the guest,
the vmi status will present two interfaces,
one based on the domain spec, and one based on the
guest agent (if it exists).

The reason is that the MAC address is used in order
to match the interface of the domain spec with the
interfaces reported by the guest agent.
Once the MAC is changed, this matching can't be performed.

In order to make the info easier to understand,
add a new field to each interface in vmi.status.
infoSource field has 3 possible values:

domain - info is based on the domain spec
guest-agent - info is based on guest agent report
domain, guest-agent - info is based on both domain spec
and guest agent report.

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 https://bugzilla.redhat.com/show_bug.cgi?id=2013455

Special notes for your reviewer:
User Guide PR: kubevirt/user-guide#504

Release note:

Add infoSource field to vmi.status.interfaces.
The field specifies the origin of the interface data collected. values: domain, guest-agent, or both.

oshoval · 2021-11-14T15:44:28Z

/uncc @jean-edouard @yuhaohaoyu

oshoval · 2021-11-15T08:10:49Z

/test pull-kubevirt-check-unassigned-tests

oshoval · 2021-11-15T08:29:31Z

/test pull-kubevirt-unit-test

oshoval · 2021-11-16T07:55:19Z

/cc @EdDev

maiqueb · 2021-11-16T08:59:28Z

In case of MAC change in the guest,
the vmi status will present two interfaces,
one based on the domain spec, and one based on the
guest agent (if it exists).

Could you paste some examples of what the status representation will look like, so its easier to visualize ? As is, I am unsure I understand what you're proposing.

oshoval · 2021-11-16T09:05:43Z

Could you paste some examples of what the status representation will look like, so its easier to visualize ? As is, I am unsure I understand what you're proposing.

An interface where the domain and guest-agent both know to interface (usually the case)

  - infoSource: domain, guest-agent # new field of this PR
    interfaceName: eth1
    ipAddress: 10.200.0.1
    ipAddresses:
    - 10.200.0.1
    - fe80::1e:d6ff:fe00:6b
    mac: 02:1e:d6:00:00:6b
    name: br10-vlan100-network

Now if you change the mac of the interface in the guest itself, the situation will be (before this PR)

  - mac: 02:1e:d6:00:00:6b
    name: br10-vlan100-network
  - interfaceName: eth1
    ipAddress: 10.200.0.1
    ipAddresses:
    - 10.200.0.1
    - fe80::1e:d6ff:fe00:6b
    mac: 02:00:b5:b5:b5:c9

This PR suggest to add the infoSource field as follows

  - infoSource: domain
    mac: 02:1e:d6:00:00:6b
    name: br10-vlan100-network
  - infoSource: guest-agent
    interfaceName: eth1
    ipAddress: 10.200.0.1
    ipAddresses:
    - 10.200.0.1
    - fe80::1e:d6ff:fe00:6b
    mac: 02:00:b5:b5:b5:c9

You can see here the 3 possible values of the new field.

maiqueb · 2021-11-16T09:18:23Z

  - infoSource: domain
    mac: 02:1e:d6:00:00:6b
    name: br10-vlan100-network
  - infoSource: guest-agent
    interfaceName: eth1
    ipAddress: 10.200.0.1
    ipAddresses:
    - 10.200.0.1
    - fe80::1e:d6ff:fe00:6b
    mac: 02:00:b5:b5:b5:c9

But do the above 2 entries refer to the same entity ? From your PR's description, I think so.

How can I - or anything that reads the interface status - infer that @oshoval ?

AFAIU, there isn't any attribute there to map them together. Probably if you add a more complete example I'll understand.

AlonaKaplan · 2021-12-30T10:17:29Z

pkg/virt-launcher/virtwrap/agent-poller/agent_parser.go

@@ -249,11 +250,13 @@ func MergeAgentStatusesWithDomainData(domInterfaces []api.Interface, interfaceSt
 	}

 	aliasesCoveredByAgent := []string{}
-	// Add alias from domain to interfaceStatus


why was this comment removed?

Because now it adds also the InfoSource not just the alias,so it is outdated (and code is self explanatory about those 2)

AlonaKaplan · 2021-12-30T10:19:42Z

pkg/network/setup/netstat.go

@@ -158,14 +159,18 @@ func (c *NetStat) UpdateStatus(vmi *v1.VirtualMachineInstance, domain *api.Domai
 			// Remove the interface from domainInterfaceStatusByMac to mark it as handled
 			if interfaceStatus, exists := domainInterfaceStatusByMac[interfaceMAC]; exists {
 				newInterface.InterfaceName = interfaceStatus.InterfaceName
+				newInterface.InfoSource = interfaceStatus.InfoSource


Why do we need the InfoSource information from interfaceStatus, isn't it obvious the InfoSource here is InfoSourceDomainAndGA?

Since the merge has one of 3 values:

domain InfoSourceDomain

~~guest-agent~~ (can't be in this specific line, because we know we are iterating the spec)

domain and guest-agent (InfoSourceDomainAndGA)

then there are two options left 1 and 3, not just InfoSourceDomainAndGA (option 3)

AlonaKaplan · 2021-12-30T10:20:22Z

pkg/network/setup/netstat.go

@@ -180,6 +185,7 @@ func (c *NetStat) UpdateStatus(vmi *v1.VirtualMachineInstance, domain *api.Domai
 				IP:            domainInterfaceStatus.Ip,
 				IPs:           domainInterfaceStatus.IPs,
 				InterfaceName: domainInterfaceStatus.InterfaceName,
+				InfoSource:    domainInterfaceStatus.InfoSource,


Same here, it is obvious the InfoSource is InfoSourceGuestAgent

Right, but delegated it to the one that calculate it atm (the merger), here we just copied the result (we can do it explicit if needed)

AlonaKaplan · 2021-12-30T10:21:38Z

pkg/virt-launcher/virtwrap/agent-poller/agent_parser.go

 	for i, interfaceStatus := range interfaceStatuses {
 		if alias, exists := aliasByMac[interfaceStatus.Mac]; exists {
 			interfaceStatuses[i].Name = alias
+			interfaceStatuses[i].InfoSource = netvmispec.InfoSourceDomainAndGA


I believe the InfoSource information here is redundant, please refer to the comments of netstat.go.

In order to move the whole logic to netstat.go
We need first this fix #6926
(which isn't enough by it's own, see example below)

I tried to start with it, and saw that it uncover the leakage in case the mac is changed
#6744 (last comment shows it)

As discussed also previously in this PR, there are couple place that are doing merges,
we can do an effort in the future to refactor them, meanwhile i prefer to keep the current logic please if possible, as it is more self contained in the current state of the code.

This change would be bit delicate, wider, and with some gotchas so I think it deserves a separate PR.
For example I tried to take #6926 + #6744 + this PR and check what happens:
We would still have the zombie ips as last comment of 6744 shows
We can see here that the ips are added because they appear in the pod, the merge code "protects"
and we would need to protect in a different method if we remove it (remove the code of 6744):

interfaces: - infoSource: domain ipAddress: 10.244.140.77 ipAddresses: - 10.244.140.77 - fd10:244::8c4c mac: "02:00:05:05:05:05" name: default - infoSource: guest-agent interfaceName: eth0 ipAddress: 10.244.140.77 ipAddresses: - 10.244.140.77 - fe80::5ff:fe05:505 mac: 02:00:b5:b5:b5:b5

Another reason is that Edy is already working on improving the updateStatus, and it's test coverage,
I think it will be safer and easier to refactor the virt-launcher merger and updateStatus once we have his PRs merged.

AlonaKaplan · 2021-12-30T10:54:59Z

pkg/network/setup/netstat.go

 				// Do not update if interface has Masquerede binding
 				// virt-controller should update VMI status interface with Pod IP instead
 				if !isForwardingBindingInterface {
 					newInterface.IP = interfaceStatus.Ip
 					newInterface.IPs = interfaceStatus.IPs
 				}
 				delete(domainInterfaceStatusByMac, interfaceMAC)
+			} else {


IIUC, currently this else is redundant since domainInterfaceStatusByMac represents both the guet agent + libvirt domain interfaces. So even interfaces the are not reported y the ga by reported by libvirt will be there.

it is true only if there is guest agent, if there isn't (or even if there is but until the first report of it) then we do need this else as the whole merge won't be called and we will need to set the infoSource ourself, because the interface won't be found on domainInterfaceStatusByMac

AlonaKaplan · 2021-12-30T10:55:38Z

pkg/network/setup/netstat_test.go

 			}},
 		}

 		netStat.UpdateStatus(vmi, domain)

 		Expect(vmi.Status.Interfaces).To(Equal([]v1.VirtualMachineInstanceNetworkInterface{
-			newVMIStatusIface(networkName, nil, ifaceMAC, guestIfaceName),
+			newVMIStatusIface(networkName, nil, ifaceMAC, guestIfaceName, netvmispec.InfoSourceDomainAndGA),


Please add a test with InfoSourceDomain.

see this please
https://github.com/kubevirt/kubevirt/pull/6793/files#diff-1c532caac99cc080ba1fb740c75c0ff712ca91509093f55cebdfaefee06ae887R287

same file line 287 if link doesnt open

EdDev · 2022-01-02T09:00:39Z

/sig network

AlonaKaplan · 2022-01-03T07:50:48Z

/approve

I"m waiting for a follow-up PR removing the non guest agent information from the merge as #6744 intended to do,
And specifying the info-source directly in netstat.go (status update) and not in two plaves (the metges and the status update).

kubevirt-bot · 2022-01-03T07:57:23Z

[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

oshoval · 2022-01-03T08:18:14Z

Thanks

CI has some problems fetching from quay / github, will retest soon

oshoval · 2022-01-03T08:44:48Z

/retest

still
docker: Error response from daemon: Get "https://quay.io/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers).

oshoval · 2022-01-03T09:26:55Z

/retest

oshoval · 2022-01-03T14:10:40Z

/retest

kubevirt-commenter-bot · 2022-01-03T21:15:16Z

/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.

Since two networks can use the same network attachement defintion, allow MultusNetwork to specify the network name in addition to the nad name. This way more than one network can be created using the same nad. Signed-off-by: Or Shoval <oshoval@redhat.com>

In case of MAC change in the guest, the vmi status will present two interfaces, one based on the domain spec, and one based on the guest agent (if it exists). The reason is that the MAC address is used in order to match the interface of the domain spec with the interfaces reported by the guest agent. Once the MAC is changed, this matching can't be performed. In order to make the info easier to understand, add a new field to each interface in vmi.status. infoSource field has 3 possible values: 1. domain - info is based on the domain spec 2. guest-agent - info is based on guest agent report 3. domain, guest-agent - info is based on both domain spec and guest agent report. Signed-off-by: Or Shoval <oshoval@redhat.com>

oshoval · 2022-01-04T07:50:08Z

rebased

oshoval · 2022-01-04T08:44:44Z

/test pull-kubevirt-check-tests-for-flakes

kubevirt-commenter-bot · 2022-01-04T14:16:18Z

/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.

oshoval · 2022-01-05T07:53:42Z

/test pull-kubevirt-e2e-kind-1.22-sriov

for some reason CI didnt run it for 18 hours
(and once i triggered it , it did)

kubevirt-commenter-bot · 2022-01-05T16:18:22Z

/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.

kubevirt-commenter-bot · 2022-01-05T19:18:21Z

/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.

oshoval · 2022-01-06T11:39:01Z

/cherry-pick release-0.49

kubevirt-bot · 2022-01-06T11:39:40Z

@oshoval: new pull request created: #7034

In response to this:

/cherry-pick release-0.49

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. labels Nov 14, 2021

kubevirt-bot requested review from jean-edouard and yuhaohaoyu November 14, 2021 15:44

kubevirt-bot added the size/M label Nov 14, 2021

kubevirt-bot removed request for jean-edouard and yuhaohaoyu November 14, 2021 15:44

oshoval force-pushed the info branch from 0f27bbb to e277e60 Compare November 15, 2021 08:01

oshoval force-pushed the info branch from e277e60 to a05ae16 Compare November 15, 2021 08:18

oshoval marked this pull request as draft November 15, 2021 12:46

oshoval force-pushed the info branch from a05ae16 to 7860e79 Compare November 15, 2021 12:52

oshoval mentioned this pull request Nov 15, 2021

guest_agent_information: Add info about vmi status infoSource kubevirt/user-guide#504

Merged

oshoval force-pushed the info branch from 7860e79 to 0a07f6e Compare November 15, 2021 16:10

kubevirt-bot added size/L and removed size/M labels Nov 15, 2021

oshoval marked this pull request as ready for review November 15, 2021 16:11

oshoval force-pushed the info branch 3 times, most recently from 75d5134 to 43a66d2 Compare November 16, 2021 07:55

oshoval changed the title ~~WIP vmi scheme, Add infoSource field to vmi status~~ virt-handler, Add infoSource field to vmi status Nov 16, 2021

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 16, 2021

kubevirt-bot requested a review from EdDev November 16, 2021 07:55

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

AlonaKaplan reviewed Dec 30, 2021

View reviewed changes

kubevirt-bot added the sig/network label Jan 2, 2022

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

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

oshoval added 2 commits January 4, 2022 09:20

oshoval force-pushed the info branch from 623fb34 to 530dde4 Compare January 4, 2022 07:49

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 4, 2022

EdDev approved these changes Jan 4, 2022

View reviewed changes

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

kubevirt-bot merged commit 87ef225 into kubevirt:main Jan 5, 2022

kubevirt-bot mentioned this pull request Jan 6, 2022

[release-0.49] virt-handler, Add infoSource field to vmi status #7034

Merged

virt-handler, Add infoSource field to vmi status #6793

virt-handler, Add infoSource field to vmi status #6793

Conversation

oshoval commented Nov 14, 2021 • edited Loading

oshoval commented Nov 14, 2021

oshoval commented Nov 15, 2021

oshoval commented Nov 15, 2021

oshoval commented Nov 16, 2021

maiqueb commented Nov 16, 2021

oshoval commented Nov 16, 2021 • edited Loading

maiqueb commented Nov 16, 2021 • edited Loading

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021

Choose a reason for hiding this comment

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

AlonaKaplan Dec 30, 2021

Choose a reason for hiding this comment

oshoval Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

EdDev commented Jan 2, 2022

AlonaKaplan commented Jan 3, 2022

kubevirt-bot commented Jan 3, 2022

oshoval commented Jan 3, 2022 • edited Loading

oshoval commented Jan 3, 2022 • edited Loading

oshoval commented Jan 3, 2022

oshoval commented Jan 3, 2022

kubevirt-commenter-bot commented Jan 3, 2022

oshoval commented Jan 4, 2022

oshoval commented Jan 4, 2022

kubevirt-commenter-bot commented Jan 4, 2022

oshoval commented Jan 5, 2022 • edited Loading

kubevirt-commenter-bot commented Jan 5, 2022

kubevirt-commenter-bot commented Jan 5, 2022

oshoval commented Jan 6, 2022

kubevirt-bot commented Jan 6, 2022

oshoval commented Nov 14, 2021 •

edited

Loading

oshoval commented Nov 16, 2021 •

edited

Loading

maiqueb commented Nov 16, 2021 •

edited

Loading

oshoval Dec 30, 2021 •

edited

Loading

oshoval Dec 30, 2021 •

edited

Loading

oshoval Dec 30, 2021 •

edited

Loading

oshoval Dec 30, 2021 •

edited

Loading

oshoval Dec 30, 2021 •

edited

Loading

oshoval commented Jan 3, 2022 •

edited

Loading

oshoval commented Jan 3, 2022 •

edited

Loading

oshoval commented Jan 5, 2022 •

edited

Loading