Copilot commented on code in PR #315: URL: https://github.com/apache/cloudstack-terraform-provider/pull/315#discussion_r3795500442
########## cloudstack/data_source_cloudstack_vgpu_profile.go: ########## @@ -0,0 +1,190 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package cloudstack + +import ( + "fmt" + "log" + "reflect" + "regexp" + "strings" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func dataSourceCloudstackVgpuProfile() *schema.Resource { + return &schema.Resource{ + Read: datasourceCloudStackVgpuProfileRead, + Schema: map[string]*schema.Schema{ + "filter": dataSourceFiltersSchema(), + + //Computed values + "id": { + Type: schema.TypeString, + Computed: true, + }, + "name": { + Description: "the name of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "description": { + Description: "the description of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "device_id": { + Description: "the device id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_name": { + Description: "the device name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "gpu_card_id": { + Description: "the GPU card id of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "gpu_card_name": { + Description: "the GPU card name of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "max_heads": { + Description: "the maximum displays per vGPU instance", + Type: schema.TypeInt, + Computed: true, + }, + "max_resolution_x": { + Description: "the maximum X resolution per display", + Type: schema.TypeInt, + Computed: true, + }, + "max_resolution_y": { + Description: "the maximum Y resolution per display", + Type: schema.TypeInt, + Computed: true, + }, + "max_vgpu_per_physical_gpu": { + Description: "the maximum number of vGPU instances per physical GPU", + Type: schema.TypeInt, + Computed: true, + }, + "vendor_id": { + Description: "the vendor id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_name": { + Description: "the vendor name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "video_ram": { + Description: "the video RAM size in MB for the vGPU profile", + Type: schema.TypeInt, + Computed: true, + }, + }, + } +} + +func datasourceCloudStackVgpuProfileRead(d *schema.ResourceData, meta interface{}) error { + cs := meta.(*cloudstack.CloudStackClient) + p := cs.GPU.NewListVgpuProfilesParams() + + csVgpuProfiles, err := cs.GPU.ListVgpuProfiles(p) + if err != nil { + return fmt.Errorf("failed to list vGPU profiles: %s", err) + } + + filters := d.Get("filter") + + for _, profile := range csVgpuProfiles.VgpuProfiles { + match, err := applyVgpuProfileFilters(profile, filters.(*schema.Set)) + if err != nil { + return err + } + if match { + return vgpuProfileDescriptionAttributes(d, profile) + } + } + + return fmt.Errorf("no vGPU profiles found") +} + +func vgpuProfileDescriptionAttributes(d *schema.ResourceData, profile *cloudstack.VgpuProfile) error { + d.SetId(profile.Id) + + fields := map[string]interface{}{ + "id": profile.Id, + "name": profile.Name, + "description": profile.Description, + "device_id": profile.Deviceid, + "device_name": profile.Devicename, + "gpu_card_id": profile.Gpucardid, + "gpu_card_name": profile.Gpucardname, + "max_heads": profile.Maxheads, + "max_resolution_x": profile.Maxresolutionx, + "max_resolution_y": profile.Maxresolutiony, + "max_vgpu_per_physical_gpu": profile.Maxvgpuperphysicalgpu, + "vendor_id": profile.Vendorid, + "vendor_name": profile.Vendorname, + "video_ram": profile.Videoram, + } + + for k, v := range fields { + if err := d.Set(k, v); err != nil { + log.Printf("[WARN] Error setting %s: %s", k, err) + } + } + + return nil +} + +func applyVgpuProfileFilters(profile *cloudstack.VgpuProfile, filters *schema.Set) (bool, error) { + val := reflect.ValueOf(profile).Elem() + + for _, f := range filters.List() { + filter := f.(map[string]interface{}) + r, err := regexp.Compile(filter["value"].(string)) + if err != nil { + return false, fmt.Errorf("invalid regex: %s", err) + } + updatedName := strings.ReplaceAll(filter["name"].(string), "_", "") + profileField := val.FieldByNameFunc(func(fieldName string) bool { + if strings.EqualFold(fieldName, updatedName) { + updatedName = fieldName + return true + } + return false + }).String() + + if !r.MatchString(profileField) { + return false, nil + } + } Review Comment: This filter implementation calls `.String()` on the reflected field unconditionally. That will panic if the requested field is non-string (e.g., `max_heads`, `video_ram`, `max_resolution_x/y`) or if the field name doesn't exist (invalid `reflect.Value`). Since the docs state filters can target any returned field, this needs to safely handle missing fields and non-string kinds (e.g., check `IsValid()`, then convert via a kind switch or `fmt.Sprint(field.Interface())`). ########## cloudstack/service_offering_util.go: ########## @@ -170,6 +170,16 @@ func (state *ServiceOfferingDiskQosStorage) commonRead(ctx context.Context, cs * } +func (state *ServiceOfferingGpu) commonRead(ctx context.Context, cs *cloudstack.ServiceOffering) { + if cs.Vgpuprofileid != "" { + state.VgpuProfileId = types.StringValue(cs.Vgpuprofileid) + } + if cs.Gpucount > 0 { + state.Count = types.Int32Value(int32(cs.Gpucount)) Review Comment: On read, `VgpuProfileId` and `Count` are only updated when the API returns non-empty / >0 values. If the offering is changed out-of-band (GPU removed/reset), these fields will remain stuck at their previous state values, preventing drift detection. Consider explicitly setting `state.VgpuProfileId` to `types.StringNull()` when `cs.Vgpuprofileid == \"\"`, and `state.Count` to `types.Int32Null()` (or a known default) when `cs.Gpucount` is not set. ########## cloudstack/data_source_cloudstack_gpu_card.go: ########## @@ -0,0 +1,142 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package cloudstack + +import ( + "fmt" + "log" + "reflect" + "regexp" + "strings" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func dataSourceCloudstackGpuCard() *schema.Resource { + return &schema.Resource{ + Read: datasourceCloudStackGpuCardRead, + Schema: map[string]*schema.Schema{ + "filter": dataSourceFiltersSchema(), + + //Computed values + "id": { + Type: schema.TypeString, + Computed: true, + }, + "name": { + Description: "the name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_id": { + Description: "the device id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_name": { + Description: "the device name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_id": { + Description: "the vendor id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_name": { + Description: "the vendor name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + }, + } +} + +func datasourceCloudStackGpuCardRead(d *schema.ResourceData, meta interface{}) error { + cs := meta.(*cloudstack.CloudStackClient) + p := cs.GPU.NewListGpuCardsParams() + + csGpuCards, err := cs.GPU.ListGpuCards(p) + if err != nil { + return fmt.Errorf("failed to list GPU cards: %s", err) + } + + filters := d.Get("filter") + + for _, card := range csGpuCards.GpuCards { + match, err := applyGpuCardFilters(card, filters.(*schema.Set)) + if err != nil { + return err + } + if match { + return gpuCardDescriptionAttributes(d, card) + } + } + + return fmt.Errorf("no GPU cards found") +} + +func gpuCardDescriptionAttributes(d *schema.ResourceData, card *cloudstack.GpuCard) error { + d.SetId(card.Id) + + fields := map[string]interface{}{ + "id": card.Id, + "name": card.Name, + "device_id": card.Deviceid, + "device_name": card.Devicename, + "vendor_id": card.Vendorid, + "vendor_name": card.Vendorname, + } + + for k, v := range fields { + if err := d.Set(k, v); err != nil { + log.Printf("[WARN] Error setting %s: %s", k, err) + } + } + + return nil +} + +func applyGpuCardFilters(card *cloudstack.GpuCard, filters *schema.Set) (bool, error) { + val := reflect.ValueOf(card).Elem() + + for _, f := range filters.List() { + filter := f.(map[string]interface{}) + r, err := regexp.Compile(filter["value"].(string)) + if err != nil { + return false, fmt.Errorf("invalid regex: %s", err) + } + updatedName := strings.ReplaceAll(filter["name"].(string), "_", "") + cardField := val.FieldByNameFunc(func(fieldName string) bool { + if strings.EqualFold(fieldName, updatedName) { + updatedName = fieldName + return true + } + return false + }).String() + + if !r.MatchString(cardField) { + return false, nil + } + } Review Comment: Even if current `GpuCard` fields are strings, this still panics when a user provides an unknown `filter.name` because `FieldByNameFunc` will return an invalid value and `.String()` will panic. Consider validating that the field exists (`IsValid()`) and returning a clear error like `unknown filter field '...'`. ########## cloudstack/data_source_cloudstack_gpu_card.go: ########## @@ -0,0 +1,142 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package cloudstack + +import ( + "fmt" + "log" + "reflect" + "regexp" + "strings" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func dataSourceCloudstackGpuCard() *schema.Resource { + return &schema.Resource{ + Read: datasourceCloudStackGpuCardRead, + Schema: map[string]*schema.Schema{ + "filter": dataSourceFiltersSchema(), + + //Computed values + "id": { + Type: schema.TypeString, + Computed: true, + }, + "name": { + Description: "the name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_id": { + Description: "the device id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_name": { + Description: "the device name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_id": { + Description: "the vendor id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_name": { + Description: "the vendor name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + }, + } +} + +func datasourceCloudStackGpuCardRead(d *schema.ResourceData, meta interface{}) error { + cs := meta.(*cloudstack.CloudStackClient) + p := cs.GPU.NewListGpuCardsParams() + + csGpuCards, err := cs.GPU.ListGpuCards(p) + if err != nil { + return fmt.Errorf("failed to list GPU cards: %s", err) + } + + filters := d.Get("filter") + + for _, card := range csGpuCards.GpuCards { + match, err := applyGpuCardFilters(card, filters.(*schema.Set)) + if err != nil { + return err + } + if match { + return gpuCardDescriptionAttributes(d, card) + } + } + + return fmt.Errorf("no GPU cards found") Review Comment: Similar to the vGPU profile data source, this error message lacks context about the applied filters. Including the filters (or at least the filter names) would make failures easier to debug. ########## cloudstack/service_offering_unconstrained_resource_test.go: ########## @@ -205,3 +214,25 @@ resource "cloudstack_service_offering_unconstrained" "disk_storage" { } } ` + +const testAccServiceOfferingUnconstrained_gpu = ` +resource "cloudstack_service_offering_unconstrained" "gpu" { + display_text = "gpu" + name = "gpu" + + host_tags = "test0101,test0202" + network_rate = 1024 + deployment_planner = "UserDispersingPlanner" + + dynamic_scaling_enabled = true + is_volatile = true + limit_cpu_use = true + offer_ha = true + + gpu = { + vgpu_profile_id = "a6000-8a-profile" + count = 1 + display = true + } +} +` Review Comment: These acceptance tests hard-code a specific `vgpu_profile_id` value, which will cause consistent failures in environments that don't have that exact profile. To make the tests portable, consider reading the profile ID from an environment variable (with a precheck that skips when missing) or dynamically looking it up via the new `cloudstack_vgpu_profile` data source in the test config. ########## cloudstack/data_source_cloudstack_vgpu_profile.go: ########## @@ -0,0 +1,190 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package cloudstack + +import ( + "fmt" + "log" + "reflect" + "regexp" + "strings" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func dataSourceCloudstackVgpuProfile() *schema.Resource { + return &schema.Resource{ + Read: datasourceCloudStackVgpuProfileRead, + Schema: map[string]*schema.Schema{ + "filter": dataSourceFiltersSchema(), + + //Computed values + "id": { + Type: schema.TypeString, + Computed: true, + }, + "name": { + Description: "the name of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "description": { + Description: "the description of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "device_id": { + Description: "the device id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "device_name": { + Description: "the device name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "gpu_card_id": { + Description: "the GPU card id of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "gpu_card_name": { + Description: "the GPU card name of the vGPU profile", + Type: schema.TypeString, + Computed: true, + }, + "max_heads": { + Description: "the maximum displays per vGPU instance", + Type: schema.TypeInt, + Computed: true, + }, + "max_resolution_x": { + Description: "the maximum X resolution per display", + Type: schema.TypeInt, + Computed: true, + }, + "max_resolution_y": { + Description: "the maximum Y resolution per display", + Type: schema.TypeInt, + Computed: true, + }, + "max_vgpu_per_physical_gpu": { + Description: "the maximum number of vGPU instances per physical GPU", + Type: schema.TypeInt, + Computed: true, + }, + "vendor_id": { + Description: "the vendor id of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "vendor_name": { + Description: "the vendor name of the GPU card", + Type: schema.TypeString, + Computed: true, + }, + "video_ram": { + Description: "the video RAM size in MB for the vGPU profile", + Type: schema.TypeInt, + Computed: true, + }, + }, + } +} + +func datasourceCloudStackVgpuProfileRead(d *schema.ResourceData, meta interface{}) error { + cs := meta.(*cloudstack.CloudStackClient) + p := cs.GPU.NewListVgpuProfilesParams() + + csVgpuProfiles, err := cs.GPU.ListVgpuProfiles(p) + if err != nil { + return fmt.Errorf("failed to list vGPU profiles: %s", err) + } + + filters := d.Get("filter") + + for _, profile := range csVgpuProfiles.VgpuProfiles { + match, err := applyVgpuProfileFilters(profile, filters.(*schema.Set)) + if err != nil { + return err + } + if match { + return vgpuProfileDescriptionAttributes(d, profile) + } + } + + return fmt.Errorf("no vGPU profiles found") Review Comment: The error message doesn't include any context (e.g., the configured filters), which makes it hard to diagnose why a lookup failed. Consider including the filter set and/or a hint like \"check filter regex\" in the error. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
