StoragePoolType as class by sureshanaparti · Pull Request #8544 · apache/cloudstack

sureshanaparti · 2024-01-22T07:37:53Z

Description

This PR moves the StoragePoolType from an Enum to a class that is largely compatible with Enum. This allows for storage pool plugins to be added dynamically without needing to hard code an enum value into the core code. We are trying to be Enum compatible to avoid having to refactor much of the code where these are used.

To do this, on the KVM agent side we needed to move from annotation based StoragePoolTypes on the StorageAdaptors to having an adaptor method report the StoragePoolType. This was because annotations aren't dynamic enough to be able to use StoragePoolTypes that aren't static.

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?

… annotation. Implemented conveter class and logic to utilize @convert annotation.

Co-authored-by: mprokopchuk <mprokopchuk@apple.com>

sureshanaparti · 2024-01-22T07:38:17Z

@blueorangutan package

blueorangutan · 2024-01-22T07:40: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 · 2024-01-22T07:45:53Z

Codecov Report

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

Comparison is base (6d916ca) 30.85% compared to head (47d455b) 21.76%.
Report is 38 commits behind head on main.

Files	Patch %	Lines
.../hypervisor/kvm/storage/KVMStoragePoolManager.java	0.00%	25 Missing ⚠️
api/src/main/java/com/cloud/storage/Storage.java	74.46%	8 Missing and 4 partials ⚠️
.../hypervisor/kvm/storage/LibvirtStorageAdaptor.java	0.00%	10 Missing ⚠️
.../cloud/agent/transport/StoragePoolTypeAdaptor.java	37.50%	4 Missing and 1 partial ⚠️
...c/main/java/com/cloud/utils/db/GenericDaoBase.java	82.35%	2 Missing and 1 partial ⚠️
.../hypervisor/kvm/storage/ScaleIOStorageAdaptor.java	0.00%	2 Missing ⚠️
...ain/java/com/cloud/storage/StorageManagerImpl.java	33.33%	2 Missing ⚠️
...cloudstack/storage/datastore/db/StoragePoolVO.java	50.00%	0 Missing and 1 partial ⚠️
...ud/hypervisor/kvm/storage/FiberChannelAdapter.java	0.00%	1 Missing ⚠️
...hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java	0.00%	1 Missing ⚠️
... and 7 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8544      +/-   ##
============================================
- Coverage     30.85%   21.76%   -9.09%     
+ Complexity    34048    22317   -11731     
============================================
  Files          5341     5212     -129     
  Lines        374861   353060   -21801     
  Branches      54518    50573    -3945     
============================================
- Hits         115659    76854   -38805     
- Misses       243973   265088   +21115     
+ Partials      15229    11118    -4111

Flag	Coverage Δ
simulator-marvin-tests	`23.30% <46.51%> (-1.46%)`	⬇️
uitests	`4.39% <ø> (-0.01%)`	⬇️
unit-tests	`?`

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.

blueorangutan · 2024-01-22T08:40:55Z

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

kiranchavala · 2024-01-22T09:11:46Z

@blueorangutan test matrix

blueorangutan · 2024-01-22T09:12:03Z

@kiranchavala a [SL] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2024-01-23T23:09:19Z

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

Test	Result	Time (s)	Test File
test_create_pvlan_network	`Error`	0.06	test_pvlan.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	987.07	test_kubernetes_clusters.py

kiranchavala

LGTM, Tested by adding various types of primary storage and performed basic operations

yadvr · 2024-01-24T08:51:08Z

@blueorangutan test matrix

blueorangutan · 2024-01-24T08:52:03Z

@rohityadavcloud a [SL] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

sureshanaparti · 2024-01-24T09:23:14Z

@blueorangutan package

blueorangutan · 2024-01-24T09:34: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-01-24T10:41:18Z

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

blueorangutan · 2024-01-24T22:19:44Z

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

Test	Result	Time (s)	Test File
test_01_vpc_site2site_vpn_multiple_options	`Failure`	308.87	test_vpc_vpn.py

blueorangutan · 2024-01-25T00:18:35Z

[SF] Trillian test result (tid-8932)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 54034 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8544-t8932-xenserver-71.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

sureshanaparti · 2024-01-29T12:12:05Z

@blueorangutan package

blueorangutan · 2024-01-29T12:14: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-01-29T13:15:06Z

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

sureshanaparti · 2024-01-29T14:38:37Z

@blueorangutan test matrix

blueorangutan · 2024-01-29T14:40:03Z

@sureshanaparti a [SL] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

blueorangutan · 2024-01-30T03:08:37Z

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

Test	Result	Time (s)	Test File
test_01_cancel_host_maintenace_with_no_migration_jobs	`Error`	308.39	test_host_maintenance.py

blueorangutan · 2024-01-30T04:53:10Z

[SF] Trillian test result (tid-8979)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 49449 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8544-t8979-vmware-67u3.zip
Smoke tests completed. 120 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_balanced_drs_algorithm	`Error`	433.44	test_cluster_drs.py

JoaoJandre

CLGTM, didn't test it

kiranchavala

LGTM, tested manually was able to add cluster of type "DatastoreCluster"

harikrishna-patnala

LGTM, tested with some storage related operations

@Enumerated

* StoragePoolType as a class * Fix agent side StoragePoolType enum to class * Handle StoragePoolType for StoragePoolJoinVO * Since StoragePoolType is a class, it cannot be converted by @Enumerated annotation. Implemented conveter class and logic to utilize @convert annotation. * Fix UserVMJoinVO for StoragePoolType * fixed missing imports * Since StoragePoolType is a class, it cannot be converted by @Enumerated annotation. Implemented conveter class and logic to utilize @convert annotation. * Fixed equals for the enum. * removed not needed try/catch for prepareAttribute * Added license to the file. * Implemented "supportsPhysicalDiskCopy" for storage adaptor. Co-authored-by: mprokopchuk <mprokopchuk@apple.com> * Add javadoc to StoragePoolType class * Add unit test for StoragePoolType comparisons * StoragePoolType "==" and ".equals()" fix. * Fix StoragePoolType for FiberChannelAdapter * Fix for abstract storage adaptor set up issue * review comments * Pass StoragePoolType object for poolType dao attribute --------- Co-authored-by: Marcus Sorensen <mls@apple.com> Co-authored-by: mprokopchuk <mprokopchuk@apple.com> Co-authored-by: mprokopchuk <mprokopchuk@gmail.com>

Marcus Sorensen and others added 15 commits January 22, 2024 11:02

StoragePoolType as a class

86e9092

Fix agent side StoragePoolType enum to class

154c736

Handle StoragePoolType for StoragePoolJoinVO

2da13ee

Since StoragePoolType is a class, it cannot be converted by @Enumerated…

3801764

… annotation. Implemented conveter class and logic to utilize @convert annotation.

Fix UserVMJoinVO for StoragePoolType

ea9a7c8

fixed missing imports

5c63110

Since StoragePoolType is a class, it cannot be converted by @Enumerated…

c0b421b

… annotation. Implemented conveter class and logic to utilize @convert annotation.

Fixed equals for the enum.

38108b4

removed not needed try/catch for prepareAttribute

4c40bdc

Added license to the file.

d33197c

Implemented "supportsPhysicalDiskCopy" for storage adaptor.

9f252f9

Co-authored-by: mprokopchuk <mprokopchuk@apple.com>

Add javadoc to StoragePoolType class

17e9cff

Add unit test for StoragePoolType comparisons

9af4432

StoragePoolType "==" and ".equals()" fix.

edecd06

Fix StoragePoolType for FiberChannelAdapter

03c7b60

boring-cyborg bot added component:api component:database component:iscsi component:kvm component:primary-storage component:simulator component:storage labels Jan 22, 2024

sureshanaparti requested review from harikrishna-patnala and kiranchavala January 22, 2024 09:18

kiranchavala approved these changes Jan 24, 2024

View reviewed changes

review comments

f178724

yadvr added the status:ready-for-merge label Jan 29, 2024

Pass StoragePoolType object for poolType dao attribute

47d455b

boring-cyborg bot added the component:orchestration label Jan 29, 2024

sureshanaparti removed the status:ready-for-merge label Jan 29, 2024

JoaoJandre approved these changes Jan 30, 2024

View reviewed changes

kiranchavala approved these changes Jan 31, 2024

View reviewed changes

harikrishna-patnala approved these changes Jan 31, 2024

View reviewed changes

DaanHoogland added the status:ready-for-merge label Jan 31, 2024

yadvr merged commit 8ea9fc9 into apache:main Feb 5, 2024

yadvr deleted the storagepooltype-as-class branch February 5, 2024 07:57

vishesh92 mentioned this pull request Mar 21, 2024

Hypervisor as class #8798

Merged

13 tasks

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Conversation

sureshanaparti commented Jan 22, 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 Jan 22, 2024

Uh oh!

blueorangutan commented Jan 22, 2024

Uh oh!

codecov bot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jan 22, 2024

Uh oh!

kiranchavala commented Jan 22, 2024

Uh oh!

blueorangutan commented Jan 22, 2024

Uh oh!

blueorangutan commented Jan 23, 2024

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jan 24, 2024

Uh oh!

blueorangutan commented Jan 24, 2024

Uh oh!

sureshanaparti commented Jan 24, 2024

Uh oh!

blueorangutan commented Jan 24, 2024

Uh oh!

blueorangutan commented Jan 24, 2024

Uh oh!

blueorangutan commented Jan 24, 2024

Uh oh!

blueorangutan commented Jan 25, 2024

Uh oh!

sureshanaparti commented Jan 29, 2024

Uh oh!

blueorangutan commented Jan 29, 2024

Uh oh!

blueorangutan commented Jan 29, 2024

Uh oh!

sureshanaparti commented Jan 29, 2024

Uh oh!

blueorangutan commented Jan 29, 2024

Uh oh!

blueorangutan commented Jan 30, 2024

Uh oh!

blueorangutan commented Jan 30, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Jan 22, 2024 •

edited

Loading