Skip to content

feat(BREV-1938): add logging options for Shadeform Client #49

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

Merged
patelspratik merged 6 commits into from
Oct 28, 2025
Merged

feat(BREV-1938): add logging options for Shadeform Client #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7c37d9f
feat(BREV-1938): add logging options for Shadeform Client
patelspratik Oct 17, 2025
942394a
feat(BREV-1938): adding wrapped errors and some debugging
patelspratik Oct 18, 2025
0267802
feat(BREV-1938): don't log loops, expose some swalloed errors
patelspratik Oct 18, 2025
d453811
fix(BREV-1983): review feedback
patelspratik Oct 28, 2025
9c51233
Merge branch 'main' into pratik/BREV-1938/loggingoption
patelspratik Oct 28, 2025
8074236
feat(BREV-2049): undoing update
patelspratik Oct 28, 2025
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
8 changes: 6 additions & 2 deletions v1/providers/shadeform/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,12 @@ func (c *ShadeformCredential) GetCapabilities(ctx context.Context) (v1.Capabilit
}

// MakeClient creates a new Shadeform client from this credential
func (c *ShadeformCredential) MakeClient(_ context.Context, _ string) (v1.CloudClient, error) {
return NewShadeformClient(c.RefID, c.APIKey), nil
func (c *ShadeformCredential) MakeClient(ctx context.Context, location string) (v1.CloudClient, error) {
return c.MakeClientWithOptions(ctx, location)
}

func (c *ShadeformCredential) MakeClientWithOptions(_ context.Context, _ string, opts ...ShadeformClientOption) (v1.CloudClient, error) {
return NewShadeformClient(c.RefID, c.APIKey, opts...), nil
}
Comment on lines 65 to 67
Copy link
Contributor

@drewmalin drewmalin Oct 21, 2025

Choose a reason for hiding this comment

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

This could instead call:

  return c.MakeClient(ctx, location), nil


// Shadeform implements the CloudClient interface for Shadeform
Expand Down
95 changes: 55 additions & 40 deletions v1/providers/shadeform/instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ package v1

import (
"context"
"errors"
"fmt"
"io"
"strings"

"github.com/alecthomas/units"
"github.com/brevdev/cloud/internal/errors"
v1 "github.com/brevdev/cloud/v1"
openapi "github.com/brevdev/cloud/v1/providers/shadeform/gen/shadeform"
"github.com/google/uuid"
Expand All @@ -24,9 +24,11 @@ const (
func (c *ShadeformClient) CreateInstance(ctx context.Context, attrs v1.CreateInstanceAttrs) (*v1.Instance, error) { //nolint:gocyclo,funlen // ok
authCtx := c.makeAuthContext(ctx)

c.logger.Debug(ctx, "Creating instance", v1.LogField("instanceAttrs", attrs))
// Check if the instance type is allowed by configuration
if !c.isInstanceTypeAllowed(attrs.InstanceType) {
return nil, fmt.Errorf("instance type: %v is not deployable", attrs.InstanceType)
allowed, _ := c.isInstanceTypeAllowed(attrs.InstanceType)
if !allowed {
return nil, errors.WrapAndTrace(fmt.Errorf("instance type: %v is not deployable", attrs.InstanceType))
}

sshKeyID := ""
Expand All @@ -38,52 +40,53 @@ func (c *ShadeformClient) CreateInstance(ctx context.Context, attrs v1.CreateIns

if keyPairName == "" {
keyPairName = uuid.New().String()
c.logger.Debug(ctx, "No key pair name provided, generating new one", v1.LogField("keyPairName", keyPairName))
Copy link
Contributor

@drewmalin drewmalin Oct 18, 2025

Choose a reason for hiding this comment

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

👍

}

if attrs.PublicKey != "" {
var err error
sshKeyID, err = c.addSSHKey(ctx, keyPairName, attrs.PublicKey)
if err != nil && !strings.Contains(err.Error(), "name must be unique") {
return nil, fmt.Errorf("failed to add SSH key: %w", err)
return nil, errors.WrapAndTrace(fmt.Errorf("failed to add SSH key: %w", err))
}
}

region := attrs.Location
cloud, shadeInstanceType, err := c.getShadeformCloudAndInstanceType(attrs.InstanceType)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

cloudEnum, err := openapi.NewCloudFromValue(cloud)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

// Add refID tag
refIDTag, err := c.createTag(refIDTagName, attrs.RefID)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

// Add cloudRefID tag
cloudCredRefIDTag, err := c.createTag(cloudCredRefIDTagName, c.GetReferenceID())
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

tags := []string{refIDTag, cloudCredRefIDTag}
// Add all other tags
for key, value := range attrs.Tags {
createdTag, err := c.createTag(key, value)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}
tags = append(tags, createdTag)
}

base64Script, err := c.GenerateFirewallScript(attrs.FirewallRules)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

req := openapi.CreateRequest{
Expand All @@ -108,18 +111,18 @@ func (c *ShadeformClient) CreateInstance(ctx context.Context, attrs v1.CreateIns
}
if err != nil {
httpMessage, _ := io.ReadAll(httpResp.Body)
return nil, fmt.Errorf("failed to create instance: %w, %s", err, string(httpMessage))
return nil, errors.WrapAndTrace(fmt.Errorf("failed to create instance: %w, %s", err, string(httpMessage)))
}

if resp == nil {
return nil, fmt.Errorf("no instance returned from create request")
return nil, errors.WrapAndTrace(fmt.Errorf("no instance returned from create request"))
}

// Since Shadeform doesn't return the full instance that's created, we need to make a second API call to get
// the created instance after we call create
createdInstance, err := c.GetInstance(authCtx, v1.CloudProviderInstanceID(resp.Id))
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

return createdInstance, nil
Expand Down Expand Up @@ -152,11 +155,11 @@ func (c *ShadeformClient) addSSHKey(ctx context.Context, keyPairName string, pub
}
if err != nil {
httpMessage, _ := io.ReadAll(httpResp.Body)
return "", fmt.Errorf("failed to add SSH Key: %w, %s", err, string(httpMessage))
return "", errors.WrapAndTrace(fmt.Errorf("failed to add SSH Key: %w, %s", err, string(httpMessage)))
}

if resp == nil {
return "", fmt.Errorf("no instance returned from post request")
return "", errors.WrapAndTrace(fmt.Errorf("no instance returned from post request"))
}

return resp.Id, nil
Expand All @@ -170,16 +173,16 @@ func (c *ShadeformClient) GetInstance(ctx context.Context, instanceID v1.CloudPr
defer func() { _ = httpResp.Body.Close() }()
}
if err != nil {
return nil, fmt.Errorf("failed to get instance: %w", err)
return nil, errors.WrapAndTrace(fmt.Errorf("failed to get instance: %w", err))
}

if resp == nil {
return nil, fmt.Errorf("no instance returned from get request")
return nil, errors.WrapAndTrace(fmt.Errorf("no instance returned from get request"))
}

instance, err := c.convertInstanceInfoResponseToV1Instance(*resp)
instance, err := c.convertInstanceInfoResponseToV1Instance(ctx, *resp)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

return instance, nil
Expand All @@ -193,7 +196,7 @@ func (c *ShadeformClient) TerminateInstance(ctx context.Context, instanceID v1.C
defer func() { _ = httpResp.Body.Close() }()
}
if err != nil {
return fmt.Errorf("failed to terminate instance: %w", err)
return errors.WrapAndTrace(fmt.Errorf("failed to terminate instance: %w", err))
}

return nil
Expand All @@ -207,14 +210,14 @@ func (c *ShadeformClient) ListInstances(ctx context.Context, _ v1.ListInstancesA
defer func() { _ = httpResp.Body.Close() }()
}
if err != nil {
return nil, fmt.Errorf("failed to list instances: %w", err)
return nil, errors.WrapAndTrace(fmt.Errorf("failed to list instances: %w", err))
}

var instances []v1.Instance
for _, instance := range resp.Instances {
singleInstance, err := c.convertShadeformInstanceToV1Instance(instance)
singleInstance, err := c.convertShadeformInstanceToV1Instance(ctx, instance)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}
instances = append(instances, *singleInstance)
}
Expand Down Expand Up @@ -254,27 +257,33 @@ func (c *ShadeformClient) getLifecycleStatus(status string) v1.LifecycleStatus {
}

// convertInstanceInfoResponseToV1Instance - converts Instance Info to v1 instance
func (c *ShadeformClient) convertInstanceInfoResponseToV1Instance(instanceInfo openapi.InstanceInfoResponse) (*v1.Instance, error) {
func (c *ShadeformClient) convertInstanceInfoResponseToV1Instance(ctx context.Context, instanceInfo openapi.InstanceInfoResponse) (*v1.Instance, error) {
c.logger.Debug(ctx, "converting instance info response to v1 instance", v1.LogField("instanceInfo", instanceInfo))
instanceType := c.getInstanceType(string(instanceInfo.Cloud), instanceInfo.ShadeInstanceType)
lifeCycleStatus := c.getLifecycleStatus(string(instanceInfo.Status))

tags, err := c.convertShadeformTagToV1Tag(instanceInfo.Tags)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

refID, found := tags[refIDTagName]
if !found {
return nil, errors.New("could not find refID tag")
return nil, errors.WrapAndTrace(errors.New("could not find refID tag"))
}
c.logger.Debug(ctx, "refID found, deleting from tags", v1.LogField("refID", refID))
Copy link
Contributor

@drewmalin drewmalin Oct 21, 2025

Choose a reason for hiding this comment

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

Followup for when this gets consumed by dev-plane:

it would be nice for these logs to be very obviously "shadeform" (and for other providers, very obviously "[provider]"), without needing each implementer to remember to associate their name/ID with each and every log line.

One way we could do that is to adjust the creation of the logger itself (over in dev-plane) to always add span attributes that indicate the provider / cloud cred ID. Maybe instead of:

dev-plane: internal/cloud/cloudsdkv1adapter/adapter_logger.go:

type AdapterLogger struct{}

func NewAdapterLogger() *AdapterLogger {
	return &AdapterLogger{}
}

we have:

type AdapterLogger struct{
	commonFields []zapcore.Field
}

func NewAdapterLogger(commonFields ...zapcore.Field) *AdapterLogger {
	return &AdapterLogger{
		commonFields: commonFields,
	}
}

then we could change the fieldsToZapFields to something like:

func (a *AdapterLogger) fieldsToZapFields(fields []cloudv1.Field) []zapcore.Field {
	zapFields := []zapcore.Field{}
	zapFields = append(zapFields, a.commonFields...)
	for _, field := range fields {
		zapFields = append(zapFields, zap.Any(field.Key, field.Value))
	}
	return zapFields
}

then when the logger is created (over in internal/cloudcred/service.go) instead of:

			logger := cloudsdkadapterv1.NewAdapterLogger()
			launchpadClient, err := cred.MakeClientWithOptions(ctx, location, launchpadv1.WithLogger(logger))
			if err != nil {
				return nil, errors.WrapAndTrace(err)
			}

we could have:

			logger := cloudsdkadapterv1.NewAdapterLogger(
				zap.String("cloud_cred_id", string(c.ID)),
				zap.String("cloud_provider_id", string(c.ProviderID)),
				zap.String("location", location),
			)
			launchpadClient, err := cred.MakeClientWithOptions(ctx, location, launchpadv1.WithLogger(logger))
			if err != nil {
				return nil, errors.WrapAndTrace(err)
			}

Copy link
Contributor

@drewmalin drewmalin Oct 28, 2025

Choose a reason for hiding this comment

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

Created linear ticket: https://linear.app/nvidia/issue/BREV-2053/add-provider-prefix-to-cloud-sdk-logs

delete(tags, refIDTagName)

cloudCredRefID := tags[cloudCredRefIDTagName]
if err != nil {
return nil, errors.New("could not find cloudCredRefID tag")
cloudCredRefID, found := tags[cloudCredRefIDTagName]
if !found {
return nil, errors.WrapAndTrace(errors.New("could not find cloudCredRefID tag"))
}
c.logger.Debug(ctx, "cloudCredRefID found, deleting from tags", v1.LogField("cloudCredRefID", cloudCredRefID))
delete(tags, cloudCredRefIDTagName)

diskSize := units.Base2Bytes(instanceInfo.Configuration.StorageInGb) * units.GiB
c.logger.Debug(ctx, "calculated diskSize", v1.LogField("diskSize", diskSize), v1.LogField("storageInGb", instanceInfo.Configuration.StorageInGb))

instance := &v1.Instance{
Name: c.getProvidedInstanceName(instanceInfo.Name),
CreatedAt: instanceInfo.CreatedAt,
Expand All @@ -285,7 +294,7 @@ func (c *ShadeformClient) convertInstanceInfoResponseToV1Instance(instanceInfo o
ImageID: instanceInfo.Configuration.Os,
InstanceType: instanceType,
InstanceTypeID: v1.InstanceTypeID(c.getInstanceTypeID(instanceType, instanceInfo.Region)),
DiskSize: units.Base2Bytes(instanceInfo.Configuration.StorageInGb) * units.GiB,
DiskSize: diskSize,
SSHUser: instanceInfo.SshUser,
SSHPort: int(instanceInfo.SshPort),
Status: v1.Status{
Expand All @@ -304,27 +313,33 @@ func (c *ShadeformClient) convertInstanceInfoResponseToV1Instance(instanceInfo o

// convertInstanceInfoResponseToV1Instance - converts /instances response to v1 instance; the api struct is slightly
// different from instance info response and expected to diverge so keeping it as a separate function for now
func (c *ShadeformClient) convertShadeformInstanceToV1Instance(shadeformInstance openapi.Instance) (*v1.Instance, error) {
func (c *ShadeformClient) convertShadeformInstanceToV1Instance(ctx context.Context, shadeformInstance openapi.Instance) (*v1.Instance, error) {
c.logger.Debug(ctx, "converting shadeform instance to v1 instance", v1.LogField("shadeformInstance", shadeformInstance))
instanceType := c.getInstanceType(string(shadeformInstance.Cloud), shadeformInstance.ShadeInstanceType)
lifeCycleStatus := c.getLifecycleStatus(string(shadeformInstance.Status))

tags, err := c.convertShadeformTagToV1Tag(shadeformInstance.Tags)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}

refID, found := tags[refIDTagName]
if !found {
return nil, errors.New("could not find refID tag")
return nil, errors.WrapAndTrace(errors.New("could not find refID tag"))
}
c.logger.Debug(ctx, "refID found, deleting from tags", v1.LogField("refID", refID))
delete(tags, refIDTagName)

cloudCredRefID := tags[cloudCredRefIDTagName]
if err != nil {
return nil, errors.New("could not find cloudCredRefID tag")
cloudCredRefID, found := tags[cloudCredRefIDTagName]
if !found {
return nil, errors.WrapAndTrace(errors.New("could not find cloudCredRefID tag"))
}
c.logger.Debug(ctx, "cloudCredRefID found, deleting from tags", v1.LogField("cloudCredRefID", cloudCredRefID))
delete(tags, cloudCredRefIDTagName)

diskSize := units.Base2Bytes(shadeformInstance.Configuration.StorageInGb) * units.GiB
c.logger.Debug(ctx, "calculated diskSize", v1.LogField("diskSize", diskSize), v1.LogField("storageInGb", shadeformInstance.Configuration.StorageInGb))

instance := &v1.Instance{
Name: shadeformInstance.Name,
CreatedAt: shadeformInstance.CreatedAt,
Expand All @@ -334,7 +349,7 @@ func (c *ShadeformClient) convertShadeformInstanceToV1Instance(shadeformInstance
Hostname: hostname,
ImageID: shadeformInstance.Configuration.Os,
InstanceType: instanceType,
DiskSize: units.Base2Bytes(shadeformInstance.Configuration.StorageInGb) * units.GiB,
DiskSize: diskSize,
SSHUser: shadeformInstance.SshUser,
SSHPort: int(shadeformInstance.SshPort),
Status: v1.Status{
Expand All @@ -357,7 +372,7 @@ func (c *ShadeformClient) convertShadeformTagToV1Tag(shadeformTags []string) (v1
for _, tag := range shadeformTags {
key, value, err := c.getTag(tag)
if err != nil {
return nil, err
return nil, errors.WrapAndTrace(err)
}
tags[key] = value
}
Expand All @@ -366,7 +381,7 @@ func (c *ShadeformClient) convertShadeformTagToV1Tag(shadeformTags []string) (v1

func (c *ShadeformClient) createTag(key string, value string) (string, error) {
if strings.Contains(key, "=") {
return "", errors.New("tags cannot contain the '=' character")
return "", errors.WrapAndTrace(errors.New("tags cannot contain the '=' character"))
}

return fmt.Sprintf("%v=%v", key, value), nil
Expand All @@ -375,7 +390,7 @@ func (c *ShadeformClient) createTag(key string, value string) (string, error) {
func (c *ShadeformClient) getTag(shadeformTag string) (string, string, error) {
key, value, found := strings.Cut(shadeformTag, "=")
if !found {
return "", "", fmt.Errorf("tag %v does not conform to the key value tag format", shadeformTag)
return "", "", errors.WrapAndTrace(fmt.Errorf("tag %v does not conform to the key value tag format", shadeformTag))
}
return key, value, nil
}
Loading
Loading