fix: undefined instance state on provisioning state failed #7750
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
area/provider/azure
comtalyst
force-pushed
the
comtalyst/fix-missing-instance-state-from-provisioning-state-failed
branch
from
445f5b9 to
d979d43
Compare
|
/cherry-pick cluster-autoscaler-release-1.32 |
|
@comtalyst: once the present PR merges, I will cherry-pick it on top of In response to this:
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-sigs/prow repository. |
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
| @@ -823,6 +824,8 @@ func instanceStatusFromProvisioningStateAndPowerState(resourceID string, provisi | |||
| case provisioningStateCreating: | |||
| status.State = cloudprovider.InstanceCreating | |||
| case provisioningStateFailed: | |||
| status.State = cloudprovider.InstanceRunning | |||
There was a problem hiding this comment.
this might be a bit more elegant is we simply declared the running state at the declaration, e.g. at L819:
status := &cloudprovider.InstanceStatus{
State: cloudprovider.InstanceRunning
}
And then we could remove the default case.
This way we don't suggest that the Failed provisioning state is meaningfully connected to the InstanceRunning state.
wdyt?
There was a problem hiding this comment.
In fact, I would want to make the mapping between the two explicit, given the small enough code size and the special complexity in this area (both Azure provisioning states and CAS instance states)---at least until we have higher confidence in the understanding. What do you think?
With that, I think we should refactor to not rely on the default case below and make sure all cases are considered explicitly. But that will be reconsidered later.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
| @@ -224,6 +225,8 @@ func (scaleSet *ScaleSet) instanceStatusFromVM(vm *compute.VirtualMachineScaleSe | |||
| case string(compute.GalleryProvisioningStateCreating): | |||
There was a problem hiding this comment.
Let's also cleanup these "Gallery*" constants at some point; even if there is no correct constant in compute (I don't think there is), let's have something reasonably named within the provider
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: comtalyst, jackfrancis, tallaxes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@comtalyst: new pull request created: #7754 In response to this:
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-sigs/prow repository. |
|
@comtalyst: new pull request created: #7755 In response to this:
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-sigs/prow repository. |
|
@comtalyst: new pull request created: #7756 In response to this:
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-sigs/prow repository. |
|
@comtalyst: new pull request created: #7757 In response to this:
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-sigs/prow repository. |
|
@comtalyst: new pull request created: #7758 In response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Comparing to the prior behavior, it looks like ef3b63c have accidentally created a caveat where
provisioningStateFailed+enableFastDeleteOnFailedProvisioning == falseleads tostatus.Statenot being assigned, leading to the undefined status. This PR fixes it and add a set of unit tests for that.The
provisioningStateFailedcase is generally rare. And as of now, it is likely harmless, asInstanceRunninggives the same result as undefined state.A lot of further improvements could be made in this space (e.g., reunifying two near-identical methods, more test cases), but shall be revisited in a different scope.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: