cloudstack-terraform-provider icon indicating copy to clipboard operation
cloudstack-terraform-provider copied to clipboard

Service offering resource update

Open jeanvetorello opened this issue 3 months ago • 6 comments

Overview

This PR completely updates the cloudstack_service_offering resource to support CloudStack API version 4.21, achieving 100% SDK coverage with all 54 parameters from CreateServiceOfferingParams fully implemented.

Motivation

I needed to update the Service Offering API to the latest CloudStack version (4.21) to support modern features like GPU configuration, advanced IOPS settings, and dynamic scaling. The existing implementation was missing critical parameters and had several drift issues.

What's New

1. Complete API v4.21 Coverage (54/54 Parameters)

All parameters from CloudStack Go SDK v2 CreateServiceOfferingParams are now implemented:

CPU & Memory:

  • cpu_number, cpu_speed, memory
  • customized, min_cpu_number, max_cpu_number
  • min_memory, max_memory

GPU Support (NEW):

  • gpu_card - GPU card name (maps to gpucardid)
  • gpu_type - vGPU profile UUID (maps to vgpuprofileid)
  • gpu_count - Number of GPUs

Storage:

  • storage_type, storage_tags, host_tags
  • root_disk_size, encrypt_root
  • provisioning_type, cache_mode

IOPS & Bandwidth (12 parameters):

  • bytes_read_rate, bytes_write_rate
  • bytes_read_rate_max, bytes_write_rate_max
  • bytes_read_rate_max_length, bytes_write_rate_max_length
  • iops_read_rate, iops_write_rate
  • iops_read_rate_max, iops_write_rate_max
  • iops_read_rate_max_length, iops_write_rate_max_length

Performance:

  • disk_iops_min, disk_iops_max
  • hypervisor_snapshot_reserve
  • offer_ha, limit_cpu_use
  • dynamic_scaling_enabled
  • customized_iops

System & Domain:

  • is_system, system_vm_type
  • zone_id, domain_id
  • is_volatile, deployment_planner

Advanced:

  • service_offering_details (map for custom settings)

2. Bug Fixes

Fixed service_offering_details Drift

  • Problem: CloudStack adds system keys (External:key, External:value, purge.db.entities) causing state drift
  • Solution: Implemented filtering in Read function (lines 847-863) to only track user-configured keys

Fixed customized Parameter Logic

  • Problem: Incorrect handling of Fixed vs Custom offerings
  • Solution: Smart conditional logic based on CPU/memory presence:
    • If cpu_number + memory provided → customized = false (Fixed Offering)
    • If customized = true + min/max → Custom Constrained
    • If customized = true + no limits → Custom Unconstrained

Identified ForceNew Parameters (47 of 54)

  • Problem: Most CloudStack Service Offering parameters are immutable after creation
  • Solution: Added ForceNew: true to 47 parameters based on CloudStack API limitations
  • Updateable: Only display_text, host_tags, and storage_tags can be updated in-place

3. Comprehensive Testing

Added 4 new tests covering all CloudStack UI offering types:

  • TestAccServiceOfferingTypeFixed - Fixed CPU/memory
  • TestAccServiceOfferingTypeCustomConstrained - Customizable with limits
  • TestAccServiceOfferingTypeCustomUnconstrained - Fully customizable
  • TestAccServiceOfferingAllUITypes - All types together

Total: 17 tests (13 existing + 4 new)

4. Complete Documentation

Created comprehensive documentation in website/docs/r/service_offering.html.markdown:

  • Detailed explanation of all 3 offering types with decision tree
  • GPU configuration examples (A6000, A100, H100, RTX 4090, Multi-GPU)
  • IOPS/bandwidth configuration examples
  • High-performance offerings
  • ForceNew behavior documentation
  • CloudStack API behavior notes

jeanvetorello avatar Oct 17 '25 19:10 jeanvetorello

@jeanvetorello There was already work done for service offerings around fixed, constrained and unconstrained.

Please see the following for discussions around this. #138 #113 #77

@poddm can you review this and give your input on this as your commit a while back took a different aproach and I know we had this discussion about this before. (see links above)

@DaanHoogland Also, what is your opinion on this as we need to pick a path and stick to it. We shouldn't have multiple resource files out there for the same thing

We need to make sure we are using the most recent framework/sdk as well.

This is what was decided back then on method to split up the resources

image image

CodeBleu avatar Oct 17 '25 23:10 CodeBleu

@DaanHoogland Also, what is your opinion on this as we need to pick a path and stick to it. We shouldn't have multiple resource files out there for the same thing

@CodeBleu , I completely agree with the blanket statement, but am sure I am missing some context. on first sight I thought you were talking about production code vs test code, but I see no issue there. Can you explain?

DaanHoogland avatar Oct 21 '25 13:10 DaanHoogland

@DaanHoogland Also, what is your opinion on this as we need to pick a path and stick to it. We shouldn't have multiple resource files out there for the same thing

@CodeBleu , I completely agree with the blanket statement, but am sure I am missing some context. on first sight I thought you were talking about production code vs test code, but I see no issue there. Can you explain?

@DaanHoogland There are both resource_cloudstack_service_offering.go and service_offering_*.go files in the code for service offerings. They both take a different approach and this PR is updating what I thought was the old way?
See https://github.com/apache/cloudstack-terraform-provider/pull/138 https://github.com/apache/cloudstack-terraform-provider/pull/113 https://github.com/apache/cloudstack-terraform-provider/pull/77 for more context.

CodeBleu avatar Oct 21 '25 13:10 CodeBleu

@CodeBleu, I think you're right. We should deprecated this resource.

  • https://developer.hashicorp.com/terraform/plugin/sdkv2/best-practices/deprecations#provider-data-source-or-resource-removal

I can open a PR if thats whats decided.

poddm avatar Oct 21 '25 15:10 poddm

There are both resource_cloudstack_service_offering.go and service_offering_*.go files in the code for service offerings.

makes sense, @CodeBleu .

I can open a PR if thats whats decided.

thanks @poddm .

it makes no sense to maintain two SDKs for the same API. Will we then defer merging this? or is this a parallel effort?

DaanHoogland avatar Oct 21 '25 16:10 DaanHoogland

I need to remove old duplicate service offering files to pass all tests. Should I proceed?

jeanvetorello avatar Oct 22 '25 13:10 jeanvetorello