Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report list of devices in the REST API #99

Merged
merged 10 commits into from
Feb 21, 2024

Conversation

cfergeau
Copy link
Collaborator

The aim of this PR was to add the list of devices to the /inspect endpoint with their parameters.
This supersedes #73

My initial goal was to only add tests for this code.
Along the way I found 2 related issues (memory byte/MiB confusion, and too long unix socket paths) which I fixed, and I also realized the code from #73 could be made simpler: config.VirtualMachine already can be serialized to/from json, so we don't need any new code.

@openshift-ci openshift-ci bot requested review from anjannath and baude February 13, 2024 14:26
@cfergeau cfergeau force-pushed the improve-inspect branch 6 times, most recently from 0a85eb4 to 7fad3ea Compare February 16, 2024 14:31
pkg/config/json_test.go Outdated Show resolved Hide resolved
`NewVirtualMachine` has a `memoryBytes` arguments, but its users (crc and podman), and
vfkit code expects it to be a mibibyte value, not a byte value.
Since crc and podman are using correctly, this commit only renames the
`memoryBytes` argument to `memoryMiB`, and converts it to bytes
internally.

Signed-off-by: Christophe Fergeau <[email protected]>
To avoid the bytes/mibibytes confusion fixed in the previous commit, we
can use github.com/containers/common/pkg/strongunits to attach an
explicit unit to the memory size.

Signed-off-by: Christophe Fergeau <[email protected]>
All fields now start with a lowercase letter
This is a breaking change if you used the json output for serialization
purpose.

This is based on a patch from Kevin Cui <[email protected]>

Signed-off-by: Christophe Fergeau <[email protected]>
Instead of marshalling it to a not useful base64-encoded []byte, marshal
it to its human-readable string representation.

Signed-off-by: Christophe Fergeau <[email protected]>
This currently is only a stub with hardcoded values.
This commit adds a reference to
config.VirtualMachine to VzVirtualMachine. We can then serialize it to
JSON as there is already code for this.

This changes the Memory field to MemoryBytes, but since it contained a
dummy value so far, this should not be a big problem

This is based on a patch from Kevin Cui <[email protected]>

Signed-off-by: Christophe Fergeau <[email protected]>
This reuses the PCIID tests to check that the configuration returned
through the REST API matches how the VM was configured.

Signed-off-by: Christophe Fergeau <[email protected]>
UNIX socket paths have a 104 bytes upper limit which is not so hard to
trigger with the temporary directories created during testing:
`/var/folders/l0/rh4v8_j54k37h2w320__7d1h0000gn/T/TestVsockConnect2047157905/001/`

Better to detect this early and return an explicit error when this
happens.

This also modifies the REST API test, it shortens one of the test names
and the rest socket name as these were triggering this problem.

Signed-off-by: Christophe Fergeau <[email protected]>
[GIN-debug] [WARNING] Running in debug mode. Switch to release mode in production.
 - using env:	export GIN_MODE=release
 - using code:	gin.SetMode(gin.ReleaseMode)

Signed-off-by: Christophe Fergeau <[email protected]>
initrd and kernel cmdline were swapped. This is only a bug in
json_test.go, not some actual bug/confusion in the rest of vfkit

Signed-off-by: Christophe Fergeau <[email protected]>
Copy link
Contributor

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

openshift-ci bot commented Feb 21, 2024

@BlackHole1: changing LGTM is restricted to collaborators

In response to this:

LGTM

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.

@cfergeau
Copy link
Collaborator Author

This can be tested by starting vfkit with --restful-uri unix:///Users/teuf/dev/vfkit/rest.sock and then running curl --unix-socket /Users/teuf/dev/vfkit/rest.sock http://unused/vm/inspect

@praveenkumar
Copy link
Member

Tested with following and works as expected

✗ ./out/vfkit \
    --cpus 2 --memory 2048 \
    --bootloader efi,variable-store=efi-variable-store,create \
    --device virtio-blk,path=vfkit-test-image.raw \
>   --restful-uri unix:///Users/prkumar/dev/vfkit/rest.sock
INFO[0000] &{2 2048    {[efi variable-store=efi-variable-store create] true}  [virtio-blk,path=vfkit-test-image.raw] unix:///Users/prkumar/dev/vfkit/rest.sock  false} 
INFO[0000] boot parameters: &{EFIVariableStorePath:efi-variable-store CreateVariableStore:true} 
INFO[0000]                                              
INFO[0000] virtual machine parameters:                  
INFO[0000] 	vCPUs: 2                                    
INFO[0000] 	memory: 2048 MiB                            
INFO[0000]                                              
INFO[0000] Adding virtio-blk device (imagePath: vfkit-test-image.raw) 
INFO[0000] virtual machine is running                   
INFO[0000] waiting for VM to stop    

and api response

✗ curl --unix-socket /Users/prkumar/dev/vfkit/rest.sock http://unused/vm/inspect | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   236  100   236    0     0   524k      0 --:--:-- --:--:-- --:--:--  230k
{
  "vcpus": 2,
  "memoryBytes": 2147483648,
  "bootloader": {
    "kind": "efiBootloader",
    "efiVariableStorePath": "efi-variable-store",
    "createVariableStore": true
  },
  "devices": [
    {
      "kind": "virtioblk",
      "devName": "virtio-blk",
      "imagePath": "vfkit-test-image.raw"
    }
  ]
}

/lgtm

@praveenkumar
Copy link
Member

/approve
/lgtm

Copy link

openshift-ci bot commented Feb 21, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BlackHole1, praveenkumar
Once this PR has been reviewed and has the lgtm label, please ask for approval from cfergeau. 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:

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

@praveenkumar praveenkumar merged commit d372c19 into crc-org:main Feb 21, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants