Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume #8373

sureshanaparti · 2023-12-19T06:14:20Z

Description

This PR fixes reorder/list pools when cluster details are not set, while deploying vm / attaching volume.

Problem:
Attach volume to a VM fails, on infra with zone-wide pools & vm.allocation.algorithm=userdispersing as the cluster details are not set (passed as null) while reordering / listing pools by volumes.

Solution:
Ignore cluster details when not set, while reordering / listing pools by volumes.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Manually tested Deploy VM, and Attach Volume operations on infra with zone-wide pools & vm.allocation.algorithm=userdispersing

How did you try to break this feature and the system with this change?

sureshanaparti · 2023-12-19T06:15:39Z

@blueorangutan package

blueorangutan · 2023-12-19T06:16:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov · 2023-12-19T06:22:48Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6d916ca) 30.85% compared to head (4634aaf) 30.85%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
...main/java/com/cloud/storage/dao/VolumeDaoImpl.java	78.94%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8373   +/-   ##
=========================================
  Coverage     30.85%   30.85%           
- Complexity    34048    34054    +6     
=========================================
  Files          5341     5341           
  Lines        374861   374871   +10     
  Branches      54518    54521    +3     
=========================================
+ Hits         115659   115676   +17     
  Misses       243973   243973           
+ Partials      15229    15222    -7

Flag	Coverage Δ
simulator-marvin-tests	`24.75% <0.00%> (-0.01%)`	⬇️
uitests	`4.39% <ø> (ø)`
unit-tests	`16.47% <78.94%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

weizhouapache

code lgtm

need manual testing

rohityadavcloud

LGTM - refactoring the sql query to evoke based on passed parameter (when not null) makes it super clear why NPE could happen and why it makes the code more defensive.

shwstppr · 2023-12-21T08:02:09Z

@blueorangutan package

blueorangutan · 2023-12-21T08:04:03Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-12-21T09:14:41Z

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 8113

sureshanaparti · 2023-12-21T09:23:09Z

@blueorangutan package

blueorangutan · 2023-12-21T09:24:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

…ing vm / attaching volume

sureshanaparti · 2023-12-21T12:27:52Z

@blueorangutan package

blueorangutan · 2023-12-21T12:30:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-12-21T12:42:35Z

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 8115

blueorangutan · 2023-12-21T15:26:03Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8119

weizhouapache · 2023-12-21T16:09:23Z

@blueorangutan test

blueorangutan · 2023-12-21T16:10:03Z

@weizhouapache a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-12-22T04:23:34Z

[SF] Trillian test result (tid-8655)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42453 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8373-t8655-kvm-centos7.zip
Smoke tests completed. 127 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_05_list_volumes_isrecursive	`Failure`	0.03	test_list_volumes.py
test_07_list_volumes_listall	`Failure`	0.03	test_list_volumes.py

kiranchavala

LGTM, Tested manually by creating a zone wide storage and creating and attaching a data volume to a running vm

Before the fix there was sql exception

2024-01-03 04:16:06,397 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (Work-Job-Executor-15:ctx-61717f63 job-64/job-65) (logid:fc47afe3) Done executing com.cloud.vm.VmWorkAttachVolume for job-65
2024-01-03 04:16:06,398 INFO  [o.a.c.f.j.i.AsyncJobMonitor] (Work-Job-Executor-15:ctx-61717f63 job-64/job-65) (logid:fc47afe3) Remove job-65 from job monitoring
2024-01-03 04:16:06,414 ERROR [c.c.a.ApiAsyncJobDispatcher] (API-Job-Executor-11:ctx-24c62e2f job-64) (logid:fc47afe3) Unexpected exception while executing org.apache.cloudstack.api.command.admin.volume.AttachVolumeCmdByAdmin
com.cloud.utils.exception.CloudRuntimeException: Caught: SELECT pool.id, SUM(IF(vol.state='Ready' AND vol.account_id = ?, 1, 0)) FROM `cloud`.`storage_pool` pool LEFT JOIN `cloud`.`volumes` vol ON pool.id = vol.pool_id WHERE pool.data_center_id = ?  AND pool.pod_id = ? AND pool.cluster_id = ?  GROUP BY pool.id ORDER BY 2 ASC
	at com.cloud.storage.dao.VolumeDaoImpl.listPoolIdsByVolumeCount(VolumeDaoImpl.java:613)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

After the fix there was no sql error observed and the volume got attached

2024-01-03 06:50:22,550 DEBUG [c.c.s.StorageManagerImpl] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) Checking pool: 2 for storage allocation , maxSize : (3.9980 TB) 4395899027456, totalAllocatedSize : (22.65 GB) 24318968832, askingSize : (5.00 GB) 5368709120, allocated disable threshold: 0.95
2024-01-03 06:50:22,550 DEBUG [o.a.c.s.a.AbstractStoragePoolAllocator] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) ZoneWideStoragePoolAllocator is returning [1] suitable storage pools [[{"name":"test","uuid":"4fe5cbe5-ab56-37a4-8727-9d9795164ca7"}]].
2024-01-03 06:50:22,552 DEBUG [o.a.c.e.o.VolumeOrchestrator] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) VM [VM instance {"id":3,"instanceName":"i-2-3-VM","type":"User","uuid":"922e5659-38fe-4fb7-b10e-9df3ffadb1dc"}] does not have a preferred storage pool or it cannot be used. Volume Orchestrator will use the available Storage Pool [{"name":"test","uuid":"4fe5cbe5-ab56-37a4-8727-9d9795164ca7"}], which was discovered using Storage Pool Allocator [ZoneWideStoragePoolAllocator].

2024-01-03 06:50:22,818 DEBUG [c.c.a.t.Request] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) Seq 1-5166191722547380285: Received:  { Ans: , MgmtId: 32988620391433, via: 1(ref-trl-6114-k-M7-kiran-chavala-kvm1), Ver: v1, Flags: 10, { AttachAnswer } }
2024-01-03 06:50:22,827 DEBUG [c.c.s.VolumeApiServiceImpl] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) Volume: vb successfully attached to VM: i-2-3-VM
2024-01-03 06:50:22,834 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-4:ctx-48decd5c job-50/job-51 ctx-5d9c474b) (logid:8aea532a) Done executing VM work job: com.cloud.vm.VmWorkAttachVolume{"volumeId":8,"userId":2,"accountId":2,"vmId":3,"handlerName":"VolumeApiServiceImpl"}

rohityadavcloud · 2024-01-03T12:33:13Z

cc @shwstppr this is a bug fix should we merge it, or is main frozen for others?
@blueorangutan test

blueorangutan · 2024-01-03T12:34:03Z

@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-01-04T02:07:57Z

[SF] Trillian test result (tid-8717)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47276 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8373-t8717-kvm-centos7.zip
Smoke tests completed. 126 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_05_list_volumes_isrecursive	`Failure`	0.04	test_list_volumes.py
test_07_list_volumes_listall	`Failure`	0.03	test_list_volumes.py
test_08_migrate_vm	`Error`	0.05	test_vm_life_cycle.py

shwstppr · 2024-01-04T05:52:41Z

@rohityadavcloud it can be merged if it is ready. We will need to cut a new RC so we can accept bug fixes now.

shwstppr

Code LGTM

shwstppr · 2024-01-04T06:02:08Z

Packages are 2 weeks old, returning more errors - test_list_volumes.py
@blueorangutan package

blueorangutan · 2024-01-04T06:04:03Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-01-04T07:11:58Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8195

sureshanaparti · 2024-01-04T07:58:54Z

@blueorangutan test

blueorangutan · 2024-01-04T08:00:04Z

@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-01-04T22:03:32Z

[SF] Trillian test result (tid-8722)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49111 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8373-t8722-kvm-centos7.zip
Smoke tests completed. 121 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

harikrishna-patnala

LGTM

shwstppr · 2024-01-10T12:43:14Z

Looks good to include in 4.19RC2. Moving to the milestone and merging

…ing vm / attaching volume (apache#8373) This PR fixes reorder/list pools when cluster details are not set, while deploying vm / attaching volume. Problem: Attach volume to a VM fails, on infra with zone-wide pools & vm.allocation.algorithm=userdispersing as the cluster details are not set (passed as null) while reordering / listing pools by volumes. Solution: Ignore cluster details when not set, while reordering / listing pools by volumes.

boring-cyborg bot added the component:storage label Dec 19, 2023

sureshanaparti requested review from harikrishna-patnala and shwstppr December 19, 2023 06:20

weizhouapache approved these changes Dec 19, 2023

View reviewed changes

DaanHoogland added the status:needs-testing label Dec 19, 2023

rohityadavcloud approved these changes Dec 20, 2023

View reviewed changes

sureshanaparti force-pushed the attachvolume-issue-with-vm-alloc-userdispersing branch 2 times, most recently from 6dbe9f2 to 2f35795 Compare December 20, 2023 18:29

sureshanaparti added 2 commits December 21, 2023 16:36

Fix reorder/list pools when cluster details are not set, while deploy…

347e92b

…ing vm / attaching volume

Added unit tests

4634aaf

sureshanaparti force-pushed the attachvolume-issue-with-vm-alloc-userdispersing branch from 2f35795 to 4634aaf Compare December 21, 2023 12:27

kiranchavala approved these changes Jan 3, 2024

View reviewed changes

rohityadavcloud added this to the 4.19.1.0 milestone Jan 3, 2024

shwstppr approved these changes Jan 4, 2024

View reviewed changes

harikrishna-patnala approved these changes Jan 9, 2024

View reviewed changes

DaanHoogland added status:ready-for-merge and removed status:needs-testing labels Jan 9, 2024

shwstppr modified the milestones: 4.19.1.0, 4.19.0.0 Jan 10, 2024

shwstppr merged commit e87ce0c into apache:main Jan 10, 2024
26 checks passed

shwstppr deleted the attachvolume-issue-with-vm-alloc-userdispersing branch January 10, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume #8373

Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume #8373

sureshanaparti commented Dec 19, 2023

sureshanaparti commented Dec 19, 2023

blueorangutan commented Dec 19, 2023

codecov bot commented Dec 19, 2023 •

edited

weizhouapache left a comment

rohityadavcloud left a comment

shwstppr commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

sureshanaparti commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

sureshanaparti commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

weizhouapache commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 22, 2023

kiranchavala left a comment

rohityadavcloud commented Jan 3, 2024

blueorangutan commented Jan 3, 2024

blueorangutan commented Jan 4, 2024

shwstppr commented Jan 4, 2024

shwstppr left a comment

shwstppr commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

sureshanaparti commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

harikrishna-patnala left a comment

shwstppr commented Jan 10, 2024

Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume #8373

Fix reorder/list pools when cluster details are not set, while deploying vm / attaching volume #8373

Conversation

sureshanaparti commented Dec 19, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

sureshanaparti commented Dec 19, 2023

blueorangutan commented Dec 19, 2023

codecov bot commented Dec 19, 2023 • edited

Codecov Report

weizhouapache left a comment

Choose a reason for hiding this comment

rohityadavcloud left a comment

Choose a reason for hiding this comment

shwstppr commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

sureshanaparti commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

sureshanaparti commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

weizhouapache commented Dec 21, 2023

blueorangutan commented Dec 21, 2023

blueorangutan commented Dec 22, 2023

kiranchavala left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Jan 3, 2024

blueorangutan commented Jan 3, 2024

blueorangutan commented Jan 4, 2024

shwstppr commented Jan 4, 2024

shwstppr left a comment

Choose a reason for hiding this comment

shwstppr commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

sureshanaparti commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

blueorangutan commented Jan 4, 2024

harikrishna-patnala left a comment

Choose a reason for hiding this comment

shwstppr commented Jan 10, 2024

codecov bot commented Dec 19, 2023 •

edited