Skip to content
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

Fix serial disk add testcase logic #3232

Merged
merged 1 commit into from
Mar 30, 2024

Conversation

SRIKKANTH
Copy link
Collaborator

Current logic is adding and removing a single disk instead of adding 1 disk at a time till max supported disks.

Modified the logic to add 1 disk at a time till max allowed disks and for removal all attached disks one after the other.

@SRIKKANTH SRIKKANTH marked this pull request as ready for review March 26, 2024 14:13
@LiliDeng
Copy link
Collaborator

Current logic is adding and removing a single disk instead of adding 1 disk at a time till max supported disks.

Modified the logic to add 1 disk at a time till max allowed disks and for removal all attached disks one after the other.

Is there any test issue for current code? Can you clarify the reason for changing code? Thanks.

@SRIKKANTH SRIKKANTH closed this Mar 26, 2024
@SRIKKANTH
Copy link
Collaborator Author

SRIKKANTH commented Mar 26, 2024

Current logic is adding and removing a single disk instead of adding 1 disk at a time till max supported disks.
Modified the logic to add 1 disk at a time till max allowed disks and for removal all attached disks one after the other.

Is there any test issue for current code? Can you clarify the reason for changing code? Thanks.

Current testcase simply verifies adding removing a single disk instead of actually checking if its allowed to attach one disk at a time till max supported disks.

Is this expected behavior?

If yes why the add remove iteration count is equal to max disks allowed on the SKU why not add remove with a fixed number of times.

I think we should validate adding disks serially till max allowed disks and remove all the attached disks in the end.

@SRIKKANTH SRIKKANTH reopened this Mar 26, 2024
@adityagesh
Copy link
Collaborator

adityagesh commented Mar 28, 2024

Your change aligns with the description of the test case (pasting it below)

        1. Get maximum number of data disk for the current vm_size.
        2. Get the number of data disks already added to the vm.
        3. Serially add and remove the data disks and verify that the added
        disks are present in the vm.

However, the name of the function is misleading, how about renaming it similar to something like hot_add_disk_serial_one_by_one? Similar to how we have for the nic

Thoughts on this? @LiliDeng

@SRIKKANTH SRIKKANTH force-pushed the smyakam/fix_serial_disk_add_testcase branch 2 times, most recently from f7588b6 to a0b2996 Compare March 29, 2024 17:47
Fix '_hot_add_disk_serial' method logic
@SRIKKANTH SRIKKANTH force-pushed the smyakam/fix_serial_disk_add_testcase branch from a0b2996 to 55cb85c Compare March 30, 2024 09:40
@LiliDeng LiliDeng merged commit 4fa579b into main Mar 30, 2024
45 checks passed
@LiliDeng LiliDeng deleted the smyakam/fix_serial_disk_add_testcase branch March 30, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants