multi local storage handling for kvm

[DK101010]

Description

Is currently a POC, if someone have additional ideas or hints.

Motivation Users sometimes only have the option to use local storages, so they should have the option to use local storage like other storage types.

Implementation Decided to use already existing code to prevent breaks in current behavior(adapted agent code by user to add local storage) With this implementation it is possible to add and use local storage like other storage types.

Key properties to add a local storage Scope: HOST Protocol: Filesytem Path: mount path in KVM

Condition: Enable local storage for User VMs = true in zone detail settings

Types of changes

• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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

• [x] Major
• [ ] Minor

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [ ] Major
• [ ] Minor
• [ ] Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Currently only tested in own environment

[weizhouapache]

@DK101010

multiple local storage is supported since #6147 can you test it ?

[DK101010]

@DK101010

multiple local storage is supported since #6147 can you test it ?

@weizhouapache Damn, the last time I checked the PRs, I didn't see that one.... never mind

Hmm ... you implemented this via agent properties and my implementation works via UI like the other storages. For user acceptance reasons I prefer my solution. User can use local storages like other storage types without touch agent properties. Also no restarts required. Excuse me for insisting, but It will be hard to explain my users that they have to manipulate the agent properties instead to use the ui to add local storages but for other storages they have to use the ui. :/

Perhaps we find a compromise. But I'm afraid that our implementations are disjunct. Do you have an idea?

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[weizhouapache]

@DK101010 multiple local storage is supported since #6147 can you test it ?

@weizhouapache Damn, the last time I checked the PRs, I didn't see that one.... never mind

Hmm ... you implemented this via agent properties and my implementation works via UI like the other storages. For user acceptance reasons I prefer my solution. User can use local storages like other storage types without touch agent properties. Also no restarts required. Excuse me for insisting, but It will be hard to explain my users that they have to manipulate the agent properties instead to use the ui to add local storages but for other storages they have to use the ui. :/

Perhaps we find a compromise. But I'm afraid that our implementations are disjunct. Do you have an idea?

@DK101010

The reason I choose agent properties is, this can only be operated by administrator. they can do it by automation tools like chef, ansible, etc.

agree with you it would be nice to support it via api and UI, it will be more user-friendly. for example (1) add host with multiple local storage pools on UI (2) list all local storage pools of a host on UI (of course API as well) (3) add local storage pool to a host (create libvirt storage pool, update db and agent.properties) (4) remove local storage from a host (remove libvirt storage pool, update db and agent.properties)

1 is optional if 2,3,4 are supported. we can start by adding a host with only 1 local storage pool.

[DK101010]

@weizhouapache

I would also prefer to ignore number 1. For everything else, I can adapt the source code.

But I still have two questions. (on point 3. and 4. - create/remove libvirt storage pool ) Does it make sense that Cloudstack can create local storage? I had thought about that, but I came to the conclusion that it would be a break in logic. As far as I know (KVM, VMware) cloudstack does not create storages. CS only adds them to its own infrastructure.

Why do the agent properties have to be adapted? My PR works completely without the agent properties and so far I have not noticed any errors that would require an adjustment in the agent properties. I can add/remove local storages. Create/delete/migrate VM's.

[weizhouapache]

@weizhouapache

I would also prefer to ignore number 1. For everything else, I can adapt the source code.

But I still have two questions. (on point 3. and 4. - create/remove libvirt storage pool ) Does it make sense that Cloudstack can create local storage? I had thought about that, but I came to the conclusion that it would be a break in logic. As far as I know (KVM, VMware) cloudstack does not create storages. CS only adds them to its own infrastructure.

Why do the agent properties have to be adapted? My PR works completely without the agent properties and so far I have not noticed any errors that would require an adjustment in the agent properties. I can add/remove local storages. Create/delete/migrate VM's.

@DK101010 when cloudstack agent is started or restarted, it will (1) read the storage pools (uuid and path) in agent.properties (2) create libvirt storage pools (not the directory itself) if not exist.

you can check createLocalStoragePool method in https://github.com/apache/cloudstack/blob/main/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java#L3419

[weizhouapache]

@blueorangutan package

[blueorangutan]

@weizhouapache 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]

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 4284

[weizhouapache]

@DK101010 when the coding is done, please mark this "Ready for review"

[DK101010]

@DK101010 when the coding is done, please mark this "Ready for review"

@weizhouapache it isn't done. Currently I try to find out, why it is important to use the agent.properties file instead to create entries directly in the db via rest api.
Also open the libvirt part at these points. (Until now it is possible to attach, detach storages) (3) add local storage pool to a host (create libvirt storage pool, update db and agent.properties) (4) remove local storage from a host (remove libvirt storage pool, update db and agent.properties)

[codecov[bot]]

Codecov Report

Merging #6699 (5a938df) into main (861107f) will decrease coverage by 1.43%. Report is 16 commits behind head on main. The diff coverage is 21.70%.

@@             Coverage Diff              @@
##               main    #6699      +/-   ##
============================================
- Coverage     27.87%   26.44%   -1.43%     
+ Complexity    29354    27552    -1802     
============================================
  Files          5181     5186       +5     
  Lines        365434   365913     +479     
  Branches      53449    53519      +70     
============================================
- Hits         101855    96773    -5082     
- Misses       249482   255567    +6085     
+ Partials      14097    13573     -524     

Flag	Coverage Δ
simulator-marvin-tests	21.62% <24.77%> (-1.82%)	:arrow_down:
uitests	4.49% <0.00%> (-0.01%)	:arrow_down:
unit-tests	14.79% <0.00%> (+<0.01%)	:arrow_up:

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

Files	Coverage Δ
...loudstack/api/command/admin/host/ListHostsCmd.java	45.45% <0.00%> (-5.49%)	:arrow_down:
...main/java/com/cloud/agent/api/StoragePoolInfo.java	45.94% <0.00%> (-5.57%)	:arrow_down:
...api/command/admin/storage/ListStoragePoolsCmd.java	73.91% <33.33%> (-14.98%)	:arrow_down:
...ain/java/com/cloud/api/query/QueryManagerImpl.java	38.34% <30.76%> (-4.45%)	:arrow_down:
...cycle/CloudStackPrimaryDataStoreLifeCycleImpl.java	20.06% <21.05%> (-6.62%)	:arrow_down:
ui/src/views/infra/AddPrimaryStorage.vue	0.00% <0.00%> (ø)
...ain/java/com/cloud/storage/StorageManagerImpl.java	23.10% <26.08%> (-2.57%)	:arrow_down:

... and 714 files with indirect coverage changes

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[DK101010]

@weizhouapache long time ago but now finished and already in use in our environment. Feel free to test it. How it looks:

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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]

Packaging result [SF]: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: el9 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 6531

[DaanHoogland]

@DK101010 , you got a checkstyle error:

10:05:41 [ERROR] /jenkins/workspace/acs-centos7-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.19.0.0-SNAPSHOT/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java:74:8: Unused import - java.util.logging.Level. [UnusedImports]

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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]

Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6543

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

[SF] Trillian test result (tid-7143) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 41343 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6699-t7143-kvm-centos7.zip Smoke tests completed. 112 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	79.75	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	Error	55.43	test_vm_life_cycle.py

[DaanHoogland]

@weizhouapache , can you approve of this solution?

[weizhouapache]

@weizhouapache , can you approve of this solution?

@DaanHoogland I am working on other stuff, I will look into it

[shwstppr]

@weizhouapache were you able check this?

[weizhouapache]

@weizhouapache were you able check this?

@shwstppr sorry I could not find time for this.

[github-actions[bot]]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[DaanHoogland]

@DK101010 , can you address the conflicts?

[DK101010]

@DK101010 , can you address the conflicts?

I will take care of it