New resources `azurerm_capacity_reservation_group` and `azurerm_capacity_reservation` #16464

myc2h6o · 2022-04-20T09:11:56Z

Implementing On-demand Capacity Reservation
VM resources also need an additional property capacityReservationGroup Id to specify the CapacityReserviceGroup to put the VM, I'll add it in a separate pr

myc2h6o · 2022-04-20T13:28:03Z

Test result:

tombuildsstuff

Thanks for opening this PR - I've taken a look through and left some comments inline, if we can fix those up then we should be able to take another look here.

Thanks!

tombuildsstuff · 2022-04-20T15:42:31Z

internal/services/compute/capacity_reservation_resource.go

+			"resource_group_name": azure.SchemaResourceGroupName(),
+
+			"location": azure.SchemaLocation(),
+
+			"capacity_reservation_group_name": {
+				Type:         pluginsdk.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.StringIsNotEmpty,
+			},


this should be passing the Capacity Reservation Group ID and parsing these values instead

Removed these three and added capacity_reservation_group_id, including location which must be same with the capacity reservation group.

tombuildsstuff · 2022-04-20T15:43:25Z

internal/services/compute/capacity_reservation_resource.go

+	if v, ok := d.GetOk("zone"); ok {
+		zones := []string{v.(string)}
+		parameters.Zones = &zones
+	}


this should be using the common Expand/Flatten functions for Zones: https://github.com/hashicorp/go-azure-helpers/blob/main/resourcemanager/zones/expand.go

The API exposes zones, however, it can only accept one zone, so used the non-common way to expand/flatten this. Similar to managed_disk.zone. Updated a little bit in the new commit though to make it more consistent

terraform-provider-azurerm/internal/services/compute/managed_disk_resource.go

Lines 472 to 476 in 7c27c45

if zone, ok := d.GetOk("zone"); ok {

createDisk.Zones = &[]string{

zone.(string),

}

}

tombuildsstuff · 2022-04-20T15:43:41Z

internal/services/compute/capacity_reservation_resource.go

+	resp, err := client.Get(ctx, id.ResourceGroup, id.CapacityReservationGroupName, id.Name, "")
+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			log.Printf("[INFO] Capacity Reservation %q does not exist - removing from state", d.Id())


Suggested change

log.Printf("[INFO] Capacity Reservation %q does not exist - removing from state", d.Id())

log.Printf("[INFO] %s was not found - removing from state", *id)

Thanks for pointing out, updated

tombuildsstuff · 2022-04-20T15:43:57Z

internal/services/compute/capacity_reservation_resource.go

+	if zones := resp.Zones; zones != nil && len(*zones) > 0 {
+		d.Set("zone", (*zones)[0])
+	}


this should be using the common Expand/Flatten functions for Zones: https://github.com/hashicorp/go-azure-helpers/blob/main/resourcemanager/zones/flatten.go

The API exposes zones, however, it can only accept one zone, so used the non-common way to expand/flatten this. Similar to managed_disk.zone. Updated a little bit in the new commit though to make it more consistent

terraform-provider-azurerm/internal/services/compute/managed_disk_resource.go

Lines 825 to 830 in 7c27c45

zone := ""

if resp.Zones != nil && len(*resp.Zones) > 0 {

z := *resp.Zones

zone = z[0]

}

d.Set("zone", zone)

tombuildsstuff · 2022-04-20T15:45:02Z

internal/services/compute/capacity_reservation_resource.go

+	future, err := client.Delete(ctx, id.ResourceGroup, id.CapacityReservationGroupName, id.Name)
+	if err != nil {
+		return fmt.Errorf("deleting %s: %+v", id, err)
+	}
+
+	if err := future.WaitForCompletionRef(ctx, client.Client); err != nil {
+		return fmt.Errorf("waiting for deletion of %s: %+v", id, err)
+	}
+
+	// API has bug, which appears to be eventually consistent.
+	log.Printf("[DEBUG] Waiting for %s to be fully deleted..", *id)


where's the Swagger bug for this? There's no point using the WaitForCompletion if this API is broken

Suggested change

future, err := client.Delete(ctx, id.ResourceGroup, id.CapacityReservationGroupName, id.Name)

if err != nil {

return fmt.Errorf("deleting %s: %+v", id, err)

}

if err := future.WaitForCompletionRef(ctx, client.Client); err != nil {

return fmt.Errorf("waiting for deletion of %s: %+v", id, err)

}

// API has bug, which appears to be eventually consistent.

log.Printf("[DEBUG] Waiting for %s to be fully deleted..", *id)

if _, err := client.Delete(ctx, id.ResourceGroup, id.CapacityReservationGroupName, id.Name); err != nil {

return fmt.Errorf("deleting %s: %+v", id, err)

}

// API has bug, which appears to be eventually consistent.

log.Printf("[DEBUG] Waiting for %s to be fully deleted..", *id)

Have opened an issue within Azure spec repo, updated in code comment as well
Azure/azure-rest-api-specs#18767

tombuildsstuff · 2022-04-20T15:54:01Z

internal/services/compute/capacity_reservation_resource.go

+			"name": {
+				Type:     pluginsdk.TypeString,
+				Required: true,
+				ForceNew: true,


we should validate the name

Have added the validation for name

internal/services/compute/capacity_reservation_resource.go

tombuildsstuff · 2022-04-20T15:54:32Z

internal/services/compute/capacity_reservation_group_resource.go

+	zones := zones.Expand(d.Get("zones").(*schema.Set).List())
+	if len(zones) > 0 {
+		parameters.Zones = &zones
+	}


this should be using the standard Expand functions for Zones

different from the other resource in this change, in line 87 (before the new commit), already used the standard zones.Expand

tombuildsstuff · 2022-04-20T15:54:48Z

internal/services/compute/capacity_reservation_group_resource.go

+	resp, err := client.Get(ctx, id.ResourceGroup, id.Name, "")
+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			log.Printf("[INFO] Capacity Reservation Group %q does not exist - removing from state", d.Id())


Suggested change

log.Printf("[INFO] Capacity Reservation Group %q does not exist - removing from state", d.Id())

log.Printf("[INFO] %s was not found - removing from state", *id)

tombuildsstuff · 2022-04-20T15:55:14Z

internal/services/compute/capacity_reservation_group_resource.go

+	if _, err := client.Delete(ctx, id.ResourceGroup, id.Name); err != nil {
+		return fmt.Errorf("deleting %s: %+v", id, err)
+	}


so this is gone straight away? given the other API has issues, are there any API errors when deleting multiple reservations within a parent group here?

This api only has the non-future version: https://github.com/Azure/azure-sdk-for-go/blob/bf144f61fde04d2f29dd52d84f25e931cea67fd8/services/compute/mgmt/2021-11-01/compute/capacityreservationgroups.go#L120.
The api issue is that, although capacity reservation is deleted and not present in the Get response, capacity reservation group still needs some additional wait (about 5 seconds) to realize that, so added the wait in the capacity reservation deletion. Have put the detail in spec repo Azure/azure-rest-api-specs#18767

i don't see any wait? we souldn't be waiting and instead poll to check until its finished

Hi @katbyte The deletion for this resource capacity_reservation_group is synchrounous per sdk.

And the wait and poll is for the other resource capaicty_reservation below in this change, where the asynchronous sdk has bug

I just took another thought. The actual issue Azure/azure-rest-api-specs#18767 happens within deleting the parent resource capacity_reservation_group due to cache refresh issue. So instead of adding a wait on capacity_reservation, probably it's better to add a retry logic on the parent resource here itself. I'll update the pr and let you know once it's ready to be reviewed again

myc2h6o · 2022-04-24T16:08:34Z

Updated test result

myc2h6o · 2022-04-24T16:13:18Z

Hi @tombuildsstuff Thanks for reviewing the pr! I've updated the pr to resolve the comments, please take a look. Thanks!

katbyte · 2022-05-18T01:44:06Z

internal/services/compute/capacity_reservation_group_resource.go

+	if _, err := client.Delete(ctx, id.ResourceGroup, id.Name); err != nil {
+		return fmt.Errorf("deleting %s: %+v", id, err)
+	}


i don't see any wait? we souldn't be waiting and instead poll to check until its finished

myc2h6o · 2022-05-18T15:40:22Z

Hi @katbyte I've updated the delete operation of capacity_reservation_resource to use future.WaitForCompletionRef and move the workaround of Azure/azure-rest-api-specs#18767 to capacity_reservation_group_resource, by adding a delete retry after 1 minutes, as the cache of reservation list is refreshed in a few seconds.

myc2h6o · 2022-05-18T16:06:27Z

Test result:

katbyte

Thanks @myc2h6o - LGTM 🌻

katbyte · 2022-05-26T04:34:26Z

internal/services/compute/capacity_reservation_group_resource.go

+	// It takes several seconds to sync the cache of reservations list in Capacity Reservation Group. Delete operation requires the list to be empty, and fails before the cache sync is completed.
+	// Retry the delete operation after a minute as a workaround. Issue is tracked by: https://github.com/Azure/azure-rest-api-specs/issues/18767
+	if _, err := client.Delete(ctx, id.ResourceGroup, id.Name); err != nil {
+		time.Sleep(1 * time.Minute)


i just noticed this - we should not be sleeping. is there something you can check/poll on instead?

@katbyte I tried get the reservations list in the reservation group but unfortunately it changes to empty very soon after the reservation is deleted. However, even if it's empty, the deletion still needs some additional seconds to work

I saw the deletion in resource_group waits for 30 seconds in each polling, maybe we can wait for 30 seconds instead here as well? The internal cache is invisible from api but it shall be sync in less than 10 seconds

terraform-provider-azurerm/internal/services/resource/resource_group_resource.go

Lines 151 to 153 in 82de9ae

if len(nestedResourceIds) > 0 {

time.Sleep(30 * time.Second)

return pluginsdk.RetryableError(resourceGroupContainsItemsError(id.ResourceGroup, nestedResourceIds))

that is inside a retry function and acts as a delay between checking, it was a quick hack that was put in post 3.0 in 3.0.1 very quickly and should likely be changed/fixed to a state wait.

Could we instead put this inside a statewait where the retry and sleep logic is handled correctly? thanksa

@katbyte yes it's better to use the common way. I've moved the deletion workaround into a stateRefresh function, can you take a look? Thanks!

myc2h6o · 2022-06-01T14:46:47Z

Updated test result

katbyte

Thanks @myc2h6o - LGTM 👍

github-actions · 2022-06-10T09:07:44Z

This functionality has been released in v3.10.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

…ion_group_id` (#17236) Cont. of #16464 to support the Capacity Reservation Group on virtual machine. By specifying the Group ID, the virtual machine will be allocated to the specific Group which has reserved certain VM sizes in advance.

github-actions · 2022-07-11T02:35:06Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

New resources capacity_reservation_group and capacity_reservation

827c2c2

github-actions bot added documentation size/XL labels Apr 20, 2022

tombuildsstuff suggested changes Apr 20, 2022

View reviewed changes

katbyte added the waiting-response label Apr 21, 2022

Resolve comments

45a24dc

github-actions bot removed the waiting-response label Apr 24, 2022

Upadte doc

b848bfa

katbyte requested changes May 18, 2022

View reviewed changes

Update delete workaround

26114f7

Update sku with more quota in test

2eee0b7

myc2h6o force-pushed the capacity_reservation branch from 21ac0e5 to 2eee0b7 Compare May 18, 2022 15:59

myc2h6o mentioned this pull request May 23, 2022

new resource azurerm_capacity_reservation_group #16774

Closed

katbyte approved these changes May 26, 2022

View reviewed changes

katbyte requested changes May 26, 2022

View reviewed changes

katbyte added the waiting-response label May 26, 2022

use stateConf instead of sleep

9d1de89

myc2h6o force-pushed the capacity_reservation branch from ab6c6f6 to 9d1de89 Compare June 1, 2022 14:39

github-actions bot removed the waiting-response label Jun 1, 2022

katbyte approved these changes Jun 8, 2022

View reviewed changes

katbyte merged commit 5e1cb6d into hashicorp:main Jun 8, 2022

katbyte changed the title ~~New resources capacity_reservation_group and capacity_reservation~~ New resources azurerm_capacity_reservation_group and azurerm_capacity_reservation Jun 8, 2022

github-actions bot added this to the v3.10.0 milestone Jun 8, 2022

katbyte added a commit that referenced this pull request Jun 8, 2022

CHANGELOG.md for #16464

30671d3

myc2h6o deleted the capacity_reservation branch June 9, 2022 03:07

myc2h6o mentioned this pull request Jun 14, 2022

r\linux/windows_virtual_machine: add support for capacity_reservation_group_id #17236

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2022

	if zone, ok := d.GetOk("zone"); ok {
	createDisk.Zones = &[]string{
	zone.(string),
	}
	}

	log.Printf("[INFO] Capacity Reservation %q does not exist - removing from state", d.Id())
	log.Printf("[INFO] %s was not found - removing from state", *id)

	zone := ""
	if resp.Zones != nil && len(*resp.Zones) > 0 {
	z := *resp.Zones
	zone = z[0]
	}
	d.Set("zone", zone)

	if len(nestedResourceIds) > 0 {
	time.Sleep(30 * time.Second)
	return pluginsdk.RetryableError(resourceGroupContainsItemsError(id.ResourceGroup, nestedResourceIds))

New resources azurerm_capacity_reservation_group and azurerm_capacity_reservation #16464

New resources azurerm_capacity_reservation_group and azurerm_capacity_reservation #16464

Conversation

myc2h6o commented Apr 20, 2022 • edited Loading

myc2h6o commented Apr 20, 2022

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o commented Apr 24, 2022

myc2h6o commented Apr 24, 2022

Choose a reason for hiding this comment

myc2h6o commented May 18, 2022

myc2h6o commented May 18, 2022

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o commented Jun 1, 2022

katbyte left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2022

github-actions bot commented Jul 11, 2022

New resources `azurerm_capacity_reservation_group` and `azurerm_capacity_reservation` #16464

New resources `azurerm_capacity_reservation_group` and `azurerm_capacity_reservation` #16464

myc2h6o commented Apr 20, 2022 •

edited

Loading

myc2h6o Apr 24, 2022 •

edited

Loading

myc2h6o Apr 24, 2022 •

edited

Loading

myc2h6o Apr 24, 2022 •

edited

Loading