Storage plugin support to check if volume on datastore requires access for migration by sureshanaparti · Pull Request #8655 · apache/cloudstack

sureshanaparti · 2024-02-15T07:53:10Z

Description

This PR adds storage plugin support to check if volume on datastore requires access for migration, and grant/revoke volume access if requires.

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?

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

Tested VM migration with volumes of NFS & PowerFlex storages.

…/revoke volume access if requires

sureshanaparti · 2024-02-15T07:54:00Z

@blueorangutan package

blueorangutan · 2024-02-15T07:56: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.

harikrishna-patnala

code LGTM. As I can see this is currently enabled only for powerflex storage

codecov · 2024-02-15T08:45:25Z

Codecov Report

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

Comparison is base (e47a910) 13.16% compared to head (3b17c58) 13.16%.

Files	Patch %	Lines
...e/cloudstack/storage/volume/VolumeServiceImpl.java	0.00%	6 Missing ⚠️
...stack/engine/orchestration/VolumeOrchestrator.java	0.00%	2 Missing ⚠️
.../subsystem/api/storage/PrimaryDataStoreDriver.java	0.00%	1 Missing ⚠️
...atastore/driver/ScaleIOPrimaryDataStoreDriver.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.18    #8655      +/-   ##
============================================
- Coverage     13.16%   13.16%   -0.01%     
  Complexity     9203     9203              
============================================
  Files          2724     2724              
  Lines        258120   258128       +8     
  Branches      40231    40233       +2     
============================================
  Hits          33989    33989              
- Misses       219823   219831       +8     
  Partials       4308     4308

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

blueorangutan · 2024-02-15T08:58:51Z

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

DaanHoogland · 2024-02-15T10:44:24Z

general question @sureshanaparti : the word Access used in this PR seems to me to mean Authorisation (or Authorization depending on your continent). Does this make sense? I think we should rename it to avoid confusion. Access is always needed and it seems that we are checking if we need access to a store instead of checking if we need authentication/authorisation to get access. ???

sureshanaparti · 2024-02-15T12:10:25Z

general question @sureshanaparti : the word Access used in this PR seems to me to mean Authorisation (or Authorization depending on your continent). Does this make sense? I think we should rename it to avoid confusion. Access is always needed and it seems that we are checking if we need access to a store instead of checking if we need authentication/authorisation to get access. ???

@DaanHoogland It's about making the volume/disk available in the host, and can be accessed at certain path. In managed storages eg. powerflex, disk is not available directly unless it is mapped to a particular host/client through their proprietary API/cmd). Will keep it the same for now, as there are other methods with similar names (we can do a name refactor later, in a separate PR), Is that Ok?

sureshanaparti · 2024-02-15T12:11:14Z

@blueorangutan test

blueorangutan · 2024-02-15T12:12:04Z

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

JoaoJandre

CLGTM, didn't test it as I don't have an environment with Powerflex.

…in PrimaryDataStoreDriver

sureshanaparti · 2024-02-16T07:49:28Z

@blueorangutan package

blueorangutan · 2024-02-16T07:50: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 · 2024-02-16T08:49:21Z

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

sureshanaparti · 2024-02-16T08:56:27Z

@blueorangutan test

blueorangutan · 2024-02-16T08:58:03Z

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

blueorangutan · 2024-02-16T09:17:39Z

[SF] Trillian test result (tid-9235)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44078 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8655-t9235-kvm-centos7.zip
Smoke tests completed. 109 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_migrate_vm	`Error`	0.06	test_vm_life_cycle.py

blueorangutan · 2024-02-16T20:59:03Z

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

Test	Result	Time (s)	Test File
test_09_arping_in_cpvm	`Failure`	5.18	test_diagnostics.py
test_01_secure_vm_migration	`Error`	121.92	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	121.93	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	46.88	test_vm_life_cycle.py

sureshanaparti · 2024-02-20T08:16:20Z

@blueorangutan package

blueorangutan · 2024-02-20T08:18:05Z

@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 · 2024-02-20T09:17:05Z

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

yadvr · 2024-02-20T18:52:33Z

@blueorangutan test

blueorangutan · 2024-02-20T18:54:03Z

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

blueorangutan · 2024-02-21T06:26:31Z

[SF] Trillian test result (tid-9312)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40050 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8655-t9312-kvm-centos7.zip
Smoke tests completed. 109 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_migrate_vm	`Error`	0.05	test_vm_life_cycle.py

borisstoyanov · 2024-02-26T13:45:55Z

[SF] Trillian test result (tid-9312) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 40050 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8655-t9312-kvm-centos7.zip Smoke tests completed. 109 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_migrate_vm Error 0.05 test_vm_life_cycle.py

I've rerun the tests and it passed

[root@pr8655-t9325-kvm-alma8-marvin ~]# cat /marvin//MarvinLogs/test_vm_life_cycle_G87SBT/results.txt
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
ok
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
ok
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
ok
Test VM will be migrated with it's root volume ... === TestName: test_01_migrate_VM_and_root_volume | Status : SUCCESS ===
ok
Test VM will be migrated with it's root volume ... === TestName: test_02_migrate_VM_with_two_data_disks | Status : SUCCESS ===
ok
Test VM will be migrated with it's root volume ... SKIP: VM Migration with Volumes is not supported on other than VMware
Test VM will be migrated with it's root volume ... SKIP: VM Migration with Volumes is not supported on other than VMware
Test VM will be migrated with it's root volume ... SKIP: VM Migration with Volumes is not supported on other than VMware
Test VM will be migrated with it's root volume ... SKIP: VM Migration with Volumes is not supported on other than VMware
Test secure VM migration ... === TestName: test_01_secure_vm_migration | Status : SUCCESS ===
ok
Test Non-secured VM Migration ... === TestName: test_02_unsecure_vm_migration | Status : SUCCESS ===
ok
Test destroy Virtual Machine ... === TestName: test_03_secured_to_nonsecured_vm_migration | Status : SUCCESS ===
ok
Test Non-secured VM Migration ... === TestName: test_04_nonsecured_to_secured_vm_migration | Status : SUCCESS ===
ok
Test the following for all found ovf templates: ... SKIP: Skipping test: Reason -  hypervisorNotSupported
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
ok
Test Force Stop Virtual Machine ... === TestName: test_01_stop_vm_forced | Status : SUCCESS ===
ok
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
ok
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
ok
Test Force Reboot Virtual Machine ... === TestName: test_04_reboot_vm_forced | Status : SUCCESS ===
ok
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
ok
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
ok
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
ok
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
ok
Test for attach and detach ISO to virtual machine ... === TestName: test_10_attachAndDetach_iso | Status : SUCCESS ===
ok
Test destroy Virtual Machine and it's volumes ... === TestName: test_11_destroy_vm_and_volumes | Status : SUCCESS ===
ok
Test attaching multiple datadisks and start VM ... === TestName: test_12_start_vm_multiple_volumes_allocated | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 26 tests in 1965.493s

OK (SKIP=5)

borisstoyanov

LGTM

yadvr · 2024-02-26T14:46:58Z

cc @DaanHoogland @sureshanaparti could you help fwd-merge 4.18-4.19 and 4.19-> main branch, thanks.

* 4.18: Storage plugin support to check if volume on datastore requires access for migration (#8655) CKS: fix /opt/bin/deploy-cloudstack-secret in CKS control nodes (#8697)

…s for migration (apache#8655) * Check if volume on datastore requires access for migration, and grant/revoke volume access if requires * Updated default implementation for requiresAccessForMigration method in PrimaryDataStoreDriver

* 4.18: Storage plugin support to check if volume on datastore requires access for migration (apache#8655) CKS: fix /opt/bin/deploy-cloudstack-secret in CKS control nodes (apache#8697)

Check if volume on datastore requires access for migration, and grant…

7ebc392

…/revoke volume access if requires

boring-cyborg Bot added component:orchestration component:primary-storage component:solidfire component:storage labels Feb 15, 2024

sureshanaparti added this to the 4.18.2.0 milestone Feb 15, 2024

sureshanaparti requested a review from harikrishna-patnala February 15, 2024 07:53

sureshanaparti changed the title ~~Storage plugin support to check if volume on datastore requires access for migration, and grant/revoke volume access if requires~~ Storage plugin support to check if volume on datastore requires access for migration Feb 15, 2024

sureshanaparti requested a review from JoaoJandre February 15, 2024 07:58

yadvr approved these changes Feb 15, 2024

View reviewed changes

harikrishna-patnala approved these changes Feb 15, 2024

View reviewed changes

JoaoJandre approved these changes Feb 15, 2024

View reviewed changes

Updated default implementation for requiresAccessForMigration method …

3b17c58

…in PrimaryDataStoreDriver

DaanHoogland added the status:needs-testing label Feb 16, 2024

yadvr requested a review from kiranchavala February 21, 2024 06:22

DaanHoogland marked this pull request as draft February 23, 2024 08:01

borisstoyanov approved these changes Feb 26, 2024

View reviewed changes

yadvr removed the status:needs-testing label Feb 26, 2024

yadvr marked this pull request as ready for review February 26, 2024 14:45

yadvr merged commit f731fe8 into apache:4.18 Feb 26, 2024

yadvr deleted the plugin-support-to-grant-access-for-volume-migration branch February 26, 2024 14:46

DaanHoogland added a commit that referenced this pull request Feb 26, 2024

Merge release branch 4.18 to 4.19

f4987bf

* 4.18: Storage plugin support to check if volume on datastore requires access for migration (#8655) CKS: fix /opt/bin/deploy-cloudstack-secret in CKS control nodes (#8697)

Conversation

sureshanaparti commented Feb 15, 2024

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?

Uh oh!

sureshanaparti commented Feb 15, 2024

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

DaanHoogland commented Feb 15, 2024

Uh oh!

sureshanaparti commented Feb 15, 2024

Uh oh!

sureshanaparti commented Feb 15, 2024

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Feb 16, 2024

Uh oh!

blueorangutan commented Feb 16, 2024

Uh oh!

blueorangutan commented Feb 16, 2024

Uh oh!

sureshanaparti commented Feb 16, 2024

Uh oh!

blueorangutan commented Feb 16, 2024

Uh oh!

blueorangutan commented Feb 16, 2024

Uh oh!

blueorangutan commented Feb 16, 2024

Uh oh!

sureshanaparti commented Feb 20, 2024

Uh oh!

blueorangutan commented Feb 20, 2024

Uh oh!

blueorangutan commented Feb 20, 2024

Uh oh!

yadvr commented Feb 20, 2024

Uh oh!

blueorangutan commented Feb 20, 2024

Uh oh!

blueorangutan commented Feb 21, 2024

Uh oh!

borisstoyanov commented Feb 26, 2024

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Feb 15, 2024 •

edited

Loading