Cancel rolling upgrades before deleting VMSS & extensions #18973

harshavmb · 2022-10-25T15:26:12Z

This is redo work of PR-15634. The old PR was having conflicts & expects an additional argument to cancel before deleting VMSS & extensions.

Since destroy call is a clear intention of deleting VMSS & associated resources like extensions, rolling upgrades could be cancelled as they are trivial.

As of now, we write destroy time provisioners to cancel rolling upgrades which are error prone. We hope this will improve the way VMSS & extensions are deleted in a clean manner.

Our problem statement had been explained to Gauthier Donikian, Hashicorp few months ago. We expect some feedback from Hashicorp team.

Thanks,
Harsha

…adeus.net/iac/terraform-provider-azurerm into feature/SACP-47873-add-internal-ci

Merge in IAC/terraform-provider-azurerm from feature/SACP-47873-add-internal-ci to master * commit '6d1953a834749477c5693b3fef7a441b1ef0869a': SACP-47573: Fallback to Amadeus versioning SACP-47573: Adding internal CI Changes Changes with parallelism=2 SACP:47873: changes SACP:47873: changes SACP:47873: changes SACP:47873: changes SACP:47873: changes SACP:47873: changes SACP:47873: Removing stage from Jenkinsfile Add internal CI

…g VMSS Merge in IAC/terraform-provider-azurerm from feature/SACP-47877-redo-vmss-pr to master * commit '8ef20ab8515d3ccb45557aea51dfab5e31e6be3f': SACP-47877: Rolling Upgrade cancelled before deleting VMSS

…ovider-azurerm

…ider-azurerm

katbyte · 2022-10-25T16:19:14Z

thanks for re-opening this @harshavmb - its on our list to review soon - as this is supersedes #15634 i'm going to close that one out

harshavmb · 2022-11-01T20:43:30Z

Hi @katbyte @tombuildsstuff,

I've got another idea today to take out the rolling_upgrade_policy outside of azurerm_linux_virtual_machine_scale_set & have a new resource. This way, there is no need of writing explicit cancel rolling upgrade calls made to ARM.

Are you okay with this approach?

harshavmb · 2022-11-28T08:55:16Z

Hi @katbyte / @tombuildsstuff ,

Any update on this PR? We need some update on this before we end up with some code conflicts. The last PR (already closed) was also left un-reviewed for longtime.

evaroquaux · 2022-12-11T12:00:13Z

Hello,
Any update on this contribution?
Still highly expected, for a long time...
Thank you

harshavmb · 2023-08-07T11:43:18Z

Hi @stephybun ,

Your feedback is highly appreciated. We have been managing all these times with ugly destroy provisioners. If you think this PR helps, please share your feedback as this has been in your review queue for many months.

internal/services/compute/client/helper.go

stephybun

Thanks for this PR @harshavmb and apologies being unable to get to this sooner.

I think this behaviour makes sense and is something we can incorporate into the resource. I've left suggestions which will bring this functionality more in-line with the rest of the provider. These will need to be made before we can consider merging this.

We also need to add a test for all three resources, this is a bit tricky since we need to simulate the behaviour of deleting the VMSS when rolling upgrades are happening in an automated fashion. It isn't blatantly obvious how to do that unless you're very familiar with our testing framework which is why I've gone ahead and written one, unfortunately I'm unable to push it to your branch since I lack permission.

I have the test and all of the suggested changes on my branch, which in the interest of time I could open as a separate PR to supersede this one. Alternatively I can provide the test and test config to you in another way. Let me know which you prefer.

stephybun · 2023-08-16T10:13:52Z

internal/services/compute/client/helper.go

+	if err != nil && !(future.Response().StatusCode == http.StatusConflict && strings.Contains(err.Error(), "There is no ongoing Rolling Upgrade to cancel.")) {
+		return fmt.Errorf("error cancelling rolling upgrades of Virtual Machine Scale Set %q (Resource Group %q): %+v", vmScaleSetName, resourceGroupName, err)
+	}


Thanks for the clarification. This actually resulted in a crash for me when when trying to reproduce the case described in the comments above. In addition matching on the error messages tends to be brittle since that is something that can change very easily in the API.

A more robust way to check whether to call the cancel function for rolling upgrades would be to query whether there are any ongoing rolling upgrades first. I've left comments in-line with what that would look like.

stephybun · 2023-08-16T11:39:19Z

internal/services/compute/client/helper.go

+	"strings"
+)
+
+func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, resourceGroupName string, vmScaleSetName string) error {


Let's just pass in the VMSS resource ID here since that contains all the info we need

Suggested change

func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, resourceGroupName string, vmScaleSetName string) error {

func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, id parse.VirtualMachineScaleSetId) error {

stephybun · 2023-08-16T11:41:51Z

internal/services/compute/client/helper.go

+)
+
+func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, resourceGroupName string, vmScaleSetName string) error {
+	future, err := c.VMScaleSetRollingUpgradesClient.Cancel(ctx, resourceGroupName, vmScaleSetName)


There is an API call to retrieve the latest rolling upgrades, I think we should do that here and return nil if no rolling upgrade is fine, i.e. there is no need to cancel and trigger the 409 error described down below, and if there is an existing rolling upgrade then we proceed to cancel - all other errors should be caught and surfaced

Suggested change

future, err := c.VMScaleSetRollingUpgradesClient.Cancel(ctx, resourceGroupName, vmScaleSetName)

resp, err := c.VMScaleSetRollingUpgradesClient.GetLatest(ctx, id.ResourceGroup, id.Name)

if err != nil {

if utils.ResponseWasNotFound(resp.Response) {

return nil

}

return fmt.Errorf("retrieving rolling updates for %s: %+v", id, err)

}

future, err := c.VMScaleSetRollingUpgradesClient.Cancel(ctx, resourceGroupName, vmScaleSetName)

stephybun · 2023-08-16T11:42:51Z

internal/services/compute/client/helper.go

+	// If rolling upgrades haven't been run (when VMSS are just provisioned with rolling upgrades but no extensions, auto-scaling are run )
+	// we can not cancel rolling upgrades
+	// API call :: GET https://management.azure.com/subscriptions/{subId}/resourceGroups/{rgName}/providers/Microsoft.Compute/virtualMachineScaleSets/{vmSSName}/rollingUpgrades/latest?api-version=2021-07-01
+	// Azure API throws 409 conflict error saying "The entity was not found in this Azure location."
+	// If the above error message matches, we identify and move forward to delete the VMSS
+	// in all other cases, it just cancels the rolling upgrades and move ahead to delete the VMSS
+	if err != nil && !(future.Response().StatusCode == http.StatusConflict && strings.Contains(err.Error(), "There is no ongoing Rolling Upgrade to cancel.")) {
+		return fmt.Errorf("error cancelling rolling upgrades of Virtual Machine Scale Set %q (Resource Group %q): %+v", vmScaleSetName, resourceGroupName, err)
+	}


Means none of this is necessary since we aren't hitting this behaviour anymore

Suggested change

// If rolling upgrades haven't been run (when VMSS are just provisioned with rolling upgrades but no extensions, auto-scaling are run )

// we can not cancel rolling upgrades

// API call :: GET https://management.azure.com/subscriptions/{subId}/resourceGroups/{rgName}/providers/Microsoft.Compute/virtualMachineScaleSets/{vmSSName}/rollingUpgrades/latest?api-version=2021-07-01

// Azure API throws 409 conflict error saying "The entity was not found in this Azure location."

// If the above error message matches, we identify and move forward to delete the VMSS

// in all other cases, it just cancels the rolling upgrades and move ahead to delete the VMSS

if err != nil && !(future.Response().StatusCode == http.StatusConflict && strings.Contains(err.Error(), "There is no ongoing Rolling Upgrade to cancel.")) {

return fmt.Errorf("error cancelling rolling upgrades of Virtual Machine Scale Set %q (Resource Group %q): %+v", vmScaleSetName, resourceGroupName, err)

}

stephybun · 2023-08-16T11:44:17Z

internal/services/compute/client/helper.go

+	if err := future.WaitForCompletionRef(ctx, c.VMScaleSetExtensionsClient.Client); err != nil && !(future.Response().StatusCode == http.StatusConflict && strings.Contains(err.Error(), "There is no ongoing Rolling Upgrade to cancel.")) {
+		return fmt.Errorf("waiting for cancelling rolling upgrades of Virtual Machine Scale Set %q (Resource Group %q): %+v", vmScaleSetName, resourceGroupName, err)
+	}
+	log.Printf("[DEBUG] cancelled Virtual Machine Scale Set Rolling Upgrades %q (Resource Group %q).", vmScaleSetName, resourceGroupName)
+	return nil


And we can simplify the condition here since it should be prevented by the check retrieving any rolling upgrades above

Suggested change

if err := future.WaitForCompletionRef(ctx, c.VMScaleSetExtensionsClient.Client); err != nil && !(future.Response().StatusCode == http.StatusConflict && strings.Contains(err.Error(), "There is no ongoing Rolling Upgrade to cancel.")) {

return fmt.Errorf("waiting for cancelling rolling upgrades of Virtual Machine Scale Set %q (Resource Group %q): %+v", vmScaleSetName, resourceGroupName, err)

}

log.Printf("[DEBUG] cancelled Virtual Machine Scale Set Rolling Upgrades %q (Resource Group %q).", vmScaleSetName, resourceGroupName)

return nil

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

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

}

log.Printf("[DEBUG] cancelled Virtual Machine Scale Set Rolling Upgrades for %s.", id)

return nil

stephybun · 2023-08-16T11:45:23Z

internal/services/compute/linux_virtual_machine_scale_set_resource.go

+	err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.Name)
+	if err != nil {
+		return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.Name, id.ResourceGroup, err)
+	}


Let's simplify this to

Suggested change

err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.Name)

if err != nil {

return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.Name, id.ResourceGroup, err)

}

if err := meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, *id); err != nil {

fmt.Errorf("cancelling rolling upgrades for %s: %+v", *id, err)

}

stephybun · 2023-08-16T11:46:43Z

internal/services/compute/virtual_machine_scale_set_extension_resource.go

+	err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.VirtualMachineScaleSetName)
+	if err != nil {
+		return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.VirtualMachineScaleSetName, id.ResourceGroup, err)
+	}


Same here

Suggested change

err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.VirtualMachineScaleSetName)

if err != nil {

return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.VirtualMachineScaleSetName, id.ResourceGroup, err)

}

if err := meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, *id); err != nil {

fmt.Errorf("cancelling rolling upgrades for %s: %+v", *id, err)

}

stephybun · 2023-08-16T11:47:22Z

internal/services/compute/windows_virtual_machine_scale_set_resource.go

+	err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.Name)
+	if err != nil {
+		return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.Name, id.ResourceGroup, err)
+	}


And here

Suggested change

err = meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, id.ResourceGroup, id.Name)

if err != nil {

return fmt.Errorf("error while cancelling rolling upgrade during destroy phase in Linux Virtual Machine Scale Set %q (Resource Group %q) : %+v", id.Name, id.ResourceGroup, err)

}

if err := meta.(*clients.Client).Compute.CancelRollingUpgradesBeforeDeletion(ctx, *id); err != nil {

fmt.Errorf("cancelling rolling upgrades for %s: %+v", *id, err)

}

harshavmb · 2023-08-16T15:00:40Z

Thanks for this PR @harshavmb and apologies being unable to get to this sooner.

I think this behaviour makes sense and is something we can incorporate into the resource. I've left suggestions which will bring this functionality more in-line with the rest of the provider. These will need to be made before we can consider merging this.

We also need to add a test for all three resources, this is a bit tricky since we need to simulate the behaviour of deleting the VMSS when rolling upgrades are happening in an automated fashion. It isn't blatantly obvious how to do that unless you're very familiar with our testing framework which is why I've gone ahead and written one, unfortunately I'm unable to push it to your branch since I lack permission.

I have the test and all of the suggested changes on my branch, which in the interest of time I could open as a separate PR to supersede this one. Alternatively I can provide the test and test config to you in another way. Let me know which you prefer.

Hi @stephybun ,

Many Thanks for your review. If you have all the changes you needed to push from your branch, I'm fine with it. This PR can be closed when you create the PR.

Also, I must admit I can't write the three test cases you mentioned.

Thanks again for your help. Very much appreciated.

Thanks,
Harsha

stephybun · 2023-08-16T15:51:09Z

@harshavmb I'll get a PR open in the morning, I think it may be cutting it a bit short to make it into this week's release, I need to do some additional testing around the extensions resource but it should make it in the week after.

Apologies again for the long delay on this one.

stephybun · 2023-08-17T07:12:41Z

Closing in favour of #22991

github-actions · 2024-05-17T02:09:26Z

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.

harshavmb added 24 commits September 27, 2022 14:01

Add internal CI

5a324aa

SACP:47873: Removing stage from Jenkinsfile

d3400ef

SACP:47873: changes

b9862b9

SACP:47873: changes

2ddd4ed

SACP:47873: changes

9912820

SACP:47873: changes

44c498b

SACP:47873: changes

58d807d

SACP:47873: changes

1bacd30

with parallelism=2

5a39c53

Changes

38d86b6

Changes

1f9b0fa

Merge branch 'feature/SACP-47873-add-internal-ci' of ssh://git.rnd.am…

985cc04

…adeus.net/iac/terraform-provider-azurerm into feature/SACP-47873-add-internal-ci

SACP-47573: Adding internal CI

affd5eb

SACP-47573: Resolving conflicts

6c2def3

SACP-47573: Fallback to Amadeus versioning

6d1953a

SACP-47873: Removing .terraformrc file & limiting parallel tests

68dd089

SACP-47873: Limiting parallel builds to 5

14ebf71

SACP-47877: Rolling Upgrade cancelled before deleting VMSS

8ef20ab

Pull request #2: SACP-47877: Rolling Upgrade cancelled before deletin…

3516879

…g VMSS Merge in IAC/terraform-provider-azurerm from feature/SACP-47877-redo-vmss-pr to master * commit '8ef20ab8515d3ccb45557aea51dfab5e31e6be3f': SACP-47877: Rolling Upgrade cancelled before deleting VMSS

Merge branch 'main' of https://github.com/AmadeusITGroup/terraform-pr…

876a8da

…ovider-azurerm

Merge branch 'master' of ssh://git.rnd.amadeus.net/iac/terraform-prov…

6c4090f

…ider-azurerm

pushing changes to github branch

8e7ddd3

reverting changes to .goreleaser.yml

e14adc0

katbyte mentioned this pull request Oct 25, 2022

Cancel RollingUpgrades before deleting VMSS resources #15634

Closed

tombuildsstuff self-assigned this Nov 7, 2022

stephybun assigned stephybun and unassigned tombuildsstuff Jun 13, 2023

stephybun reviewed Aug 14, 2023

View reviewed changes

internal/services/compute/client/helper.go Show resolved Hide resolved

github-actions bot added the waiting-response label Aug 14, 2023

stephybun requested changes Aug 16, 2023

View reviewed changes

stephybun added enhancement service/virtual-machine-extensions service/vmss Virtual Machine Scale Sets labels Aug 16, 2023

github-actions bot removed the waiting-response label Aug 16, 2023

stephybun mentioned this pull request Aug 17, 2023

azurerm_linux/windows_virtual_machine_scale_set - cancel rolling upgrades before deleting resource #22991

Merged

stephybun closed this Aug 17, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024

harshavmb deleted the redo-vm-ss-pr branch May 17, 2024 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel rolling upgrades before deleting VMSS & extensions #18973

Cancel rolling upgrades before deleting VMSS & extensions #18973

harshavmb commented Oct 25, 2022 •

edited

Loading

katbyte commented Oct 25, 2022

harshavmb commented Nov 1, 2022 •

edited

Loading

harshavmb commented Nov 28, 2022

evaroquaux commented Dec 11, 2022

harshavmb commented Aug 7, 2023

stephybun left a comment

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

stephybun Aug 16, 2023

harshavmb commented Aug 16, 2023

stephybun commented Aug 16, 2023

stephybun commented Aug 17, 2023

github-actions bot commented May 17, 2024

	func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, resourceGroupName string, vmScaleSetName string) error {
	func (c *Client) CancelRollingUpgradesBeforeDeletion(ctx context.Context, id parse.VirtualMachineScaleSetId) error {

Cancel rolling upgrades before deleting VMSS & extensions #18973

Cancel rolling upgrades before deleting VMSS & extensions #18973

Conversation

harshavmb commented Oct 25, 2022 • edited Loading

katbyte commented Oct 25, 2022

harshavmb commented Nov 1, 2022 • edited Loading

harshavmb commented Nov 28, 2022

evaroquaux commented Dec 11, 2022

harshavmb commented Aug 7, 2023

stephybun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavmb commented Aug 16, 2023

stephybun commented Aug 16, 2023

stephybun commented Aug 17, 2023

github-actions bot commented May 17, 2024

harshavmb commented Oct 25, 2022 •

edited

Loading

harshavmb commented Nov 1, 2022 •

edited

Loading