server: fix volume migration on user vm scale by shwstppr · Pull Request #6704 · apache/cloudstack

shwstppr · 2022-09-05T18:23:35Z

Description

When volume migration is initiated by system, account check is not needed.

Introduces a new global setting - allow.diskOffering.change.during.scale.vm
Determines whether to allow or disallow disk offering change for root volume during scaling of a stopped or running VM. Default value is true

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Fixes apache#6701 When volume migration is initiated by system account check is not needed. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2022-09-05T18:23:47Z

@blueorangutan package

blueorangutan · 2022-09-05T18:24:03Z

@shwstppr a 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 · 2022-09-05T19:06:43Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 4129

sonarqubecloud · 2022-09-05T19:18:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

0.0% Coverage
0.0% Duplication

shwstppr · 2022-09-05T19:21:47Z

@blueorangutan test

blueorangutan · 2022-09-05T19:22:02Z

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

codecov · 2022-09-05T20:50:19Z

Codecov Report

Merging #6704 (94db086) into 4.17 (296035d) will decrease coverage by 0.00%.
The diff coverage is 4.16%.

❗ Current head 94db086 differs from pull request most recent head 2c7f27f. Consider uploading reports for the commit 2c7f27f to get more accurate results

@@             Coverage Diff              @@
##               4.17    #6704      +/-   ##
============================================
- Coverage     10.34%   10.34%   -0.01%     
- Complexity     6610     6611       +1     
============================================
  Files          2451     2451              
  Lines        242340   242352      +12     
  Branches      37924    37926       +2     
============================================
- Hits          25070    25066       -4     
- Misses       214182   214198      +16     
  Partials       3088     3088

Impacted Files	Coverage Δ
...in/java/com/cloud/server/ManagementServerImpl.java	`2.50% <0.00%> (-0.01%)`	⬇️
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	`10.79% <0.00%> (-0.03%)`	⬇️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	`5.63% <0.00%> (-0.01%)`	⬇️
.../apache/cloudstack/vm/UnmanagedVMsManagerImpl.java	`31.62% <0.00%> (ø)`
...rver/src/main/java/com/cloud/vm/UserVmManager.java	`100.00% <100.00%> (ø)`
...rg/apache/cloudstack/quota/QuotaStatementImpl.java	`36.28% <0.00%> (-3.99%)`	⬇️
...dstack/network/contrail/model/ModelObjectBase.java	`28.84% <0.00%> (+7.69%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

blueorangutan · 2022-09-06T06:51:50Z

Trillian test result (tid-4820)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40069 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6704-t4820-kvm-centos7.zip
Smoke tests completed. 100 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	`Failure`	604.34	test_kubernetes_clusters.py

shwstppr · 2022-09-06T12:06:06Z

Moving this to draft, need to revisit this later. Ideally, the normal user should not be allowed to scale VM or change volume offering with volume migration.

DaanHoogland · 2022-10-11T10:50:27Z

Moving this to draft, need to revisit this later. Ideally, the normal user should not be allowed to scale VM or change volume offering with volume migration.

is ther some technical reason not to allow that @shwstppr ? because from a business point of view I would want them to be able to scale VMs and volumes.

shwstppr · 2022-10-11T10:59:11Z

@DaanHoogland didn't understand you.
I was saying scale VM operation shouldn't be allowed with migration of volume and without migration it should be fine.
This was based on the fact that by default user role is not allowed to use migrateVolume API.

DaanHoogland · 2022-10-12T06:52:59Z

@DaanHoogland didn't understand you. I was saying scale VM operation shouldn't be allowed with migration of volume and without migration it should be fine. This was based on the fact that by default user role is not allowed to use migrateVolume API.

ok, makes sense, migration not being allowed. sorry I misunderstood.

DaanHoogland

clgtm

blueorangutan · 2022-10-14T13:23:03Z

Trillian Build Failed (tid-5131)

blueorangutan · 2022-10-15T03:21:41Z

Trillian Build Failed (tid-5137)

…o allow or skip disk offering change upon scale VM operation. Added marvin tests to verify scale VM operation with user account and also to verify the new global/zone setting

shwstppr · 2022-11-24T12:16:32Z

@blueorangutan test matrix

blueorangutan · 2022-11-24T12:17:03Z

@shwstppr a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

soreana · 2022-11-24T22:07:20Z

I've tested this pr, wiith this patch, I can scale the VM as a domain admin.

blueorangutan · 2022-11-24T23:06:53Z

Trillian test result (tid-5259)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37792 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6704-t5259-xenserver-71.zip
Smoke tests completed. 100 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	`Failure`	622.38	test_kubernetes_clusters.py

blueorangutan · 2022-11-24T23:30:41Z

Trillian test result (tid-5261)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39341 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6704-t5261-vmware-65u2.zip
Smoke tests completed. 101 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2022-11-24T23:36:05Z

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

Test	Result	Time (s)	Test File

yadvr · 2022-11-25T08:52:17Z

Test LGTM
@borisstoyanov @vladimirpetrov can you help test this?

Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2022-11-29T08:07:29Z

@blueorangutan package

blueorangutan · 2022-11-29T08:08:04Z

@rohityadavcloud a 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 · 2022-11-29T09:23:24Z

Packaging result: ✔️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 4671

yadvr · 2022-11-29T11:17:48Z

env issue - pkg is okay for EL7 and others (will rekick after smoketests)
@blueorangutan test

blueorangutan · 2022-11-29T11:18:03Z

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

GutoVeronezi · 2022-11-29T12:22:27Z

+    private boolean compareEqualsIncludingNullOrZero(Long a, Long b) {
+        a = ObjectUtils.defaultIfNull(a, 0L);
+        b = ObjectUtils.defaultIfNull(b, 0L);
+
+        return a.equals(b);
+    }


@shwstppr, we could move this method to com.cloud.utils.NumbersUtils and add some unit tests.

GutoVeronezi · 2022-11-29T12:23:09Z

+     * @param b
+     * @return true if a and b are equal excluding 0 and null values.
+     */
+    private boolean compareEqualsIncludingNullOrZero(Long a, Long b) {


@shwstppr, what do you think about renaming the method to compareLongsDefaultingNullToZero?

vladimirpetrov

LGTM based on manual testing.

blueorangutan · 2022-11-29T23:32:04Z

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

Test	Result	Time (s)	Test File

Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

yadvr · 2022-11-30T07:21:25Z

I've committed suggestions, there's one outstanding on refactoring to a utils which can be done in a separate PR as #shwstppr is on holiday this week. I'll merge this to keep effort on-going for 4.17.2.0 release.
@blueorangutan package

blueorangutan · 2022-11-30T07:22:03Z

@rohityadavcloud a 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 · 2022-11-30T08:02:15Z

Packaging result: ✖️ el7 ✔️ el8 ✖️ debian ✖️ suse15. SL-JID 4694

server: fix volume migration on user vm scale

ce956b8

Fixes apache#6701 When volume migration is initiated by system account check is not needed. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborg Bot added component:api component:management-server component:storage labels Sep 5, 2022

shwstppr requested review from DaanHoogland, harikrishna-patnala, nvazquez, weizhouapache and yadvr September 6, 2022 06:58

shwstppr marked this pull request as ready for review September 6, 2022 09:17

shwstppr marked this pull request as draft September 6, 2022 12:06

shwstppr mentioned this pull request Sep 6, 2022

No permission to migrate volume, only root admin can migrate a volume #6701

Closed

yadvr added this to the 4.18.0.0 milestone Oct 11, 2022

DaanHoogland approved these changes Oct 12, 2022

View reviewed changes

Added Global/zone setting allow.diskOffering.change.during.scale.vm t…

b1fb6e8

…o allow or skip disk offering change upon scale VM operation. Added marvin tests to verify scale VM operation with user account and also to verify the new global/zone setting

boring-cyborg Bot added the component:integration-test label Oct 17, 2022

yadvr assigned vladimirpetrov and borisstoyanov Nov 25, 2022

GutoVeronezi reviewed Nov 28, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

yadvr and others added 2 commits November 29, 2022 13:33

Update server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

e75446e

Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

fix import

08a5769

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr requested a review from GutoVeronezi November 29, 2022 11:17

GutoVeronezi reviewed Nov 29, 2022

View reviewed changes

vladimirpetrov approved these changes Nov 29, 2022

View reviewed changes

yadvr removed the status:needs-testing label Nov 29, 2022

yadvr reviewed Nov 30, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/vm/UserVmManager.java Outdated

yadvr and others added 2 commits November 30, 2022 12:47

Update server/src/main/java/com/cloud/vm/UserVmManager.java

1860068

Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

24e7cb2

Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com>

yadvr merged commit 47946db into apache:4.17 Nov 30, 2022

weizhouapache mentioned this pull request Jan 10, 2023

Unknown parameters : projectid during scale up VM by project admin/user in 4.17.1 #6820

Closed

Uh oh!

Conversation

shwstppr commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

shwstppr commented Sep 5, 2022

Uh oh!

blueorangutan commented Sep 5, 2022

Uh oh!

blueorangutan commented Sep 5, 2022

Uh oh!

sonarqubecloud Bot commented Sep 5, 2022

Uh oh!

shwstppr commented Sep 5, 2022

Uh oh!

blueorangutan commented Sep 5, 2022

Uh oh!

codecov Bot commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Sep 6, 2022

Uh oh!

shwstppr commented Sep 6, 2022

Uh oh!

DaanHoogland commented Oct 11, 2022

Uh oh!

shwstppr commented Oct 11, 2022

Uh oh!

DaanHoogland commented Oct 12, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 14, 2022

Uh oh!

blueorangutan commented Oct 15, 2022

Uh oh!

shwstppr commented Nov 24, 2022

Uh oh!

blueorangutan commented Nov 24, 2022

Uh oh!

soreana commented Nov 24, 2022

Uh oh!

blueorangutan commented Nov 24, 2022

Uh oh!

blueorangutan commented Nov 24, 2022

Uh oh!

blueorangutan commented Nov 24, 2022

Uh oh!

yadvr commented Nov 25, 2022

Uh oh!

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

yadvr commented Nov 29, 2022

Uh oh!

blueorangutan commented Nov 29, 2022

Uh oh!

GutoVeronezi Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vladimirpetrov left a comment

Choose a reason for hiding this comment

shwstppr commented Sep 5, 2022 •

edited

Loading

codecov Bot commented Sep 5, 2022 •

edited

Loading