Expose serial number and bus type via GuestOsInfo #10970

alromeros · 2024-01-05T13:05:13Z

What this PR does / why we need it:

This PR updates the guestOsInfo command to show additional disk info, like serial number and bus type.

Example:

# virtctl guestosinfo testvmi-7j99w
selecting docker as container runtime
{
  "guestAgentVersion": "6.1.0",
  "supportedCommands": [
 ...
  ],
  "hostname": "testvmi-7j99w",
  "os": {
    "name": "Fedora Linux",
    "kernelRelease": "5.14.10-300.fc35.x86_64",
    "version": "35 (Cloud Edition)",
    "prettyName": "Fedora Linux 35 (Cloud Edition)",
    "versionId": "35",
    "kernelVersion": "#1 SMP Thu Oct 7 20:48:44 UTC 2021",
    "machine": "x86_64",
    "id": "fedora"
  },
  "timezone": "UTC, 0",
  "fsInfo": {
    "disks": [
      {
        "diskName": "vda3",
        "mountPoint": "/boot/efi",
        "fileSystemType": "vfat",
        "usedBytes": 10227712,
        "totalBytes": 104607744,
        "disk": [
          {
            "serial": "WD-WMAP9A966149",
            "bus-type": "virtio"
          }
        ]
      },
...

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://issues.redhat.com/browse/CNV-18671

Special notes for your reviewer:

Added the api exceptions so we can fetch the guest-agent information without unnecessary structs as intermediaries (json names need to match). The api additions are in that way (Disk being an array, for example) to match the guest-agent response.

Release note:

Expose fs disk information via GuestOsInfo

This commit updates the guestOsInfo command to show additional disk info, like serial number and bus type. Signed-off-by: Alvaro Romero <alromero@redhat.com>

kubevirt-bot · 2024-01-05T13:05:17Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2024-01-11T12:30:34Z

/test pull-kubevirt-e2e-k8s-1.28-sig-compute-migrations

mhenriks · 2024-01-18T00:06:02Z

/approve

kubevirt-bot · 2024-01-18T00:06:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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

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

enp0s3 · 2024-01-21T19:45:55Z

/lgtm

kubevirt-commenter-bot · 2024-01-21T19:46:01Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network

enp0s3 · 2024-01-21T19:59:50Z

/hold

@mhenriks @alromeros Sorry perhaps I am missing something, since you've added a non-primitive type to the api, don't you miss the piece of code that should prepare the slice as part of the parsing process?

enp0s3 · 2024-01-21T20:01:27Z

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

@@ -196,6 +197,7 @@ func parseFilesystem(agentReply string) ([]api.Filesystem, error) {
 			Type:       fs.Type,
 			TotalBytes: fs.TotalBytes,
 			UsedBytes:  fs.UsedBytes,
+			Disk:       fs.Disk,


Is it safe to just assign it like that w/o iterating and running append to the list you create with a make?

Yeah I think it's safe to just assign slices

alromeros · 2024-01-22T18:31:13Z

/hold

@mhenriks @alromeros Sorry perhaps I am missing something, since you've added a non-primitive type to the api, don't you miss the piece of code that should prepare the slice as part of the parsing process?

Hey @enp0s3, isn't that automatically handled by the generate scripts? What is exactly missing in this one?

enp0s3 · 2024-01-22T20:11:35Z

@alromeros I took GetUsers() as a reference. The code iterates over the data it got from l.agentData.GetUsers(-1) , so how this is different from your case where you receive the fs.disk as a list?

Should you have a function l.agentData.GetDisks(fs) or something similar?

enp0s3 · 2024-01-22T20:16:28Z

@alromeros I think I am confused because of the definition []v1.VirtualMachineInstanceFileSystemDisk, why did you defined it as a slice? do you expect to receive a list of disks from the AgentData in fs.Disk?

alromeros · 2024-01-23T08:52:31Z

@alromeros I took GetUsers() as a reference. The code iterates over the data it got from l.agentData.GetUsers(-1) , so how this is different from your case where you receive the fs.disk as a list?

Should you have a function l.agentData.GetDisks(fs) or something similar?

Right, I get what you mean. Following your example, GetUsers uses an intermediary struct on the agent-poller side to store the guest agent response. Then, it copies each element of the api.User type to the v1.VirtualMachineInstanceGuestOSUser type here.

We have something similar in GetFilesystems (the command I'm updating). We fetch the whole guest agent response here. Since the new field I added (disks) has the same JSON formatting as the disks field in the guest agent response, there's no need for an intermediary struct and we can just assign it without needing to iterate.

Maybe I'm missing something, but I don't see any good reasons for this intermediary step when we can just adjust the type (v1.VirtualMachineInstanceFileSystemDisk in this case) to match the formatting of the GA response.

alromeros · 2024-01-23T08:53:36Z

@alromeros I think I am confused because of the definition []v1.VirtualMachineInstanceFileSystemDisk, why did you defined it as a slice? do you expect to receive a list of disks from the AgentData in fs.Disk?

Right, I'm just mirroring the guest agent's response (an array of disk).

enp0s3 · 2024-01-23T09:39:30Z

/unhold

@alromeros Thank you for the explanation! So the json.Unmarshal([]byte(response), &result) already allocates the slice.

alromeros · 2024-01-23T11:30:09Z

/retest-required

xpivarc

@enp0s3 this PR doesn't add a new test nor does it extend the existing functional test.

xpivarc · 2024-01-24T11:11:39Z

staging/src/kubevirt.io/api/core/v1/types.go

@@ -2215,13 +2215,20 @@ type VirtualMachineInstanceFileSystemList struct {
 	Items           []VirtualMachineInstanceFileSystem `json:"items"`
 }

+// VirtualMachineInstanceFileSystemDisk represents the guest os FS disks
+type VirtualMachineInstanceFileSystemDisk struct {
+	Serial  string `json:"serial"`


Missing omitempty

@alromeros is this ever expected to be empty? I would be pretty surprised

I think BusType will never be empty, but Serial is optional: https://qemu-project.gitlab.io/qemu/interop/qemu-ga-ref.html#qapidoc-146.

xpivarc · 2024-01-24T11:12:15Z

staging/src/kubevirt.io/api/core/v1/types.go

+// VirtualMachineInstanceFileSystemDisk represents the guest os FS disks
+type VirtualMachineInstanceFileSystemDisk struct {
+	Serial  string `json:"serial"`
+	BusType string `json:"bus-type"`


This does break convention, it should be json:"busType".

@xpivarc Thanks for the heads up! this check must be automated somehow, it is something hard to remember for the long term.

we can make the make generate fail

make generate failed but I added an exception to allow this formatting so we don't need an intermediary type:

API rule violation: names_match,kubevirt.io/api/core/v1,VirtualMachineInstanceFileSystemDisk,BusType

I will fix it to avoid breaking the convention.

I assumed we wanted to expose with exact naming naming scheme as libvirt

Yeah me too. It prevents adding some unnecessary code, but I guess it's odd to only add the exception to BusType.

xpivarc · 2024-01-24T11:13:07Z

staging/src/kubevirt.io/api/core/v1/types.go

+	FileSystemType string                                 `json:"fileSystemType"`
+	UsedBytes      int                                    `json:"usedBytes"`
+	TotalBytes     int                                    `json:"totalBytes"`
+	Disk           []VirtualMachineInstanceFileSystemDisk `json:"disk,omitempty"`


Why is this a slice?

I got the explanation here

That doesn't explain the question, @alromeros can you elaborate?

We are storing here the guest agent response to the guest-get-fsinfo command. The command returns an array of disks, so I'm using one: https://qemu-project.gitlab.io/qemu/interop/qemu-ga-ref.html#qapidoc-161.

enp0s3 · 2024-01-24T11:23:32Z

@enp0s3 this PR doesn't add a new test nor does it extend the existing functional test.

@xpivarc TBH I felt uncomfortable with that as well but I had a doubt whether functional coverage fits here, or unit is enough.

xpivarc · 2024-01-24T11:32:27Z

@enp0s3 this PR doesn't add a new test nor does it extend the existing functional test.

@xpivarc TBH I felt uncomfortable with that as well but I had a doubt whether functional coverage fits here, or unit is enough.

@enp0s3
There is an existing functional test, all that is needed to include check that the proper field is populated. Zero cost.

enp0s3 · 2024-01-24T11:35:43Z

@enp0s3 this PR doesn't add a new test nor does it extend the existing functional test.

@xpivarc TBH I felt uncomfortable with that as well but I had a doubt whether functional coverage fits here, or unit is enough.

@enp0s3 There is an existing functional test, all that is needed to include check that the proper field is populated. Zero cost.

@xpivarc +1
@alromeros Can you please create a follow-up PR for that?

alromeros · 2024-01-24T13:40:36Z

@enp0s3 this PR doesn't add a new test nor does it extend the existing functional test.

@xpivarc TBH I felt uncomfortable with that as well but I had a doubt whether functional coverage fits here, or unit is enough.

@enp0s3 There is an existing functional test, all that is needed to include check that the proper field is populated. Zero cost.

Didn't see individual checks for most of the fields in fsList but I can add them in a follow-up. Will open it later once we decide if the API is good.

Follow-up #10970: Minor API fixes to get fsList disks

…ase-1.2 [release-1.2] Follow-up #10970: Minor API fixes to get fsList disks

Expose serial number and bus type via virtctl

7e407cd

This commit updates the guestOsInfo command to show additional disk info, like serial number and bus type. Signed-off-by: Alvaro Romero <alromero@redhat.com>

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2024

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 5, 2024

kubevirt-bot requested review from stu-gott and xpivarc January 5, 2024 13:05

kubevirt-bot added size/L kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jan 5, 2024

Add unit tests

693df1e

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros marked this pull request as ready for review January 10, 2024 18:40

kubevirt-bot requested review from alicefr and enp0s3 January 10, 2024 18:40

alromeros changed the title ~~[WIP] Expose serial number and bus type via GuestOsInfo~~ Expose serial number and bus type via GuestOsInfo Jan 15, 2024

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

kubevirt-bot assigned enp0s3 Jan 21, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 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 21, 2024

enp0s3 reviewed Jan 21, 2024

View reviewed changes

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

kubevirt-bot merged commit 80e35d3 into kubevirt:main Jan 24, 2024
42 checks passed

xpivarc reviewed Jan 24, 2024

View reviewed changes

alromeros mentioned this pull request Jan 29, 2024

Follow-up #10970: Minor API fixes to get fsList disks #11099

Merged

kubevirt-bot added a commit that referenced this pull request Feb 22, 2024

Merge pull request #11099 from alromeros/update-guestosinfo-followup

ef0cc63

Follow-up #10970: Minor API fixes to get fsList disks

kubevirt-bot added a commit that referenced this pull request Feb 26, 2024

Merge pull request #11349 from kubevirt-bot/cherry-pick-11099-to-rele…

f26e45d

…ase-1.2 [release-1.2] Follow-up #10970: Minor API fixes to get fsList disks

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose serial number and bus type via GuestOsInfo #10970

Expose serial number and bus type via GuestOsInfo #10970

alromeros commented Jan 5, 2024 •

edited

Loading

kubevirt-bot commented Jan 5, 2024

alromeros commented Jan 11, 2024

mhenriks commented Jan 18, 2024

kubevirt-bot commented Jan 18, 2024

enp0s3 commented Jan 21, 2024

kubevirt-commenter-bot commented Jan 21, 2024

enp0s3 commented Jan 21, 2024

enp0s3 Jan 21, 2024 •

edited

Loading

alromeros Jan 22, 2024

alromeros commented Jan 22, 2024

enp0s3 commented Jan 22, 2024

enp0s3 commented Jan 22, 2024

alromeros commented Jan 23, 2024 •

edited

Loading

alromeros commented Jan 23, 2024

enp0s3 commented Jan 23, 2024

alromeros commented Jan 23, 2024

xpivarc left a comment

xpivarc Jan 24, 2024

mhenriks Jan 24, 2024

alromeros Jan 24, 2024

xpivarc Jan 24, 2024

enp0s3 Jan 24, 2024

xpivarc Jan 24, 2024

alromeros Jan 24, 2024

mhenriks Jan 24, 2024

alromeros Jan 24, 2024

xpivarc Jan 24, 2024

enp0s3 Jan 24, 2024

xpivarc Jan 24, 2024

alromeros Jan 24, 2024

enp0s3 commented Jan 24, 2024

xpivarc commented Jan 24, 2024

enp0s3 commented Jan 24, 2024

alromeros commented Jan 24, 2024

Expose serial number and bus type via GuestOsInfo #10970

Expose serial number and bus type via GuestOsInfo #10970

Conversation

alromeros commented Jan 5, 2024 • edited Loading

kubevirt-bot commented Jan 5, 2024

alromeros commented Jan 11, 2024

mhenriks commented Jan 18, 2024

kubevirt-bot commented Jan 18, 2024

enp0s3 commented Jan 21, 2024

kubevirt-commenter-bot commented Jan 21, 2024

enp0s3 commented Jan 21, 2024

enp0s3 Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros commented Jan 22, 2024

enp0s3 commented Jan 22, 2024

enp0s3 commented Jan 22, 2024

alromeros commented Jan 23, 2024 • edited Loading

alromeros commented Jan 23, 2024

enp0s3 commented Jan 23, 2024

alromeros commented Jan 23, 2024

xpivarc 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

enp0s3 commented Jan 24, 2024

xpivarc commented Jan 24, 2024

enp0s3 commented Jan 24, 2024

alromeros commented Jan 24, 2024

alromeros commented Jan 5, 2024 •

edited

Loading

enp0s3 Jan 21, 2024 •

edited

Loading

alromeros commented Jan 23, 2024 •

edited

Loading