Fixed race condition in additional volumes reconciliation #2337
Conversation
k8s-ci-robot
added
area/provider/ibmcloud
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anshuman-agarwala The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @anshuman-agarwala. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
anshuman-agarwala
force-pushed
the
additionalvol-fix
branch
from
12e42f7 to
01f5192
Compare
|
@anshuman-agarwala squash the commits and update the release notes. |
anshuman-agarwala
force-pushed
the
additionalvol-fix
branch
from
01f5192 to
9d1c69b
Compare
Done |
|
/test pull-cluster-api-provider-ibmcloud-test |
cloud/scope/machine.go
Outdated
| volumeOptions := vpcv1.CreateVolumeOptions{} | ||
| // TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest | ||
| if vpcVolume.Profile != "custom" { | ||
| volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{ | ||
| ResourceGroup: &vpcv1.ResourceGroupIdentityByID{ | ||
| ID: &m.IBMVPCCluster.Spec.ResourceGroup, |
There was a problem hiding this comment.
Did you check if the resource group will be present for sure and there is a validation for it to be non-nil?
There was a problem hiding this comment.
Refactored it to be similar to how we are getting the resourcegroup in other places.
cloud/scope/machine.go
Outdated
| } | ||
|
|
||
| if *volumeResult.Status != vpcv1.VolumeStatusAvailableConst { | ||
| return *volumeResult.ID, fmt.Errorf("volume is not in available state") |
There was a problem hiding this comment.
can we pass a condition to requeue here as volume not being in available state is not an error?
Also, are we handling all the different states of the volume? what if the volume enters into error state?
cloud/scope/machine.go
Outdated
| return "", fmt.Errorf("error while creating volume: %w", err) | ||
| } | ||
|
|
||
| if *volumeResult.Status != vpcv1.VolumeStatusAvailableConst { |
There was a problem hiding this comment.
you can add the conditions here and use it, similar to the other resources
| if err != nil { | ||
| errList = append(errList, err) | ||
| } | ||
| if state != vpcv1.VolumeStatusAvailableConst { |
There was a problem hiding this comment.
you can add the conditions here and use it, similar to the other resources
anshuman-agarwala
force-pushed
the
additionalvol-fix
branch
from
9d1c69b to
0b71729
Compare
|
/hold @Amulyam24 @Karthik-K-N we will revert the feature for this release, lets take it up in next release. That would allow us enough time to test it out thoroughly. |
k8s-ci-robot
added
the
do-not-merge/hold
|
PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
What this PR does / why we need it:
Fixes race condition in additional volumes reconciliation
Fixes resourcegroup for created volumes
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 #
Special notes for your reviewer:
/area provider/ibmcloud
Release note: