Implement describe instance #116
Conversation
Signed-off-by: Maksym Pavlenko <[email protected]>
| @@ -672,6 +672,18 @@ func (m *Machine) UpdateGuestDrive(ctx context.Context, driveID, pathOnHost stri | |||
| return nil | |||
| } | |||
|
|
|||
| // DescribeInstance returns general information about an instance. | |||
| func (m *Machine) DescribeInstance(ctx context.Context) (*models.InstanceInfo, error) { | |||
There was a problem hiding this comment.
It looks like this is the first case where we're just returning a struct from models instead of doing something else to the return value to make it more useful. I'm worried that establishing a pattern where we wrap the Swagger client and provide little additional logic will be both hard for us to maintain and not super valuable.
Instead of doing this, it might be better to expose easier access to the underlying Swagger client for any operations that we haven't built a higher-level interface around.
func (m *Machine) APIClient() Client {
return m.client
}What do you think?
There was a problem hiding this comment.
I'm totally fine with exposing APIClient as it would simplify support existing/future firecracker API features. However there are a few things that bother me:
- We already wrap other things, like
UpdateGuestDrive,SetMetadata,UpdateGuestNetworkInterfaceRateLimit, etc. Essentially these methods just passthrough parameters to API client. As we want to be backward compatible, we would allow to do same thing in two different ways. - We exposed
MachineIfaceinterface as part of SDK which is handy for testing (when we don't want to run an actual Firecracker instance, but just a few mock calls). As firecracker client is auto generated with no interface, this might lead to some complexities with testing.
There was a problem hiding this comment.
I was thinking about this yesterday and think this is the best path going forward, a helper method to retrieve the API client.
There was a problem hiding this comment.
After some discussion offline, I'll revisit this in a separate PR.
|
I see that this work was scrapped in favor of providing a getter method for the inner client, I would be willing to implement if the change isn't already in progress. I'm looking for a way to run a health check against a running VM and know that it is in the running state. Are there any existing solutions? |
Set the agent shim as a subreaper to ensure containers become its children
Signed-off-by: Maksym Pavlenko [email protected]
Issue #, if available:
#115
firecracker-microvm/firecracker-containerd#251
Description of changes:
This PR adds
DescribeInstanceto sdkcc: @haikuoliu
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.