Update listVirtualMachines API documentation with 'projectid=-1' to list all VMs in projects.

[GabrielBrascher]

Description

[Update November 2021] PR Scope changed

Considering the reviews and discussion, I changed the scope of this PR to documentation update. I think that at least the documentation changes are mandatory as the documentation is not clear and already let to some internal issues.

Looking into the listVirtualMachines API command documentation there is the following description:

listall: If set to false, list only resources belonging to the command's caller; if set to true - list resources that the caller is authorized to see. Default value is false

The documentation is clear about the fact that when parameter listall=true then listVirtualMachines lists resources that the caller is authorized to see. Therefore, this PR considers that the ADMIN is authorized to see all VMs, including those on projects.

Fixes: #4267

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)

How Has This Been Tested?

I tested it via CloudMonkey and CloudStack UI. CloudMonkey output example:

cmk > list virtualmachines filter=id,account,project,name,state listall=true id account project name state 86ba5f14-b783-4e66-b511-ba617ca88d38 gabriel VM-86ba5f14-b783-4e66-b511-ba617ca88d38 Running 717f2e13-c207-4861-af0a-619ba1552f57 admin test-02 Running c15f35aa-2343-47c3-9d57-36b3ba16d885 gabriel VM-c15f35aa-2343-47c3-9d57-36b3ba16d885 Running 1b1047ed-8c29-40f5-bd2d-cd979adc668d admin ha-01-shared Running da787937-e3aa-409b-97b2-36ba4c45eb67 gabriel VM-da787937-e3aa-409b-97b2-36ba4c45eb67 Running a40b8e98-29c7-4bf4-8209-2563d9c3db48 testproject test-01 Running ca76ff8c-36f7-4183-b984-513106391db3 gabriel VM-ca76ff8c-36f7-4183-b984-513106391db3 Running 1f6b4fc6-f44f-4084-8ddd-14b916ada345 gabriel VM-1f6b4fc6-f44f-4084-8ddd-14b916ada345 Running e17b9817-bea1-4b98-a153-7b272ff99745 gabriel VM-e17b9817-bea1-4b98-a153-7b272ff99745 Running 75b2e67e-2bda-4a55-b84a-91e69476a893 gabriel VM-75b2e67e-2bda-4a55-b84a-91e69476a893 Running
4a226a80-8899-4ea7-80ea-220667c2b46d gabriel VM-4a226a80-8899-4ea7-80ea-220667c2b46d Running
eebb96fd-7320-4f91-8881-3c75aff9e9cc testproject vm-on-project01 Running
50215261-dbcb-47d7-bb3b-5e7626f1379f admin test-00 Running
7d314232-0c60-4b1f-89b6-e63626134628 admin test-u18-p3 Running
31f087dc-622a-412c-97cf-7e38d51643d0 admin tiny-py3 Running
54aa67ab-d106-47f8-8e3c-26a2496828d8 admin test-vr-py3 Running
85b3b81b-7ce4-4087-b791-a7e92f0c365c admin medium-local Running
701c73fb-35a1-4840-8db6-c90d41b95601 testproject vm-project-02 Running
38a2cfa3-02ba-44bb-92c6-6c5a32e38aa9 gabriel VM-38a2cfa3-02ba-44bb-92c6-6c5a32e38aa9 Running

[yadvr]

I think what you're trying to do is OK but could potentially break the listAPI for some environment. If you want we can show a toggle slider/radio in Primate list view which when checked can send the list VM API with listall=true and projectid=-1 which effectively would show the expected output.

[GabrielBrascher]

@rhtyd I understand your point. However, I think that either the documentation is incomplete or the listAll=true does not match with the documented/expected behavior.

Regarding the UI, I think that adding a toggle slider/radio in Primate would be nice indeed.

[wido]

@rhtyd The documentation currently states otherwise. The 'listall' parameter should now be called: 'listallexceptinstancesinprojects'

This seems like an oversight during implementation (a bug) rather then a feature.

[DaanHoogland]

@rhtyd @RodrigoDLopez@wido do this get your LGTMs?

[DaanHoogland]

cc @svenvogel ^^?

[RodrigoDLopez]

@rhtyd @RodrigoDLopez@wido do this get your LGTMs?

Hi @DaanHoogland
I agree with this approuch
Code LGTM

[wido]

I am LGTM with this. 'listAll' should do as it implies: List all Virtual Machines

We need to update the changelog properly though

[DaanHoogland]

We need to update the changelog properly though

what exactly do you have in mind?

[wido]

We need to update the changelog properly though

what exactly do you have in mind?

We need to mention something like:

The listAll parameter of listVirtualMachines now also lists all Instances which are under a project. Previously these would only be returned when *projectid=-1* was supplied as a parameter

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2359

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-3147)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32018 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4276-t3147-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestKubernetesSupportedVersion>:setup	Error	0.00	test_kubernetes_supported_versions.py

[DaanHoogland]

@GabrielBrascher can you have a look at #4469 and see if this is still needed or needs adjusting?

[GabrielBrascher]

@DaanHoogland PR #4469 regards listing projects, correct?

This PR is different, altering the listVirtualMachines command result in order to include all VMs from projects as well when the admin lists VMs with listall=true. The motivation is due to the API command listAll description: https://cloudstack.apache.org/api/apidocs-4.14/apis/listVirtualMachines.html

[nvazquez]

@GabrielBrascher maybe this can be solved by PR #4828?

[GabrielBrascher]

@nvazquez PR #4828 looks to be focused o the UI. This one proposes changes to the API and the scenario that this came was when calling the API; so the UI was not a target here.

@rhtyd I will build and check the UI with this patch soon(ish).

[GabrielBrascher]

@rhtyd I've tested it with the new UI and this would require changes in the UI as well, as it would be listing all VMs including from projects.

[PaulAngus]

So the API has a squiffy name, I think there are bigger things to worry about.
At worse add the info to the API description.

projectid=-1 gives the functional output required.

Why are we trying to change an API that isn't really broken, at the risk of unintended consequences?
Put an include 'projects' toggle on the UI - default it to 'on' if you like - and jobs done.

[GabrielBrascher]

@rhtyd @PaulAngus@wido@nvazquez updated this PR to address just the API documentation changes in order to address with a safer option.

In the medium-long term we might (or not) get back at changing this API implementation.

[yadvr]

@GabrielBrascher changed the milestone based on base main branch, if this is 4.16 pl fix PR base branch and advise.

[GabrielBrascher]

@rhtyd no problem in having this at 4.17.0.0.
This PR initially proposed changing the API, but later, after the feedback, I changed the scope to updating API documentation.; thus, ensuring that the projectid=-1 option is explicit and clear in the API documentation.

[DaanHoogland]

looks good, no code.

[sureshanaparti]

@blueorangutan package

[blueorangutan]

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[Pearl1594]

LGTM

[blueorangutan]

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

[sureshanaparti]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2655)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30342 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4276-t2655-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File