refactor(rest): improve inspect api #73

BlackHole1 · 2023-12-26T08:31:42Z

Add the correct cpus, memory, devices

Test Code:

package main

import (
	"os"
	"sync"

	"github.com/crc-org/vfkit/pkg/config"
	"github.com/crc-org/vfkit/pkg/rest/define"
	"github.com/gin-gonic/gin"
)

var (
	once            sync.Once
	devicesResponse define.DevicesResponse
)

func devicesToResp(devices []config.VirtioDevice) define.DevicesResponse {
	once.Do(func() {
		for _, dev := range devices {
			switch d := dev.(type) {
			case *config.USBMassStorage:
				devicesResponse.USBMassStorage = append(devicesResponse.USBMassStorage, *d)
			case *config.VirtioBlk:
				devicesResponse.Blk = append(devicesResponse.Blk, *d)
			case *config.RosettaShare:
				devicesResponse.Rosetta = *d
			case *config.NVMExpressController:
				devicesResponse.NVMe = append(devicesResponse.NVMe, *d)
			case *config.VirtioFs:
				devicesResponse.FS = append(devicesResponse.FS, *d)
			case *config.VirtioNet:
				n := define.VirtioNetResponse{
					Nat:            d.Nat,
					MacAddress:     d.MacAddress.String(),
					UnixSocketPath: d.UnixSocketPath,
				}

				if d.Socket != nil {
					n.Fd = int(d.Socket.Fd())
				}

				devicesResponse.Net = append(devicesResponse.Net, n)
			case *config.VirtioRng:
				devicesResponse.Rng = true
			case *config.VirtioSerial:
				devicesResponse.Serial = *d
			case *config.VirtioVsock:
				devicesResponse.Vsock = append(devicesResponse.Vsock, *d)
			case *config.VirtioInput:
				devicesResponse.Input = append(devicesResponse.Input, *d)
			case *config.VirtioGPU:
				devicesResponse.GPU = append(devicesResponse.GPU, *d)
			}
		}
	})

	return devicesResponse
}

func Inspect(devices []config.VirtioDevice) define.InspectResponse {
	ii := define.InspectResponse{
		Devices: devicesToResp(devices),
	}
	return ii
}

func main() {
	eng := gin.Default()
	eng.GET("/inspect", func(c *gin.Context) {
		var devs []config.VirtioDevice

		usb, _ := func() (config.VirtioDevice, error) { return config.USBMassStorageNew("/usb1") }()
		usb2, _ := func() (config.VirtioDevice, error) { return config.USBMassStorageNew("/usb2") }()
		blk, _ := func() (config.VirtioDevice, error) { return config.VirtioBlkNew("/a") }()
		blk2 := &config.VirtioBlk{
			StorageConfig: config.StorageConfig{
				ImagePath: "/b",
				DevName:   "virtio-blk",
			},
			DeviceIdentifier: "/vdd",
		}
		rosetta, _ := func() (config.VirtioDevice, error) { return config.RosettaShareNew("/rosetta") }()
		nvme, _ := func() (config.VirtioDevice, error) { return config.NVMExpressControllerNew("/nvme1") }()
		nvme2, _ := func() (config.VirtioDevice, error) { return config.NVMExpressControllerNew("/nvme2") }()
		fs, _ := func() (config.VirtioDevice, error) { return config.VirtioFsNew("/fs1", "") }()
		fs2, _ := func() (config.VirtioDevice, error) { return config.VirtioFsNew("/fs2", "foo") }()
		net, _ := func() (config.VirtioDevice, error) { return config.VirtioNetNew("4a:f7:a9:77:38:d8") }()
		net2 := &config.VirtioNet{
			UnixSocketPath: "/tmp/unix.sock",
		}
		f, _ := os.CreateTemp(os.TempDir(), "tmp")
		net3 := &config.VirtioNet{
			Socket:     f,
			MacAddress: []byte{0x00, 0x11, 0x22, 0x33, 0x44, 0x55},
		}
		rng, _ := func() (config.VirtioDevice, error) { return config.VirtioRngNew() }()
		serial, _ := func() (config.VirtioDevice, error) { return config.VirtioSerialNew("/serial") }()
		vsock, _ := func() (config.VirtioDevice, error) { return config.VirtioVsockNew(1234, "/vsock", false) }()
		vsock2, _ := func() (config.VirtioDevice, error) { return config.VirtioVsockNew(1235, "/vsock2", true) }()
		input, _ := func() (config.VirtioDevice, error) { return config.VirtioInputNew(config.VirtioInputPointingDevice) }()
		input2, _ := func() (config.VirtioDevice, error) { return config.VirtioInputNew(config.VirtioInputKeyboardDevice) }()
		gpu, _ := func() (config.VirtioDevice, error) { return config.VirtioGPUNew() }()
		gpu2 := &config.VirtioGPU{
			UsesGUI: true,
			VirtioGPUResolution: config.VirtioGPUResolution{
				Width:  100,
				Height: 200,
			},
		}

		devs = append(devs,
			usb,
			usb2,
			blk,
			blk2,
			rosetta,
			nvme,
			nvme2,
			fs,
			fs2,
			net,
			net2,
			net3,
			rng,
			serial,
			vsock,
			vsock2,
			input,
			input2,
			gpu,
			gpu2,
		)

		x := Inspect(devs)
		c.JSON(200, x)
	})

	eng.Run(":7777")
}

Result:

curl http://127.0.0.1:7777/inspect

{
  "cpus": 0,
  "memory": 0,
  "devices": {
    "input": [
      {
        "kind": "virtioinput",
        "inputType": "pointing"
      },
      {
        "kind": "virtioinput",
        "inputType": "keyboard"
      }
    ],
    "gpu": [
      {
        "kind": "virtiogpu",
        "usesGUI": false,
        "width": 800,
        "height": 600
      },
      {
        "kind": "virtiogpu",
        "usesGUI": true,
        "width": 100,
        "height": 200
      }
    ],
    "vsock": [
      {
        "kind": "virtiosock",
        "port": 1234,
        "socketURL": "/vsock",
        "listen": false
      },
      {
        "kind": "virtiosock",
        "port": 1235,
        "socketURL": "/vsock2",
        "listen": true
      }
    ],
    "blk": [
      {
        "kind": "virtioblk",
        "devName": "virtio-blk",
        "imagePath": "/a",
        "readOnly": false,
        "deviceIdentifier": ""
      },
      {
        "kind": "virtioblk",
        "devName": "virtio-blk",
        "imagePath": "/b",
        "readOnly": false,
        "deviceIdentifier": "/vdd"
      }
    ],
    "fs": [
      {
        "kind": "virtiofs",
        "mountTag": "",
        "sharedDir": "/fs1"
      },
      {
        "kind": "virtiofs",
        "mountTag": "foo",
        "sharedDir": "/fs2"
      }
    ],
    "rosetta": {
      "mountTag": "/rosetta",
      "installRosetta": false
    },
    "nvme": [
      {
        "kind": "nvme",
        "devName": "nvme",
        "imagePath": "/nvme1",
        "readOnly": false
      },
      {
        "kind": "nvme",
        "devName": "nvme",
        "imagePath": "/nvme2",
        "readOnly": false
      }
    ],
    "net": [
      {
        "nat": true,
        "macAddress": "4a:f7:a9:77:38:d8",
        "unixSocketPath": "",
        "fd": 0
      },
      {
        "nat": false,
        "macAddress": "",
        "unixSocketPath": "/tmp/unix.sock",
        "fd": 0
      },
      {
        "nat": false,
        "macAddress": "00:11:22:33:44:55",
        "unixSocketPath": "",
        "fd": 9
      }
    ],
    "rng": true,
    "serial": {
      "logFile": "/serial",
      "usesStdio": false
    },
    "usbMassStorage": [
      {
        "kind": "usbmassstorage",
        "devName": "usb-mass-storage",
        "imagePath": "/usb1",
        "readOnly": false
      },
      {
        "kind": "usbmassstorage",
        "devName": "usb-mass-storage",
        "imagePath": "/usb2",
        "readOnly": false
      }
    ]
  }
}

openshift-ci · 2023-12-26T08:31:51Z

[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 assign cfergeau for approval. 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

openshift-ci · 2023-12-26T08:31:55Z

Hi @BlackHole1. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

gbraad · 2023-12-27T06:55:40Z

/ok-to-test

cfergeau

Looks fine if you remove this @Black-Hole1 comment.

pkg/rest/vf/vm_config.go

Add the correct cpus, memory, devices Signed-off-by: Kevin Cui <[email protected]>

cfergeau · 2024-02-06T11:29:45Z

Test Code:

For what it's worth, if you want to add unit tests for this new code, it's now easier using https://github.com/crc-org/vfkit/blob/main/test/vm_test.go ,this makes it easy to create a VM with a set of devices, start vfkit, run some tests (eg make http request to the REST API), and check that the result is the expected one.

cfergeau

This PR is going to be helpful to #48 to provide a way to let users of vfkit know which PTY was allocated :)

pkg/config/json_test.go

cfergeau · 2024-02-06T11:37:13Z

pkg/rest/define/config.go

+	Nat            bool   `json:"nat"`
+	MacAddress     string `json:"macAddress"`
+	UnixSocketPath string `json:"unixSocketPath"`
+	Fd             int    `json:"fd"`


Fwiw, I'm not sure fd is going to be useful to expose to an external process?

cfergeau · 2024-02-06T11:39:54Z

pkg/rest/define/config.go

+	Net            []VirtioNetResponse           `json:"net"`
+	Rng            bool                          `json:"rng"`
+	Serial         config.VirtioSerial           `json:"serial"`
+	USBMassStorage []config.USBMassStorage       `json:"usbMassStorage"`


Can we reuse the same format as the preexisting json serialization code?

"devices":[{"devName":"virtio-blk","imagePath":"/virtioblk1","readOnly":false,"deviceIdentifier":""}]}

serialization/deserialization to this format should already be working, a new type should not be needed.
Or do you have strong reasons for having such a struct?

#99 reuses the existing json format

cfergeau · 2024-02-07T16:41:29Z

Test Code:

For what it's worth, if you want to add unit tests for this new code, it's now easier using https://github.com/crc-org/vfkit/blob/main/test/vm_test.go ,this makes it easy to create a VM with a set of devices, start vfkit, run some tests (eg make http request to the REST API), and check that the result is the expected one.

I've started doing this in cfergeau@4615740 but not finished yet :)

cfergeau · 2024-02-13T14:29:46Z

I've started doing this in cfergeau@4615740 but not finished yet :)

I'm now done with the testing code/..., I've filed #99 , in my opinion we can move the rest of the discussions in this new PR.

BlackHole1 · 2024-02-14T00:55:52Z

Hey @cfergeau, because it's Chinese New Year now, I'm currently on vacation and will be back in about a week.

cfergeau · 2024-02-14T13:06:54Z

Hey @cfergeau, because it's Chinese New Year now, I'm currently on vacation and will be back in about a week.

Thanks for the message, enjoy the holidays!

BlackHole1 · 2024-02-18T03:16:38Z

I'm back :)

I'm now done with the testing code/..., I've filed #99 , in my opinion we can move the rest of the discussions in this new PR.

Does it look like I can close this PR?

cfergeau · 2024-02-20T09:28:39Z

I'm back :)

I'm now done with the testing code/..., I've filed #99 , in my opinion we can move the rest of the discussions in this new PR.

Does it look like I can close this PR?

If the json format proposed in #99 for the inspect API works for you, then yes we can close this PR.

BlackHole1 · 2024-02-21T01:49:25Z

If the json format proposed in #99 for the inspect API works for you

As for me, the new JSON format will make Go developers comfortable, but I cannot be certain about developers in other languages because they may not be able to determine the result of the response by inspecting the source code. Perhaps we still need to improve the relevant documentation?
I am also concerned that the new format may introduce more subtle breaking changes. For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API, it could lead to breaking changes inadvertently.

cfergeau · 2024-02-21T08:49:53Z

Perhaps we still need to improve the relevant documentation?

Ah yes, for sure, I did not even think about documenting this yet :-/

For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API, it could lead to breaking changes inadvertently.

pkg/config/json_test.go should catch many of these breaking changes, but it's not exhaustive, we could extend it to make sure it covers all the fields that are serialized to json. This way json breaking changes will be noticeable.

cfergeau · 2024-02-22T13:04:43Z

Perhaps we still need to improve the relevant documentation?

Ah yes, for sure, I did not even think about documenting this yet :-/

For example, if I modify or delete a key but am unaware that this key is reflected in the inspect API, it could lead to breaking changes inadvertently.

pkg/config/json_test.go should catch many of these breaking changes, but it's not exhaustive, we could extend it to make sure it covers all the fields that are serialized to json. This way json breaking changes will be noticeable.

I tried to do this in #103

openshift-ci bot added the do-not-merge/work-in-progress label Dec 26, 2023

openshift-ci bot added the needs-ok-to-test label Dec 26, 2023

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Dec 27, 2023

cfergeau reviewed Jan 29, 2024

View reviewed changes

pkg/rest/vf/vm_config.go Show resolved Hide resolved

pkg/rest/vf/vm_config.go Outdated Show resolved Hide resolved

BlackHole1 force-pushed the improve-inspect branch from 65f2918 to c131dd4 Compare January 30, 2024 08:50

BlackHole1 marked this pull request as ready for review January 30, 2024 08:50

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 30, 2024

openshift-ci bot requested review from baude and praveenkumar January 30, 2024 08:50

refactor(rest): improve inspect api

5962c06

Add the correct cpus, memory, devices Signed-off-by: Kevin Cui <[email protected]>

BlackHole1 force-pushed the improve-inspect branch from c131dd4 to 5962c06 Compare January 31, 2024 08:12

cfergeau reviewed Feb 6, 2024

View reviewed changes

cfergeau mentioned this pull request Feb 13, 2024

Report list of devices in the REST API #99

Merged

BlackHole1 closed this Feb 22, 2024

BlackHole1 deleted the improve-inspect branch February 22, 2024 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(rest): improve inspect api #73

refactor(rest): improve inspect api #73

BlackHole1 commented Dec 26, 2023 •

edited

Loading

openshift-ci bot commented Dec 26, 2023

openshift-ci bot commented Dec 26, 2023

gbraad commented Dec 27, 2023

cfergeau left a comment

cfergeau commented Feb 6, 2024

cfergeau left a comment

cfergeau Feb 6, 2024

cfergeau Feb 6, 2024

cfergeau Feb 13, 2024

cfergeau commented Feb 7, 2024

cfergeau commented Feb 13, 2024

BlackHole1 commented Feb 14, 2024

cfergeau commented Feb 14, 2024

BlackHole1 commented Feb 18, 2024

cfergeau commented Feb 20, 2024

BlackHole1 commented Feb 21, 2024

cfergeau commented Feb 21, 2024

cfergeau commented Feb 22, 2024

refactor(rest): improve inspect api #73

refactor(rest): improve inspect api #73

Conversation

BlackHole1 commented Dec 26, 2023 • edited Loading

openshift-ci bot commented Dec 26, 2023

openshift-ci bot commented Dec 26, 2023

gbraad commented Dec 27, 2023

cfergeau left a comment

Choose a reason for hiding this comment

cfergeau commented Feb 6, 2024

cfergeau left a comment

Choose a reason for hiding this comment

cfergeau Feb 6, 2024

Choose a reason for hiding this comment

cfergeau Feb 6, 2024

Choose a reason for hiding this comment

cfergeau Feb 13, 2024

Choose a reason for hiding this comment

cfergeau commented Feb 7, 2024

cfergeau commented Feb 13, 2024

BlackHole1 commented Feb 14, 2024

cfergeau commented Feb 14, 2024

BlackHole1 commented Feb 18, 2024

cfergeau commented Feb 20, 2024

BlackHole1 commented Feb 21, 2024

cfergeau commented Feb 21, 2024

cfergeau commented Feb 22, 2024

BlackHole1 commented Dec 26, 2023 •

edited

Loading