Skip to content

new (java11) style storage construction#6797

Merged
DaanHoogland merged 1 commit intoapache:4.18from
shapeblue:storageConstructionException
Jul 19, 2023
Merged

new (java11) style storage construction#6797
DaanHoogland merged 1 commit intoapache:4.18from
shapeblue:storageConstructionException

Conversation

@DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Oct 2, 2022 •
edited
Loading

Description

This PR unpates the reflective construction of storage adaptors (initiated by #6794, but it does not solve it)

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland DaanHoogland added this to the 4.18.0.0 milestone Oct 2, 2022
@codecov
Copy link

codecov bot commented Oct 2, 2022 •
edited
Loading

Codecov Report

Merging #6797 (50d925b) into main (89d9a70) will increase coverage by 0.81%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #6797      +/-   ##
============================================
+ Coverage     10.33%   11.15%   +0.81%     
- Complexity     6608     7442     +834     
============================================
  Files          2451     2488      +37     
  Lines        242340   258961   +16621     
  Branches      37924    43590    +5666     
============================================
+ Hits          25054    28876    +3822     
- Misses       214200   226567   +12367     
- Partials       3086     3518     +432
Impacted Files Coverage Δ
.../hypervisor/kvm/storage/KVMStoragePoolManager.java 0.48% <0.00%> (-0.01%) ⬇️
.../cloudstack/api/response/ApiDiscoveryResponse.java 0.00% <0.00%> (-100.00%) ⬇️
.../cloudstack/api/response/ApiParameterResponse.java 0.00% <0.00%> (-100.00%) ⬇️
...e/cloudstack/api/response/ApiResponseResponse.java 0.00% <0.00%> (-80.00%) ⬇️
.../cloudstack/discovery/ApiDiscoveryServiceImpl.java 13.93% <0.00%> (-55.25%) ⬇️
...rce/wrapper/LibvirtResizeVolumeCommandWrapper.java 49.50% <0.00%> (-27.17%) ⬇️
...rc/main/java/com/cloud/storage/StorageManager.java 76.92% <0.00%> (-23.08%) ⬇️
...pper/LibvirtPrepareForMigrationCommandWrapper.java 43.10% <0.00%> (-10.09%) ⬇️
.../cloud/hypervisor/kvm/storage/KVMPhysicalDisk.java 72.58% <0.00%> (-8.67%) ⬇️
...n/java/com/cloud/vm/VirtualMachineProfileImpl.java 31.03% <0.00%> (-6.11%) ⬇️
... and 221 more

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

@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Oct 3, 2022

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Oct 3, 2022

@DaanHoogland 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
Copy link

blueorangutan commented Oct 3, 2022

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

yadvr
yadvr reviewed Oct 4, 2022
View reviewed changes
...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java
try {
this._storageMapper.put(info.storagePoolType().toString(), storageAdaptor.newInstance());
s_logger.info(String.format("adding storage adaptor for %s", storageAdaptor.getName()));
this._storageMapper.put(info.storagePoolType().toString(), storageAdaptor.getDeclaredConstructor().newInstance());
Copy link
Member

@yadvr yadvr Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland pardon my ignorance, what does getDeclaredConstructor do?

Copy link
Contributor Author

@DaanHoogland DaanHoogland Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDeclaredConstructor().newInstance() does the same as newInstance() but in a non-deprecated way. I haven´t figured it out yet, but I don´t think this will solve the issue.

@DaanHoogland DaanHoogland mentioned this pull request Nov 3, 2022
Closed
@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Dec 2, 2022

this has become stale but the change is in line with java deprecation lines, so changing the title and re-testing.

@DaanHoogland DaanHoogland changed the title proper storage construction new (java11) style storage construction Dec 2, 2022
@DaanHoogland DaanHoogland reopened this Dec 2, 2022
@DaanHoogland DaanHoogland modified the milestones: 4.18.0.0, 4.19.0.0 Dec 13, 2022
@DaanHoogland DaanHoogland marked this pull request as ready for review April 28, 2023 07:25
JoaoJandre
JoaoJandre approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLGTM

@DaanHoogland DaanHoogland changed the base branch from main to 4.18 July 17, 2023 07:26
@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jul 17, 2023

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jul 17, 2023

@DaanHoogland a [SF] 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
Copy link

blueorangutan commented Jul 17, 2023

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

@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jul 18, 2023

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jul 18, 2023

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

@blueorangutan
Copy link

blueorangutan commented Jul 18, 2023

[SF] Trillian test result (tid-7103)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44527 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6797-t7103-kvm-centos7.zip
Smoke tests completed. 107 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_migrate_VM_and_root_volume Error 75.58 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 53.35 test_vm_life_cycle.py

@DaanHoogland DaanHoogland merged commit 0aade28 into apache:4.18 Jul 19, 2023
DaanHoogland added a commit that referenced this pull request Jul 19, 2023
@DaanHoogland
Merge release branch 4.18 to main
6bb95c0 
* 4.18:
  Storage and volumes statistics tasks for StorPool primary storage (#7404)
  proper storage construction (#6797)
  guarantee MAC uniqueness (#7634)
  server: allow migration of all VMs with local storage on KVM (#7656)
  Add L2 networks to Zones with SG (#7719)
@weizhouapache weizhouapache mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@JoaoJandre JoaoJandre JoaoJandre approved these changes

Assignees

No one assigned

Labels

component:kvm

Projects

None yet

Milestone

4.19.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@DaanHoogland @blueorangutan @yadvr @JoaoJandre