From f286ddb4a13d2374e32b20c0859ff60e0f8345f6 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 27 Jun 2024 14:48:00 +0200 Subject: [PATCH 1/5] refactor(instance_server): use instance_block helpers --- .../services/instance/helpers_instance.go | 37 ++----------------- .../instance/helpers_instance_block.go | 32 ++++++++++++++-- internal/services/instance/server.go | 18 ++++----- 3 files changed, 39 insertions(+), 48 deletions(-) diff --git a/internal/services/instance/helpers_instance.go b/internal/services/instance/helpers_instance.go index dae2a9e2b..44de30394 100644 --- a/internal/services/instance/helpers_instance.go +++ b/internal/services/instance/helpers_instance.go @@ -283,13 +283,6 @@ func sanitizeVolumeMap(volumes map[string]*instance.VolumeServerTemplate) map[st Boot: v.Boot, } } - // For the root volume (index 0) if the size is 0, it is considered as a volume created from an image. - // The size is not passed to the API, so it's computed by the API - case index == "0" && v.Size == nil: - v = &instance.VolumeServerTemplate{ - VolumeType: v.VolumeType, - Boot: v.Boot, - } // If none of the above conditions are met, the volume is passed as it to the API default: } @@ -535,34 +528,12 @@ func instanceIPHasMigrated(d *schema.ResourceData) bool { } func instanceServerAdditionalVolumeTemplate(api *BlockAndInstanceAPI, zone scw.Zone, volumeID string) (*instance.VolumeServerTemplate, error) { - vol, err := api.GetVolume(&instance.GetVolumeRequest{ - Zone: zone, + vol, err := api.GetUnknownVolume(&GetUnknownVolumeRequest{ VolumeID: locality.ExpandID(volumeID), - }) - if err == nil { - return &instance.VolumeServerTemplate{ - ID: &vol.Volume.ID, - Name: &vol.Volume.Name, - VolumeType: vol.Volume.VolumeType, - Size: &vol.Volume.Size, - }, nil - } - if !httperrors.Is404(err) { - return nil, err - } - - blockVol, err := api.blockAPI.GetVolume(&blockSDK.GetVolumeRequest{ Zone: zone, - VolumeID: locality.ExpandID(volumeID), }) - if err == nil { - return &instance.VolumeServerTemplate{ - ID: &blockVol.ID, - Name: &blockVol.Name, - VolumeType: "sbs_volume", - Size: &blockVol.Size, - }, nil + if err != nil { + return nil, err } - - return nil, err + return vol.VolumeTemplate(), nil } diff --git a/internal/services/instance/helpers_instance_block.go b/internal/services/instance/helpers_instance_block.go index 2bda65646..fb3fa6d1f 100644 --- a/internal/services/instance/helpers_instance_block.go +++ b/internal/services/instance/helpers_instance_block.go @@ -25,8 +25,9 @@ type UnknownVolume struct { Zone scw.Zone ID string Name string - Size scw.Size + Size *scw.Size ServerID *string + Boot *bool // IsBlockVolume is true if volume is managed by block API IsBlockVolume bool @@ -34,12 +35,35 @@ type UnknownVolume struct { InstanceVolumeType instance.VolumeVolumeType } +func (volume *UnknownVolume) VolumeTemplate() *instance.VolumeServerTemplate { + template := &instance.VolumeServerTemplate{} + if volume.ID != "" { + template.ID = &volume.ID + } else { + template.Size = volume.Size + } + + if volume.Boot != nil { + template.Boot = volume.Boot + } + + if volume.IsBlockVolume { + template.VolumeType = volume.InstanceVolumeType + } else { + template.Name = &volume.Name + } + + return template +} + // VolumeTemplateUpdate return a VolumeServerTemplate for an UpdateServer request func (volume *UnknownVolume) VolumeTemplateUpdate() *instance.VolumeServerTemplate { template := &instance.VolumeServerTemplate{ - ID: scw.StringPtr(volume.ID), Name: &volume.Name, // name is ignored by the API, any name will work here } + if volume.ID != "" { + template.ID = scw.StringPtr(volume.ID) + } if volume.IsBlockVolume { template.Name = nil template.VolumeType = volume.InstanceVolumeType @@ -73,7 +97,7 @@ func (api *BlockAndInstanceAPI) GetUnknownVolume(req *GetUnknownVolumeRequest, o Zone: getVolumeResponse.Volume.Zone, ID: getVolumeResponse.Volume.ID, Name: getVolumeResponse.Volume.Name, - Size: getVolumeResponse.Volume.Size, + Size: &getVolumeResponse.Volume.Size, IsBlockVolume: false, InstanceVolumeType: getVolumeResponse.Volume.VolumeType, } @@ -96,7 +120,7 @@ func (api *BlockAndInstanceAPI) GetUnknownVolume(req *GetUnknownVolumeRequest, o Zone: blockVolume.Zone, ID: blockVolume.ID, Name: blockVolume.Name, - Size: blockVolume.Size, + Size: &blockVolume.Size, IsBlockVolume: true, InstanceVolumeType: instance.VolumeVolumeTypeSbsVolume, } diff --git a/internal/services/instance/server.go b/internal/services/instance/server.go index 446cf57d5..b4f9bc164 100644 --- a/internal/services/instance/server.go +++ b/internal/services/instance/server.go @@ -459,14 +459,13 @@ func ResourceInstanceServerCreate(ctx context.Context, d *schema.ResourceData, m } else if sizeInput > 0 { rootVolumeSize = scw.SizePtr(scw.Size(uint64(sizeInput) * gb)) } - - req.Volumes["0"] = &instanceSDK.VolumeServerTemplate{ - Name: types.ExpandStringPtr(rootVolumeName), - ID: types.ExpandStringPtr(rootVolumeID), - VolumeType: instanceSDK.VolumeVolumeType(rootVolumeType), - Size: rootVolumeSize, - Boot: rootVolumeIsBootVolume, - } + req.Volumes["0"] = (&UnknownVolume{ + Name: rootVolumeName, + ID: rootVolumeID, + InstanceVolumeType: instanceSDK.VolumeVolumeType(rootVolumeType), + Size: rootVolumeSize, + Boot: rootVolumeIsBootVolume, + }).VolumeTemplate() if raw, ok := d.GetOk("additional_volume_ids"); ok { for i, volumeID := range raw.([]interface{}) { // We have to get the volume to know whether it is a local or a block volume @@ -483,9 +482,6 @@ func ResourceInstanceServerCreate(ctx context.Context, d *schema.ResourceData, m return diag.FromErr(err) } - // Sanitize the volume map to respect API schemas - req.Volumes = sanitizeVolumeMap(req.Volumes) - res, err := api.CreateServer(req, scw.WithContext(ctx)) if err != nil { return diag.FromErr(err) From 8dbedea266b01459f0ae63bf24435d07c7dffb4c Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 27 Jun 2024 15:21:37 +0200 Subject: [PATCH 2/5] extract root volume creation --- .../services/instance/helpers_instance.go | 41 +++++++++++++++++++ internal/services/instance/server.go | 37 +---------------- internal/types/map.go | 11 +++++ 3 files changed, 54 insertions(+), 35 deletions(-) diff --git a/internal/services/instance/helpers_instance.go b/internal/services/instance/helpers_instance.go index 44de30394..bd40fa090 100644 --- a/internal/services/instance/helpers_instance.go +++ b/internal/services/instance/helpers_instance.go @@ -537,3 +537,44 @@ func instanceServerAdditionalVolumeTemplate(api *BlockAndInstanceAPI, zone scw.Z } return vol.VolumeTemplate(), nil } + +func prepareRootVolume(rootVolumeI map[string]any, serverType *instance.ServerType, image string) *UnknownVolume { + serverTypeCanBootOnBlock := serverType.VolumesConstraint.MaxSize == 0 + + rootVolumeIsBootVolume := types.ExpandBoolPtr(types.GetMapValue[bool](rootVolumeI, "boot")) + rootVolumeType := types.GetMapValue[string](rootVolumeI, "volume_type") + sizeInput := types.GetMapValue[int](rootVolumeI, "size_in_gb") + rootVolumeID := zonal.ExpandID(types.GetMapValue[string](rootVolumeI, "volume_id")).ID + + // If the rootVolumeType is not defined, define it depending on the offer + if rootVolumeType == "" { + if serverTypeCanBootOnBlock { + rootVolumeType = instance.VolumeVolumeTypeBSSD.String() + } else { + rootVolumeType = instance.VolumeVolumeTypeLSSD.String() + } + } + + rootVolumeName := "" + if image == "" { // When creating an instance from an image, volume should not have a name + rootVolumeName = types.NewRandomName("vol") + } + + var rootVolumeSize *scw.Size + if sizeInput == 0 && rootVolumeType == instance.VolumeVolumeTypeLSSD.String() { + // Compute the rootVolumeSize so it will be valid against the local volume constraints + // It wouldn't be valid if another local volume is added, but in this case + // the user would be informed that it does not fulfill the local volume constraints + rootVolumeSize = scw.SizePtr(serverType.VolumesConstraint.MaxSize) + } else if sizeInput > 0 { + rootVolumeSize = scw.SizePtr(scw.Size(uint64(sizeInput) * gb)) + } + + return &UnknownVolume{ + Name: rootVolumeName, + ID: rootVolumeID, + InstanceVolumeType: instance.VolumeVolumeType(rootVolumeType), + Size: rootVolumeSize, + Boot: rootVolumeIsBootVolume, + } +} diff --git a/internal/services/instance/server.go b/internal/services/instance/server.go index b4f9bc164..78508b09f 100644 --- a/internal/services/instance/server.go +++ b/internal/services/instance/server.go @@ -430,42 +430,9 @@ func ResourceInstanceServerCreate(ctx context.Context, d *schema.ResourceData, m } req.Volumes = make(map[string]*instanceSDK.VolumeServerTemplate) - serverTypeCanBootOnBlock := serverType.VolumesConstraint.MaxSize == 0 - rootVolumeIsBootVolume := types.ExpandBoolPtr(d.Get("root_volume.0.boot")) - rootVolumeType := d.Get("root_volume.0.volume_type").(string) - sizeInput := d.Get("root_volume.0.size_in_gb").(int) - rootVolumeID := zonal.ExpandID(d.Get("root_volume.0.volume_id").(string)).ID - - // If the rootVolumeType is not defined, define it depending on the offer - if rootVolumeType == "" { - if serverTypeCanBootOnBlock { - rootVolumeType = instanceSDK.VolumeVolumeTypeBSSD.String() - } else { - rootVolumeType = instanceSDK.VolumeVolumeTypeLSSD.String() - } - } + rootVolume := d.Get("root_volume.0").(map[string]any) - rootVolumeName := "" - if req.Image == "" { // When creating an instanceSDK from an image, volume should not have a name - rootVolumeName = types.NewRandomName("vol") - } - - var rootVolumeSize *scw.Size - if sizeInput == 0 && rootVolumeType == instanceSDK.VolumeVolumeTypeLSSD.String() { - // Compute the rootVolumeSize so it will be valid against the local volume constraints - // It wouldn't be valid if another local volume is added, but in this case - // the user would be informed that it does not fulfill the local volume constraints - rootVolumeSize = scw.SizePtr(serverType.VolumesConstraint.MaxSize) - } else if sizeInput > 0 { - rootVolumeSize = scw.SizePtr(scw.Size(uint64(sizeInput) * gb)) - } - req.Volumes["0"] = (&UnknownVolume{ - Name: rootVolumeName, - ID: rootVolumeID, - InstanceVolumeType: instanceSDK.VolumeVolumeType(rootVolumeType), - Size: rootVolumeSize, - Boot: rootVolumeIsBootVolume, - }).VolumeTemplate() + req.Volumes["0"] = prepareRootVolume(rootVolume, serverType, req.Image).VolumeTemplate() if raw, ok := d.GetOk("additional_volume_ids"); ok { for i, volumeID := range raw.([]interface{}) { // We have to get the volume to know whether it is a local or a block volume diff --git a/internal/types/map.go b/internal/types/map.go index 53d2c37d8..ad63cb6ed 100644 --- a/internal/types/map.go +++ b/internal/types/map.go @@ -58,3 +58,14 @@ func ExpandMapStringString(data any) map[string]string { } return m } + +// GetMapValue returns the value for a key from a map. +// returns zero value if key does not exist in map. +func GetMapValue[T any](m map[string]any, key string) T { + var val T + valI, exists := m[key] + if exists { + val = valI.(T) + } + return val +} From 8abe419ae869eecfc108a66e1e13bc32b637ee34 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 27 Jun 2024 15:22:38 +0200 Subject: [PATCH 3/5] remove unused sanitizeVolumes function --- .../services/instance/helpers_instance.go | 38 ------------------- 1 file changed, 38 deletions(-) diff --git a/internal/services/instance/helpers_instance.go b/internal/services/instance/helpers_instance.go index bd40fa090..45133babd 100644 --- a/internal/services/instance/helpers_instance.go +++ b/internal/services/instance/helpers_instance.go @@ -254,44 +254,6 @@ func validateLocalVolumeSizes(volumes map[string]*instance.VolumeServerTemplate, return nil } -// sanitizeVolumeMap removes extra data for API validation. -// -// On the api side, there are two possibles validation schemas for volumes and the validator will be chosen dynamically depending on the passed JSON request -// - With an image (in that case the root volume can be skipped because it is taken from the image) -// - Without an image (in that case, the root volume must be defined) -func sanitizeVolumeMap(volumes map[string]*instance.VolumeServerTemplate) map[string]*instance.VolumeServerTemplate { - m := make(map[string]*instance.VolumeServerTemplate) - - for index, v := range volumes { - // Remove extra data for API validation. - switch { - // If a volume already got an ID it is passed as it to the API without specifying the volume type. - // TODO: Fix once instance accept volume type in the schema validation - case v.ID != nil: - if strings.HasPrefix(string(v.VolumeType), "sbs") { - // If volume is from SBS api, the type must be passed - // This rules come from instance API and may not be documented - v = &instance.VolumeServerTemplate{ - ID: v.ID, - Boot: v.Boot, - VolumeType: v.VolumeType, - } - } else { - v = &instance.VolumeServerTemplate{ - ID: v.ID, - Name: v.Name, - Boot: v.Boot, - } - } - // If none of the above conditions are met, the volume is passed as it to the API - default: - } - m[index] = v - } - - return m -} - func preparePrivateNIC( ctx context.Context, data interface{}, server *instance.Server, vpcAPI *vpc.API, From 6cad6e72cc9bb6221313367f0b947e2aee1a018b Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 27 Jun 2024 15:27:44 +0200 Subject: [PATCH 4/5] remove update specific code --- .../services/instance/helpers_instance_block.go | 17 +---------------- internal/services/instance/server.go | 2 +- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/internal/services/instance/helpers_instance_block.go b/internal/services/instance/helpers_instance_block.go index fb3fa6d1f..49f7a8819 100644 --- a/internal/services/instance/helpers_instance_block.go +++ b/internal/services/instance/helpers_instance_block.go @@ -35,6 +35,7 @@ type UnknownVolume struct { InstanceVolumeType instance.VolumeVolumeType } +// VolumeTemplate returns a template to be used for servers requests. func (volume *UnknownVolume) VolumeTemplate() *instance.VolumeServerTemplate { template := &instance.VolumeServerTemplate{} if volume.ID != "" { @@ -56,22 +57,6 @@ func (volume *UnknownVolume) VolumeTemplate() *instance.VolumeServerTemplate { return template } -// VolumeTemplateUpdate return a VolumeServerTemplate for an UpdateServer request -func (volume *UnknownVolume) VolumeTemplateUpdate() *instance.VolumeServerTemplate { - template := &instance.VolumeServerTemplate{ - Name: &volume.Name, // name is ignored by the API, any name will work here - } - if volume.ID != "" { - template.ID = scw.StringPtr(volume.ID) - } - if volume.IsBlockVolume { - template.Name = nil - template.VolumeType = volume.InstanceVolumeType - } - - return template -} - // IsLocal returns true if the volume is a local volume func (volume *UnknownVolume) IsLocal() bool { return !volume.IsBlockVolume && volume.InstanceVolumeType == instance.VolumeVolumeTypeLSSD diff --git a/internal/services/instance/server.go b/internal/services/instance/server.go index 78508b09f..068af4c22 100644 --- a/internal/services/instance/server.go +++ b/internal/services/instance/server.go @@ -800,7 +800,7 @@ func ResourceInstanceServerUpdate(ctx context.Context, d *schema.ResourceData, m if volumeHasChange && !isStopped && volume.IsLocal() && volume.IsAttached() { return diag.FromErr(errors.New("instanceSDK must be stopped to change local volumes")) } - volumes[strconv.Itoa(i+1)] = volume.VolumeTemplateUpdate() + volumes[strconv.Itoa(i+1)] = volume.VolumeTemplate() } serverShouldUpdate = true From 1021059d65cfffa395ff78b0a741fe92bfaf88f4 Mon Sep 17 00:00:00 2001 From: Jules Casteran Date: Thu, 27 Jun 2024 15:56:58 +0200 Subject: [PATCH 5/5] lint --- internal/types/map.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/types/map.go b/internal/types/map.go index ad63cb6ed..5a6fedd0f 100644 --- a/internal/types/map.go +++ b/internal/types/map.go @@ -61,7 +61,7 @@ func ExpandMapStringString(data any) map[string]string { // GetMapValue returns the value for a key from a map. // returns zero value if key does not exist in map. -func GetMapValue[T any](m map[string]any, key string) T { +func GetMapValue[T any](m map[string]any, key string) T { //nolint: ireturn var val T valI, exists := m[key] if exists {