Add Linux VM extension bootstrap script and conditions by CecileRobertMichon · Pull Request #1232 · kubernetes-sigs/cluster-api-provider-azure

CecileRobertMichon · 2021-03-12T02:34:46Z

What type of PR is this?

What this PR does / why we need it: Implements bootstrap failure detection using conditions and VM extensions as proposed in #1076.

This is what a user will see when an AzureMachine has been successfully provisioned but has not yet finished running the bootstrap script:

$ k get azuremachine default-template-md-0-w78jt
NAME                                       READY   STATE
default-template-md-0-w78jt                false   Updating

status:
    conditions:
  - lastTransitionTime: "2021-03-17T00:02:14Z"
    reason: VMUpdating
    severity: Info
    status: "False"
    type: Ready
  - lastTransitionTime: "2021-03-17T00:02:14Z"
    reason: BootstrapInProgress
    severity: Info
    status: "False"
    type: BoostrapSucceeded
  - lastTransitionTime: "2021-03-17T00:02:14Z"
    reason: VMUpdating
    severity: Info
    status: "False"
    type: VMRunning

After the bootstrap script has executed successfully, the AzureMachine status shows:

$ k get azuremachine default-template-md-0-w78jt
NAME                                       READY   STATE
default-template-md-0-w78jt                true    Succeeded

status:
  conditions:
  - lastTransitionTime: "2021-03-16T23:56:17Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2021-03-16T23:56:17Z"
    status: "True"
    type: BoostrapSucceeded
  - lastTransitionTime: "2021-03-16T23:56:17Z"
    status: "True"
    type: VMRunning

If for some reason the bootstrap script fails to execute, the status will show:

$ k get azuremachines default-template-md-0-ppjmh
NAME                                       READY   STATE
default-template-md-0-ppjmh                false   Failed

  conditions:
  - lastTransitionTime: "2021-03-17T00:22:39Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2021-03-17T00:22:39Z"
    reason: BootstrapFailed
    severity: Error
    status: "False"
    type: BoostrapSucceeded
  - lastTransitionTime: "2021-03-17T00:22:39Z"
    status: "True"
    type: VMRunning

Also renames type "VMState" to "ProvisioningState" to be more generic and match Azure naming. This is a breaking change for anyone importing CAPZ types, but should have no impact on user templates and existing CRDs (conversion from v1alpha3 to v1alpha4 is auto-generated).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #603

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Add Linux VM extension bootstrap script and conditions
Renames type "VMState" to "ProvisioningState"

shysank · 2021-03-16T18:36:31Z

Just realized it's a wip pr, please feel free to ignore the review comments that are not relevant.

CecileRobertMichon · 2021-03-16T19:41:03Z

-		return err
-	}
-	_, err = future.Result(ac.vmextensions)
+	_, err := ac.vmextensions.CreateOrUpdate(ctx, resourceGroupName, vmName, name, parameters)


@devigned thoughts on this approach of doing "async" reconciliation? Is it significantly less efficient than storing the future or since we only care about eventually getting the extension status do you think it's appropriate in this situation?

I'm thinking in the future we can move this over to VM CreateOrUpdate once that's async. For now the extension could be a long-running operation (timeout is 20 minutes) so we don't want that to be part of a blocking operation

I think this should be fine. Though, it would be easy to call this multiple times if one is not tracking an ongoing operation via a secret. With the AMP future, we check to see if the operation is done prior to proceeding, which the Azure SDK for Go can help with. In this case, you'd be responsible for managing that state.

CecileRobertMichon · 2021-03-16T23:39:38Z

+	switch infrav1.ProvisioningState(provisioningState) {
+	case infrav1.Succeeded:
+		m.V(4).Info("extension provisioning state is succeeded", "vm extension", extensionName, "scale set", m.Name())
+		conditions.MarkTrue(m.AzureMachinePool, infrav1.BootstrapSucceededCondition)


it'd be nice to set conditions per VMSS VM, hopefully that gets easier with #819

Right now it's at the AzureMachinePool.Status level:

status: conditions: - lastTransitionTime: "2021-03-16T23:53:39Z" status: "True" type: BoostrapSucceeded - lastTransitionTime: "2021-03-16T23:49:37Z" status: "True" type: ScaleSetRunning

CecileRobertMichon · 2021-03-17T00:05:14Z

Thanks for the initial review @shysank! I've addressed most of your comments. I still have docs and unit tests as TODOs.

…ing VM extension script

k8s-ci-robot · 2021-03-19T17:46:18Z

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`638c14f`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

CecileRobertMichon · 2021-03-22T16:27:02Z

This is ready for review now

nader-ziada · 2021-03-22T19:24:32Z

when I try running it, I don't see the Updating phase

NAME                           READY   STATE
nz-test1-control-plane-wk926   true    Succeeded
nz-test1-md-0-gc5gc

then goes directly to

NAME                           READY   STATE
nz-test1-control-plane-wk926   true    Succeeded
nz-test1-md-0-gc5gc            true    Succeeded

not sure why yet, will dig into it a bit more

update:

I see this error in my logs

[manager] E0322 19:19:04.361187       1 azuremachine_controller.go:310] controllers/AzureMachine "msg"="transient failure to reconcile AzureMachine, retrying" "error"="unable to create vm extension: reconcile error occurred that can be recovered. Object will be requeued after 30s The actual error is: extension still provisioning" "AzureCluster"="nz-test1" "azureMachine"="nz-test1-md-0-gc5gc" "cluster"="nz-test1" "machine"="nz-test1-md-0-67888c96b8-h94st" "namespace"="default" "name"="nz-test1-md-0-gc5gc"

devigned

Looks great. Only had a minor comment about async APIs.

devigned · 2021-03-23T11:52:56Z

-		return err
-	}
-	_, err = future.Result(ac.vmextensions)
+	_, err := ac.vmextensions.CreateOrUpdate(ctx, resourceGroupName, vmName, name, parameters)


I think this should be fine. Though, it would be easy to call this multiple times if one is not tracking an ongoing operation via a secret. With the AMP future, we check to see if the operation is done prior to proceeding, which the Azure SDK for Go can help with. In this case, you'd be responsible for managing that state.

CecileRobertMichon · 2021-03-24T01:46:13Z

when I try running it, I don't see the Updating phase

@nader-ziada I wonder if you're doing something differently, this is what I see when scaling up:

Every 2.0s: kubectl get azuremachines                                                                                                 
Ceciles-MacBook-Pro.local: Tue Mar 23 18:42:51 2021

NAME                                   READY   STATE
default-template-control-plane-5zq49   true    Succeeded
default-template-md-0-b9cpb            true    Succeeded
default-template-md-0-bnq44
default-template-md-0-vkd4b            true    Succeeded

Every 2.0s: kubectl get azuremachines                                                                                                 
Ceciles-MacBook-Pro.local: Tue Mar 23 18:43:08 2021

NAME                                   READY   STATE
default-template-control-plane-5zq49   true    Succeeded
default-template-md-0-b9cpb            true    Succeeded
default-template-md-0-bnq44            false   Updating
default-template-md-0-vkd4b            true    Succeeded

Every 2.0s: kubectl get azuremachines                                                                                                 
Ceciles-MacBook-Pro.local: Tue Mar 23 18:43:38 2021

NAME                                   READY   STATE
default-template-control-plane-5zq49   true    Succeeded
default-template-md-0-b9cpb            true    Succeeded
default-template-md-0-bnq44            true    Succeeded
default-template-md-0-vkd4b            true    Succeeded

That first step I'm hoping we can change to default-template-md-0-bnq44 false Creating as part of #1067 (by returning and setting status before the VM is done creating)

nader-ziada · 2021-03-25T15:11:45Z

I tried it again and seems to work as expected

NAME                                READY   STATE
nz-test1-control-plane-pq8lt        true    Succeeded
nz-test1-md-0-qs2nf                 false   Updating
nz-test1-md-0-qs2nf                 false   Updating
nz-test1-md-0-qs2nf                 true    Succeeded

nader-ziada · 2021-03-25T15:12:12Z

/lgtm

@devigned any comments?

devigned

/lgtm
/approve

k8s-ci-robot · 2021-03-25T16:57:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [devigned]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from cpanato and nader-ziada March 12, 2021 02:34

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Mar 12, 2021

CecileRobertMichon changed the title ~~[WIP] Add VM extension bootstrap script and~~ [WIP] Add Linux VM extension bootstrap script and conditions Mar 12, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2021

CecileRobertMichon force-pushed the extension-script branch from 6828aea to 1ddcd95 Compare March 15, 2021 17:05

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2021

CecileRobertMichon force-pushed the extension-script branch from 1ddcd95 to 0366cd4 Compare March 16, 2021 18:02

shysank reviewed Mar 16, 2021

View reviewed changes

CecileRobertMichon force-pushed the extension-script branch from 0366cd4 to 240c5b5 Compare March 16, 2021 19:35

CecileRobertMichon commented Mar 16, 2021

View reviewed changes

CecileRobertMichon force-pushed the extension-script branch 2 times, most recently from 436e638 to e700487 Compare March 16, 2021 20:02

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2021

CecileRobertMichon force-pushed the extension-script branch from c861661 to e7e6060 Compare March 16, 2021 23:36

CecileRobertMichon commented Mar 16, 2021

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 16, 2021

CecileRobertMichon force-pushed the extension-script branch from e7e6060 to ad702de Compare March 18, 2021 00:31

alexeldeib reviewed Mar 18, 2021

View reviewed changes

Comment thread api/v1alpha4/types.go Outdated

CecileRobertMichon force-pushed the extension-script branch 2 times, most recently from dea6219 to 669b1c0 Compare March 18, 2021 23:20

CecileRobertMichon changed the title ~~[WIP] Add Linux VM extension bootstrap script and conditions~~ Add Linux VM extension bootstrap script and conditions Mar 18, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2021

CecileRobertMichon force-pushed the extension-script branch from 669b1c0 to 7d5b441 Compare March 19, 2021 00:25

CecileRobertMichon mentioned this pull request Mar 19, 2021

docs: add troubleshooting examples debugging missing nodes #831

Closed

3 tasks

Cecile Robert-Michon added 3 commits March 19, 2021 10:44

Set AzureMachine and AzureMachinePool BootstrapSucceeded condition us…

9027dde

…ing VM extension script

⚠️ Rename VMState to ProvisioningState

342ef15

Update troubleshooting docs

638c14f

CecileRobertMichon force-pushed the extension-script branch from 7d5b441 to 638c14f Compare March 19, 2021 17:44

CecileRobertMichon mentioned this pull request Mar 22, 2021

Don't useExperimentalRetryJoin #1029

Merged

3 tasks

devigned reviewed Mar 23, 2021

View reviewed changes

k8s-ci-robot assigned nader-ziada Mar 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2021

devigned approved these changes Mar 25, 2021

View reviewed changes

k8s-ci-robot assigned devigned Mar 25, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2021

k8s-ci-robot merged commit 4f3f98b into kubernetes-sigs:master Mar 25, 2021

k8s-ci-robot added this to the v0.5.0 milestone Mar 25, 2021

CecileRobertMichon deleted the extension-script branch February 17, 2023 22:59

Conversation

CecileRobertMichon commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shysank commented Mar 16, 2021

Uh oh!

CecileRobertMichon Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Mar 19, 2021

Choose a reason for hiding this comment

Uh oh!

devigned Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Mar 17, 2021

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CecileRobertMichon commented Mar 22, 2021

Uh oh!

nader-ziada commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

devigned Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

CecileRobertMichon commented Mar 24, 2021

Uh oh!

nader-ziada commented Mar 25, 2021

Uh oh!

nader-ziada commented Mar 25, 2021

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CecileRobertMichon commented Mar 12, 2021 •

edited

Loading

k8s-ci-robot commented Mar 19, 2021 •

edited

Loading

nader-ziada commented Mar 22, 2021 •

edited

Loading