Introduce domainid and account parameter in createTemplate API by sudo87 · Pull Request #8210 · apache/cloudstack

sudo87 · 2023-11-09T14:47:35Z

Description

This PR introduces domainid and account as optional parameter for createTemplate API. It will allow admin to create templates for specified domain belonging to specific account.

Fixes: #5793 (comment)

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?

It's tested using 2 different users belonging to separate domains,

Create two domains - d1 and d2
Create normal user accounts in both domains - u1 and u2, associated them to d1 and d2 respectively
Deploy a VM, vm1, with u1, stop it; repeat steps with u2 account
Using ROOT admin try creating template using vol_u1, passing account u1 and domain d1
Using ROOT admin try creating template using vol_u1, passing account u2 and domain d2
Using ROOT admin try creating template using vol_u2, passing account u2 and domain d2
Using ROOT admin try creating template using vol_u2, passing account u1 and domain d1
Using ROOT admin try creating a template using vm1's volume and vm2's volume without passing domain and account

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

boring-cyborg · 2023-11-09T14:47:41Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

DaanHoogland

clgtm, good functionality enhancement and good refactor. just one suggestion for improvment

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java

codecov · 2023-11-09T15:21:09Z

Codecov Report

Merging #8210 (4732ed7) into main (2bb182c) will decrease coverage by 0.03%.
Report is 14 commits behind head on main.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##               main    #8210      +/-   ##
============================================
- Coverage     29.19%   29.17%   -0.03%     
- Complexity    31026    31044      +18     
============================================
  Files          5181     5186       +5     
  Lines        365434   365785     +351     
  Branches      53449    53487      +38     
============================================
+ Hits         106700   106712      +12     
- Misses       244113   244474     +361     
+ Partials      14621    14599      -22

Flag	Coverage Δ
simulator-marvin-tests	`25.14% <60.00%> (-0.03%)`	⬇️
uitests	`4.50% <ø> (ø)`
unit-tests	`14.79% <ø> (+<0.01%)`	⬆️

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

Files	Coverage Δ
...k/api/command/user/template/CreateTemplateCmd.java	`64.93% <60.00%> (+5.19%)`	⬆️

... and 45 files with indirect coverage changes

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

yadvr · 2023-11-09T17:11:56Z

@blueorangutan package

blueorangutan · 2023-11-09T17:14:03Z

@rohityadavcloud 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 · 2023-11-09T18:15:01Z

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

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java

shwstppr

Idea looks correct and consistent with ACS template behaviour as the templates are owned by an account and not a domain.
Added some comments on the code bits

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java

1. Created separate methods for access check and obtaining accountId 2. Included since property in params 3. Catching specific exceptions instead of all 4. Modified exception log to warn, included debug log 5. Removed projectId block as it's taken care in _accountService.finalyzeAccountId method

sudo87 · 2023-11-10T09:12:02Z

Performed tests (attached txt) with latest commit.
account_domainId_test.txt

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java

shwstppr · 2023-11-11T12:24:47Z

@blueorangutan package

blueorangutan · 2023-11-11T12:26:03Z

@shwstppr 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 · 2023-11-11T13:22:44Z

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

shwstppr · 2023-11-11T15:27:59Z

@blueorangutan test

blueorangutan · 2023-11-11T15:30:03Z

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

blueorangutan · 2023-11-12T09:09:02Z

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

Test	Result	Time (s)	Test File
ContextSuite context=TestSharedNetwork>:setup	`Error`	57.98	test_network.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3608.45	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	3615.47	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.03	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.78	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	1.24	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	65.78	test_kubernetes_clusters.py
test_08_migrate_vm	`Error`	43.75	test_vm_life_cycle.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	363.29	test_vpc_redundant.py

shwstppr · 2023-11-13T06:14:21Z

@blueorangutan package

blueorangutan · 2023-11-13T06:16:03Z

@shwstppr 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 · 2023-11-13T07:18:33Z

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

DaanHoogland · 2023-11-13T08:46:26Z

@blueorangutan test

blueorangutan · 2023-11-13T08:48:04Z

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

blueorangutan · 2023-11-14T00:58:35Z

[SF] Trillian test result (tid-8311)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 56734 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8210-t8311-kvm-centos7.zip
Smoke tests completed. 116 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_invalid_upgrade_kubernetes_cluster	`Failure`	3606.11	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	3610.05	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.04	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`	44.71	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	0.23	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	55.61	test_kubernetes_clusters.py

yadvr · 2023-11-14T07:41:42Z

@blueorangutan package

blueorangutan · 2023-11-14T07:42:04Z

@rohityadavcloud 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 · 2023-11-14T08:47:34Z

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

shwstppr

Code looks good and the last test run with k8s failures seem unrelated

DaanHoogland · 2023-11-16T08:21:00Z

@shwstppr do we need more testing on this or do we merge?

shwstppr · 2023-11-16T08:27:47Z

@DaanHoogland lets merge unless we have any other objections cc @rohityadavcloud

nvazquez

Code LGTM

shwstppr · 2023-11-16T12:06:48Z

Tested by creating a template for a account - userb from volume owned by a different account - usera,

(local) 🐱 > list volumes id=be5699fd-954a-4227-a415-25673e27f4fa 
{
  "count": 1,
  "volume": [
    {
      "account": "dom1usera",
      "created": "2023-11-16T12:00:31+0000",
      "destroyed": false,
      "deviceid": 0,
      "diskioread": 219,
      "diskiowrite": 0,
      "diskkbsread": 6786,
      "diskkbswrite": 0,
      "domain": "dom1",
      "domainid": "f5a6e69e-f43f-4059-93e7-020906edcf3f",
      "hasannotations": false,
      "id": "be5699fd-954a-4227-a415-25673e27f4fa",
      "isextractable": true,
      "name": "ROOT-3",
      "provisioningtype": "thin",
      "quiescevm": false,
      "serviceofferingdisplaytext": "Small Instance",
      "serviceofferingid": "1",
      "serviceofferingname": "Small Instance",
      "size": 8589934592,
      "state": "Ready",
      "supportsstoragesnapshot": false,
      "tags": [],
      "templatedisplaytext": "CentOS 5.5(64-bit) no GUI (KVM)",
      "templateid": "b2e36b1c-8475-11ee-b604-1e008c000114",
      "templatename": "CentOS 5.5(64-bit) no GUI (KVM)",
      "type": "ROOT",
      "virtualmachineid": "e4d9d330-6ff1-4886-b7f2-f525a7d5a86c",
      "vmdisplayname": "VM-e4d9d330-6ff1-4886-b7f2-f525a7d5a86c",
      "vmname": "VM-e4d9d330-6ff1-4886-b7f2-f525a7d5a86c",
      "vmstate": "Stopped",
      "vmtype": "User",
      "zoneid": "0f93b47f-a703-430d-a09c-76844be411a7",
      "zonename": "pr8210-t8335-kvm-centos7"
    }
  ]
}
(local) 🐱 > create template volumeid=be5699fd-954a-4227-a415-25673e27f4fa account=dom1userb domainid=f5a6e69e-f43f-4059-93e7-020906edcf3f ostypeid=b2f8d0c5-8475-11ee-b604-1e008c000114 name=test-temp
{
  "template": {
    "account": "dom1userb",
    "bits": 0,
    "created": "2023-11-16T12:03:29+0000",
    "crossZones": false,
    "deployasis": false,
    "details": {
      "Message.ReservedCapacityFreed.Flag": "false",
      "cpuOvercommitRatio": "2.0",
      "memoryOvercommitRatio": "1.0"
    },
    "directdownload": false,
    "displaytext": "test-temp",
    "domain": "dom1",
    "domainid": "f5a6e69e-f43f-4059-93e7-020906edcf3f",
    "downloaddetails": [
      {
        "datastore": "NFS://10.0.32.4/acs/secondary/pr8210-t8335-kvm-centos7/pr8210-t8335-kvm-centos7-sec1",
        "downloadPercent": "100",
        "downloadState": "DOWNLOADED"
      }
    ],
    "format": "QCOW2",
    "hasannotations": false,
    "hypervisor": "KVM",
    "id": "6ce0dbbf-eae6-487e-9a8d-c4ca75a8a561",
    "isdynamicallyscalable": false,
    "isextractable": true,
    "isfeatured": false,
    "ispublic": false,
    "isready": true,
    "name": "test-temp",
    "ostypeid": "b2f8d0c5-8475-11ee-b604-1e008c000114",
    "ostypename": "CentOS 5.5 (64-bit)",
    "passwordenabled": false,
    "physicalsize": 1510342656,
    "requireshvm": true,
    "size": 8589934592,
    "sourcetemplateid": "b2e36b1c-8475-11ee-b604-1e008c000114",
    "sshkeyenabled": false,
    "tags": [],
    "templatetype": "USER",
    "zoneid": "0f93b47f-a703-430d-a09c-76844be411a7",
    "zonename": "pr8210-t8335-kvm-centos7"
  }
}
(local) 🐱 > set username dom1userb
(local) 🐱 > sync
Discovered 314 APIs
(local) 🐱 > list templates templatefilter=self
{
  "count": 1,
  "template": [
    {
      "account": "dom1userb",
      "bits": 0,
      "created": "2023-11-16T12:03:29+0000",
      "crossZones": false,
      "deployasis": false,
      "details": {
        "Message.ReservedCapacityFreed.Flag": "false",
        "cpuOvercommitRatio": "2.0",
        "memoryOvercommitRatio": "1.0"
      },
      "directdownload": false,
      "displaytext": "test-temp",
      "domain": "dom1",
      "domainid": "f5a6e69e-f43f-4059-93e7-020906edcf3f",
      "downloaddetails": [
        {
          "datastore": "NFS://10.0.32.4/acs/secondary/pr8210-t8335-kvm-centos7/pr8210-t8335-kvm-centos7-sec1",
          "downloadPercent": "100",
          "downloadState": "DOWNLOADED"
        }
      ],
      "format": "QCOW2",
      "hasannotations": false,
      "hypervisor": "KVM",
      "id": "6ce0dbbf-eae6-487e-9a8d-c4ca75a8a561",
      "isdynamicallyscalable": false,
      "isextractable": true,
      "isfeatured": false,
      "ispublic": false,
      "isready": true,
      "name": "test-temp",
      "ostypeid": "b2f8d0c5-8475-11ee-b604-1e008c000114",
      "ostypename": "CentOS 5.5 (64-bit)",
      "passwordenabled": false,
      "physicalsize": 1510342656,
      "requireshvm": true,
      "size": 8589934592,
      "sourcetemplateid": "b2e36b1c-8475-11ee-b604-1e008c000114",
      "sshkeyenabled": false,
      "status": "Download Complete",
      "tags": [],
      "templatetype": "USER",
      "zoneid": "0f93b47f-a703-430d-a09c-76844be411a7",
      "zonename": "pr8210-t8335-kvm-centos7"
    }
  ]
}

shwstppr · 2023-11-16T12:08:44Z

Merging this. We may need to add UI support for new params at some point. @sudo87 feel free to open a new PR if you wish to do so.

boring-cyborg · 2023-11-16T12:09:42Z

Awesome work, congrats on your first merged pull request!

sudo87 · 2023-11-16T12:55:05Z

Thanks @shwstppr @DaanHoogland for merging the pr.

This PR includes Domain and Account parameters in CreateTemplate form in UI for Admin account. It will expose optional parameters domainId and account in UI supported by createTemplate API #8210

…pache#8210) Introduces domainid and account as optional parameter for createTemplate API. It will allow admin to create templates for specified domain belonging to specific account.

…#8257) This PR includes Domain and Account parameters in CreateTemplate form in UI for Admin account. It will expose optional parameters domainId and account in UI supported by createTemplate API apache#8210

Introduce domainid and account parameter in createTemplate API

725432d

boring-cyborg bot added the component:api label Nov 9, 2023

DaanHoogland approved these changes Nov 9, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java Outdated Show resolved Hide resolved

yadvr requested a review from shwstppr November 9, 2023 17:10

shwstppr reviewed Nov 9, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java Outdated Show resolved Hide resolved

shwstppr reviewed Nov 9, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java Outdated Show resolved Hide resolved

shwstppr reviewed Nov 9, 2023

View reviewed changes

DaanHoogland reviewed Nov 10, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/user/template/CreateTemplateCmd.java Outdated Show resolved Hide resolved

Remove redundant comment

4732ed7

yadvr requested review from harikrishna-patnala and nvazquez November 14, 2023 07:41

yadvr requested a review from weizhouapache November 14, 2023 07:41

yadvr added this to the 4.19.0.0 milestone Nov 14, 2023

shwstppr approved these changes Nov 16, 2023

View reviewed changes

nvazquez approved these changes Nov 16, 2023

View reviewed changes

shwstppr merged commit 0735b91 into apache:main Nov 16, 2023

sudo87 mentioned this pull request Nov 21, 2023

Include domain and account field for createTemplate UI form #8257

Merged

13 tasks

Conversation

sudo87 commented Nov 9, 2023

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!

boring-cyborg bot commented Nov 9, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yadvr commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 9, 2023

Uh oh!

blueorangutan commented Nov 9, 2023

Uh oh!

Uh oh!

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sudo87 commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shwstppr commented Nov 11, 2023

Uh oh!

blueorangutan commented Nov 11, 2023

Uh oh!

blueorangutan commented Nov 11, 2023

Uh oh!

shwstppr commented Nov 11, 2023

Uh oh!

blueorangutan commented Nov 11, 2023

Uh oh!

blueorangutan commented Nov 12, 2023

Uh oh!

shwstppr commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 13, 2023

Uh oh!

DaanHoogland commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 13, 2023

Uh oh!

blueorangutan commented Nov 14, 2023

Uh oh!

yadvr commented Nov 14, 2023

Uh oh!

blueorangutan commented Nov 14, 2023

Uh oh!

blueorangutan commented Nov 14, 2023

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Nov 16, 2023

Uh oh!

shwstppr commented Nov 16, 2023

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Nov 16, 2023

codecov bot commented Nov 9, 2023 •

edited

Loading

sudo87 commented Nov 10, 2023 •

edited

Loading