Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dynamic secondary storage selection #7659

Merged
merged 16 commits into from
Dec 4, 2023

Conversation

BryanMLima
Copy link
Contributor

@BryanMLima BryanMLima commented Jun 20, 2023

Description

This PR aims to add the possibility to direct resources (Volumes, Templates, Snapshots and ISOs) to a specific secondary storage through rules written in JavaScript that will only affect new allocated resources. This feature is fully described in issue #7654.

Fixes: #7654

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

How Has This Been Tested?

Unit tests were added to validate the functionality of the implemented methods. Besides this, the overall functionality was tested in a local lab with two secondary storage available, creating basic JS rules and validating that the resource was allocated according to the heuristic rule. Below, there are some examples used in the tests, based on the variables available for each resource type specified in the specification of the feature proposal.

  1. Allocate a resource type to a specific secondary storage.
function findStorageWithSpecificId(pool) {
	return pool.id === '7432f961-c602-4e8e-8580-2496ffbbc45d';
}

secondaryStorages.filter(findStorageWithSpecificId)[0].id
  1. Dedicate storage pools for a type of template format.
function directToDedicatedQCOW2Pool(pool) {
  return pool.id === '7432f961-c602-4e8e-8580-2496ffbbc45d';
}

function directToDedicatedVHDPool(pool) {
  return pool.id === '1ea0109a-299d-4e37-8460-3e9823f9f25c';
}

if (template.format === 'QCOW2') {
  secondaryStorages.filter(directToDedicatedQCOW2Pool)[0].id
} else if (template.format === 'VHD') {
  secondaryStorages.filter(directToDedicatedVHDPool)[0].id
}
  1. Direct snapshot of volumes with the KVM hypervisor to a specific secondary storage.
if (snapshot.hypervisorType === 'KVM') {
  '7432f961-c602-4e8e-8580-2496ffbbc45d';
}
  1. Direct resources to a specific domain:
if (account.domain.id == '52d83793-26de-11ec-8dcf-5254005dcdac') {
  '1ea0109a-299d-4e37-8460-3e9823f9f25c'
} else if (account.domain.id == 'c1186146-5ceb-4901-94a1-dd1d24bd849d') {
  '7432f961-c602-4e8e-8580-2496ffbbc45d'
}

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@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

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

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

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

Comparison is base (5651eab) 18.25% compared to head (70e1705) 28.93%.

Files Patch % Lines
.../com/cloud/template/HypervisorTemplateAdapter.java 45.45% 47 Missing and 1 partial ⚠️
...dstack/storage/heuristics/HeuristicRuleHelper.java 50.00% 46 Missing and 1 partial ⚠️
...i/response/SecondaryStorageHeuristicsResponse.java 0.00% 34 Missing ⚠️
...ain/java/com/cloud/storage/StorageManagerImpl.java 0.00% 31 Missing ⚠️
.../org/apache/cloudstack/secstorage/HeuristicVO.java 20.83% 19 Missing ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java 0.00% 17 Missing ⚠️
...e/heuristics/presetvariables/SecondaryStorage.java 0.00% 17 Missing ⚠️
.../heuristics/CreateSecondaryStorageSelectorCmd.java 0.00% 14 Missing ⚠️
.../heuristics/UpdateSecondaryStorageSelectorCmd.java 0.00% 11 Missing ⚠️
...e/heuristics/ListSecondaryStorageSelectorsCmd.java 0.00% 9 Missing ⚠️
... and 14 more
Additional details and impacted files
@@              Coverage Diff              @@
##               main    #7659       +/-   ##
=============================================
+ Coverage     18.25%   28.93%   +10.68%     
- Complexity    17824    31007    +13183     
=============================================
  Files          5100     5251      +151     
  Lines        346393   368752    +22359     
  Branches      49768    53759     +3991     
=============================================
+ Hits          63217   106682    +43465     
+ Misses       274118   247425    -26693     
- Partials       9058    14645     +5587     
Flag Coverage Δ
simulator-marvin-tests 24.71% <7.90%> (+5.23%) ⬆️
uitests 4.44% <ø> (ø)
unit-tests 14.90% <35.21%> (?)

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.

@DaanHoogland
Copy link
Contributor

hey @BryanMLima , I saw the issue with functional description that came with this. I haven't yet read it and just reacted to the lint output:

Check hooks apply to the repository......................................Passed
check for case conflicts.................................................Passed
check vcs permalinks.....................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/PresetVariables.java
Fixing api/src/main/java/org/apache/cloudstack/api/command/admin/storage/heuristics/CreateSecondaryStorageSelectorCmd.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Snapshot.java
Fixing api/src/main/java/org/apache/cloudstack/api/command/admin/storage/heuristics/UpdateSecondaryStorageSelectorCmd.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/HeuristicRuleHelper.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/GenericHeuristicPresetVariable.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/dao/SecondaryStorageHeuristicDaoImpl.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/HeuristicVO.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/SecondaryStorage.java
Fixing api/src/main/java/org/apache/cloudstack/secstorage/heuristics/HeuristicPurpose.java
Fixing api/src/main/java/org/apache/cloudstack/api/response/SecondaryStorageHeuristicsResponse.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/dao/SecondaryStorageHeuristicDao.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Volume.java
Fixing api/src/main/java/org/apache/cloudstack/secstorage/heuristics/Heuristic.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Template.java

mixed line ending........................................................Passed

@BryanMLima
Copy link
Contributor Author

hey @BryanMLima , I saw the issue with functional description that came with this. I haven't yet read it and just reacted to the lint output:

Check hooks apply to the repository......................................Passed
check for case conflicts.................................................Passed
check vcs permalinks.....................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/PresetVariables.java
Fixing api/src/main/java/org/apache/cloudstack/api/command/admin/storage/heuristics/CreateSecondaryStorageSelectorCmd.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Snapshot.java
Fixing api/src/main/java/org/apache/cloudstack/api/command/admin/storage/heuristics/UpdateSecondaryStorageSelectorCmd.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/HeuristicRuleHelper.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/GenericHeuristicPresetVariable.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/dao/SecondaryStorageHeuristicDaoImpl.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/HeuristicVO.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/SecondaryStorage.java
Fixing api/src/main/java/org/apache/cloudstack/secstorage/heuristics/HeuristicPurpose.java
Fixing api/src/main/java/org/apache/cloudstack/api/response/SecondaryStorageHeuristicsResponse.java
Fixing engine/schema/src/main/java/org/apache/cloudstack/secstorage/dao/SecondaryStorageHeuristicDao.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Volume.java
Fixing api/src/main/java/org/apache/cloudstack/secstorage/heuristics/Heuristic.java
Fixing server/src/main/java/org/apache/cloudstack/storage/heuristics/presetvariables/Template.java

mixed line ending........................................................Passed

Thanks, I always forget about these.

@github-actions
Copy link

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

Copy link
Member

@vishesh92 vishesh92 left a comment

Choose a reason for hiding this comment

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

clgtm.

Copy link

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

@DaanHoogland
Copy link
Contributor

@BryanMLima , conflict :( can you resolve it?

@rajujith
Copy link
Collaborator

rajujith commented Nov 15, 2023

@BryanMLima any documentation available that provides some guidance on the usage of this feature? Any plans to implement a UI?

Copy link
Collaborator

@rajujith rajujith left a comment

Choose a reason for hiding this comment

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

@BryanMLima

  1. I could dedicate a secondary storage for KVM snapshots.
  2. I could dedicate a secondary storage for a domain.
  3. I was unable to dedicate another secondary storage for a second domain in the same zone. I believe this might be a limitation?
  4. I faced the below issues while registering template with the following rules:
(SS-SelectorPR) 🐱 > create secondarystorageselector name=KVMtemplatesonly description=KVMtemplatesonly type=TEMPLATE zoneid=a2a9fafd-43fb-4508-915e-d3c836a7e972 heuristicrule="function directToDedicatedQCOW2Pool(pool) {return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599';} if (template.format === 'QCOW2') {secondaryStorages.filter(directToDedicatedQCOW2Pool)[0].uuid}"

{
  "heuristics": {
    "created": "2023-11-15T11:42:50+0000",
    "description": "KVMtemplatesonly",
    "heuristicrule": "function directToDedicatedQCOW2Pool(pool) {return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599';} if (template.format === 'QCOW2') {secondaryStorages.filter(directToDedicatedQCOW2Pool)[0].uuid}",
    "id": "1d9070e9-5d12-4159-9e7c-de0793c87e24",
    "name": "KVMtemplatesonly",
    "type": "TEMPLATE",
    "zoneid": "a2a9fafd-43fb-4508-915e-d3c836a7e972"
  }
}

On register template:

 due to [javax.script.ScriptException: TypeError: Cannot read property "uuid" from undefined in <eval> at line number 1]
        at org.apache.cloudstack.utils.jsinterpreter.JsInterpreter.executeScriptInThread(JsInterpreter.java:121)

Specific UUID:

create secondarystorageselector name=KVMtemplatesonly-specifc description=KVMtemplatesonly-specific type=TEMPLATE zoneid=a2a9fafd-43fb-4508-915e-d3c836a7e972 heuristicrule="function findStorageWithSpecificUuid(pool) { return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599'; } secondaryStorages.filter(findStorageWithSpecificUuid)[0].uuid"
On register template:

java.util.concurrent.ExecutionException: javax.script.ScriptException: TypeError: Cannot read property "uuid" from undefined in <eval> at line number 1
        at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)

@BryanMLima
Copy link
Contributor Author

@BryanMLima any documentation available that provides some guidance on the usage of this feature? Any plans to implement a UI?

@rajujith currently, the only documentation (more like a feature specification) is described in issue #7654; though, I pretend to add a proper documentation of this feature in the official documentation of CloudStack. Regarding the implementation of the UI for this feature, it is planned, however, it is not a priority at the moment.

@BryanMLima

1. I could dedicate a secondary storage for KVM snapshots.

2. I could dedicate a secondary storage for a domain.

3. I was unable to dedicate another secondary storage for a second domain in the same zone. I believe this might be a limitation?

4. I faced the below issues while registering template with the following rules:
(SS-SelectorPR) 🐱 > create secondarystorageselector name=KVMtemplatesonly description=KVMtemplatesonly type=TEMPLATE zoneid=a2a9fafd-43fb-4508-915e-d3c836a7e972 heuristicrule="function directToDedicatedQCOW2Pool(pool) {return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599';} if (template.format === 'QCOW2') {secondaryStorages.filter(directToDedicatedQCOW2Pool)[0].uuid}"

{
  "heuristics": {
    "created": "2023-11-15T11:42:50+0000",
    "description": "KVMtemplatesonly",
    "heuristicrule": "function directToDedicatedQCOW2Pool(pool) {return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599';} if (template.format === 'QCOW2') {secondaryStorages.filter(directToDedicatedQCOW2Pool)[0].uuid}",
    "id": "1d9070e9-5d12-4159-9e7c-de0793c87e24",
    "name": "KVMtemplatesonly",
    "type": "TEMPLATE",
    "zoneid": "a2a9fafd-43fb-4508-915e-d3c836a7e972"
  }
}

On register template:

 due to [javax.script.ScriptException: TypeError: Cannot read property "uuid" from undefined in <eval> at line number 1]
        at org.apache.cloudstack.utils.jsinterpreter.JsInterpreter.executeScriptInThread(JsInterpreter.java:121)

Specific UUID:

create secondarystorageselector name=KVMtemplatesonly-specifc description=KVMtemplatesonly-specific type=TEMPLATE zoneid=a2a9fafd-43fb-4508-915e-d3c836a7e972 heuristicrule="function findStorageWithSpecificUuid(pool) { return pool.uuid === '92dc82ff-c3cc-4756-928f-6802c358f599'; } secondaryStorages.filter(findStorageWithSpecificUuid)[0].uuid" On register template:

java.util.concurrent.ExecutionException: javax.script.ScriptException: TypeError: Cannot read property "uuid" from undefined in <eval> at line number 1
        at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)

@rajujith thanks for testing this PR.

Regarding item 3), could you provide more details of this testing? This feature only allows one heuristic rule per type, if you tried to dedicate a secondary storage to a domain for a specific type, you need to edit the same heuristic rule to dedicate to another domain.
Concisely, all heuristic rules for a given type should be in the same secondary storage selector, otherwise, an exception will be thrown and the message There is already a heuristic rule in the specified Zone {...} with the type [...]. will be presented.

In the future, this feature could allow multiple heuristic rules for the same type in a zone, allowing operators to specify the order in which each rule would be verified.

Regarding item 4), the description of this PR and the issue presented the uuid for the secondarystorage variable in the JS script; however, it is the id that is provided in the script. I apologise for the confusion, I already fixed this in both the PR and the issue description's.

Copy link

github-actions bot commented Dec 1, 2023

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

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

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

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

@DaanHoogland
Copy link
Contributor

@blueorangutan rest

@BryanMLima
Copy link
Contributor Author

@blueorangutan rest

Even the CI needs to rest a bit, right? 😂

@vishesh92
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

[SF] Trillian test result (tid-8451)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 58299 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7659-t8451-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_01_invalid_upgrade_kubernetes_cluster Failure 3611.13 test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster Failure 3618.21 test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster Failure 0.05 test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster Failure 0.04 test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster Failure 0.06 test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster Failure 0.04 test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster Failure 50.03 test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle Error 0.24 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 58.74 test_kubernetes_clusters.py
test_08_migrate_vm Error 0.05 test_vm_life_cycle.py

@rajujith
Copy link
Collaborator

rajujith commented Dec 4, 2023

@BryanMLima I updated the rules for scenarios 3) and 4). Now they are working fine.

list secondarystorageselectors zoneid=a2a9fafd-43fb-4508-915e-d3c836a7e972
{
"count": 2,
"heuristics": [
{
"created": "2023-12-04T08:39:01+0000",
"description": "KVMtemplatesonly-specific",
"heuristicrule": "function findStorageWithSpecificId(pool) { return pool.id === '92dc82ff-c3cc-4756-928f-6802c358f599'; } secondaryStorages.filter(findStorageWithSpecificId)[0].id",
"id": "2fa48314-1a01-4d17-a16f-78c4719fdc4c",
"name": "KVMtemplatesonly-specifc",
"type": "TEMPLATE",
"zoneid": "a2a9fafd-43fb-4508-915e-d3c836a7e972"
},
{
"created": "2023-12-04T08:45:23+0000",
"description": "MultiDomain",
"heuristicrule": "if (account.domain.id == '3df7450a-db07-4bfd-a54b-fa0716013917') {'92dc82ff-c3cc-4756-928f-6802c358f599'} else if {account.domain.id == '7db040fa-1ebe-404e-b15c-fbc9c60118d4') {fc9f710e-452f-4233-aa51-91747e5bb071}",
"id": "7757ba9d-34e0-40e6-9e6a-47dac301cadb",
"name": "Multi-Domain-Dedication",
"type": "SNAPSHOT",
"zoneid": "a2a9fafd-43fb-4508-915e-d3c836a7e972"
}
]
}

Copy link
Collaborator

@rajujith rajujith left a comment

Choose a reason for hiding this comment

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

LGTM.

@DaanHoogland DaanHoogland merged commit b0910fc into apache:main Dec 4, 2023
26 checks passed
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Dynamic Secondary Storage Selectors
5 participants