Skip to content

Implement describe instance #116

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Implement describe instance #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions firecracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,3 +273,12 @@ func (f *Client) PatchGuestDriveByID(ctx context.Context, driveID, pathOnHost st

return f.client.Operations.PatchGuestDriveByID(params)
}

// DescribeInstance is a wrapper for the swagger generated client to make
// calling of the API easier.
func (f *Client) DescribeInstance(ctx context.Context) (*ops.DescribeInstanceOK, error) {
params := ops.NewDescribeInstanceParams()
params.SetContext(ctx)

return f.client.Operations.DescribeInstance(params)
}
4 changes: 4 additions & 0 deletions firecracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,8 @@ func TestClient(t *testing.T) {
if _, err := client.PutGuestDriveByID(ctx, "test", drive); err != nil {
t.Errorf("unexpected error on PutGuestDriveByID, %v", err)
}

if _, err := client.DescribeInstance(ctx); err != nil {
t.Errorf("unexpected error on DescribeInstance, %v", err)
}
}
12 changes: 12 additions & 0 deletions machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. We exposed MachineIface interface 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this yesterday and think this is the best path going forward, a helper method to retrieve the API client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some discussion offline, I'll revisit this in a separate PR.

instance, err := m.client.DescribeInstance(ctx)
if err != nil {
m.logger.WithError(err).Error("Failed to describe instance")
return nil, err
}

m.logger.Printf("DescribeInstance successful")
return instance.Payload, nil
}

// refreshMachineConfiguration synchronizes our cached representation of the machine configuration
// with that reported by the Firecracker API
func (m *Machine) refreshMachineConfiguration() error {
Expand Down
24 changes: 24 additions & 0 deletions machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ func TestMicroVMExecution(t *testing.T) {
t.Run("TestAttachSecondaryDrive", func(t *testing.T) { testAttachSecondaryDrive(ctx, t, m) })
t.Run("TestAttachVsock", func(t *testing.T) { testAttachVsock(ctx, t, m) })
t.Run("SetMetadata", func(t *testing.T) { testSetMetadata(ctx, t, m) })
t.Run("DescribeInstance", func(t *testing.T) { testDescribeInstance(ctx, t, m) })
t.Run("TestUpdateGuestDrive", func(t *testing.T) { testUpdateGuestDrive(ctx, t, m) })
t.Run("TestUpdateGuestNetworkInterface", func(t *testing.T) { testUpdateGuestNetworkInterface(ctx, t, m) })
t.Run("TestStartInstance", func(t *testing.T) { testStartInstance(ctx, t, m) })
Expand Down Expand Up @@ -667,6 +668,29 @@ func testSetMetadata(ctx context.Context, t *testing.T, m *Machine) {
}
}

func testDescribeInstance(ctx context.Context, t *testing.T, m *Machine) {
instanceInfo, err := m.DescribeInstance(ctx)
if err != nil {
t.Errorf("failed to describe instance: %v", err)
}

if instanceInfo == nil {
t.Error("instance info is nil")
}

if instanceInfo.ID == nil || *instanceInfo.ID == "" {
t.Error("invalid instance id")
}

if instanceInfo.State == nil || *instanceInfo.State == "" {
t.Error("invalid vmm state")
}

if instanceInfo.VmmVersion == nil || *instanceInfo.VmmVersion == "" {
t.Error("invalid vmm version")
}
}

func TestLogFiles(t *testing.T) {
cfg := Config{
Debug: true,
Expand Down
3 changes: 3 additions & 0 deletions machineiface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ package firecracker

import (
"context"

models "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
)

// This ensures the interface method signatures match that of Machine
Expand All @@ -30,4 +32,5 @@ type MachineIface interface {
SetMetadata(context.Context, interface{}) error
UpdateGuestDrive(context.Context, string, string, ...PatchGuestDriveByIDOpt) error
UpdateGuestNetworkInterfaceRateLimit(context.Context, string, RateLimiterSet, ...PatchGuestNetworkInterfaceByIDOpt) error
DescribeInstance(ctx context.Context) (*models.InstanceInfo, error)
}