-
Notifications
You must be signed in to change notification settings - Fork 27
Pass on volume details when disk is in available state #1022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kishen-v 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 |
d8617c7 to
db93dd6
Compare
|
Retested with latest changes. |
|
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Could not create volume %q: %v", volName, err) | ||
| if errors.Is(err, cloud.ErrNotFound) { | ||
| disk, err = d.cloud.CreateDisk(volName, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just worried if Get fails because the creation did not initiate at the cloud side, this might create duplicate volumes. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we expect this not to happen usually, can we sleep for 5s and try GET/POST again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point..
Initial creation begins with
-> disk, err := d.cloud.GetDiskByName(volName)
-> proceeds to create a Volume
-> Duplicate call in the meantime(?) - Volume creation is re-triggered.
But the catch is, cloud doesn't allow volumes with duplicate names to be created.. We should be good here.
ishenv@kishen-mbp-m1 ~ % ibmcloud pi vol create test --size=1
Creating volume test under account PCLOUD Hypershift as user [email protected]...
ID 93a78bd6-fff5-4f49-ac3a-1a4f5224c5f5
Name test
Profile tier3
Status creating
Size 1
Created 2025-12-10T06:18:59.000Z
Updated 1970-01-01T00:00:00.000Z
Shareable false
Bootable false
Storage Pool General-Flash-6
IO Throttle Rate -
Replication Enabled -
PVMInstanceIDs -
WWN -
kishenv@kishen-mbp-m1 ~ % ibmcloud pi vol create test --size=1
Creating volume test under account PCLOUD Hypershift as user [email protected]...
FAILED
Failed to create volume.
FAILED
Error: [POST /pcloud/v2/cloud-instances/{cloud_instance_id}/volumes][400] pcloudV2VolumesPostBadRequest {"description":"Bad Request: test volume name already exists for cloud instance 384c8836936f4b658c5c628dc50efce0; duplicate names are not allowed","error":"Bad Request"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Volume creation is re-triggered.
But the catch is, cloud doesn't allow volumes with duplicate names to be created.. We should be good here.
Sorry but I cannot trust this because I have seen duplicate vm instances with same name before on the cloud. 🙂
|
/hold |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR mitigates the flakey-nature of passing-on Volume related information at the early stages of creation, which may have multiple fields unavailable when the volume is creating as observed in the recent failing Integration test suites.
Added supporting changes to capture information of the volume when available before proceeding with the subsequent phases.
Which issue(s) this PR fixes:
Fixes #1021
Special notes for your reviewer:
Release note: