api: Fix count and item issues returned by list APIs #3894

Pearl1594 · 2020-02-18T13:12:42Z

Description

The count value returned in the APIs responses is incorrect
Allow listing of all resources for non-user accounts when listall=true and projectid=-1
listInfrastructure API to return count of internal LBs and Alerts in addition to the existing details returned

Addresses: #3890

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)

rohityadavcloud · 2020-02-18T14:23:28Z

plugins/metrics/src/main/java/org/apache/cloudstack/metrics/MetricsServiceImpl.java

        response.setImageStores(imageStoreDao.listImageStores().size());
        response.setSystemvms(vmInstanceDao.listByTypes(VirtualMachine.Type.ConsoleProxy, VirtualMachine.Type.SecondaryStorageVm).size());
-        response.setRouters(domainRouterDao.listAll().size());
+        response.setRouters(domainRouterDao.listByRole(VirtualRouter.Role.VIRTUAL_ROUTER).size());


@Pearl1594 calling listAll() i.e. getting all the rows will make the APIs slow in a large env; this is fine but can you explore a way to simply get the count (instead of the rows).

rohityadavcloud · 2020-02-18T14:23:51Z

plugins/metrics/src/main/java/org/apache/cloudstack/metrics/MetricsServiceImpl.java

-        response.setRouters(domainRouterDao.listAll().size());
+        response.setRouters(domainRouterDao.listByRole(VirtualRouter.Role.VIRTUAL_ROUTER).size());
+        response.setInternalLbs(domainRouterDao.listByRole(VirtualRouter.Role.INTERNAL_LB_VM).size());
+        response.setAlerts(alertDao.listAll().size());


Same as above ^^ if it may be done quickly.

rohityadavcloud · 2020-02-18T14:25:38Z

I'll test against Primate
@blueorangutan package

blueorangutan · 2020-02-18T14:26:37Z

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

rohityadavcloud

LGTM, pending testing.

rohityadavcloud · 2020-02-18T14:28:12Z

blueorangutan · 2020-02-18T14:43:35Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-895

weizhouapache · 2020-02-18T14:47:57Z

server/src/main/java/com/cloud/server/ManagementServerImpl.java

+            allHostsPair = searchForServersExcluding(startIndex, pageSize, null, hostType, null, srcHost.getDataCenterId(), null, null, srcHostId, keyword, null, null, srcHost.getHypervisorType(),
                    srcHost.getHypervisorVersion());
            allHosts = allHostsPair.first();
            allHosts.remove(srcHost);


@Pearl1594 can you share why you add the method searchForServersExcluding ?

source host is removed in line

allHosts.remove(srcHost);

@weizhouapache True, however, it was noticed that, say if there were 15 hosts, with a default pagesize of 10, the 1st page (considering it contained the src host) will have 9 objects returned and the 2nd page will have the remaining 5, as opposed to the ideal behavior of the 1st page containing 10 items and the remaining 4 being listed on page 2.

rohityadavcloud · 2020-02-18T15:51:49Z

@blueorangutan test

blueorangutan · 2020-02-18T15:52:38Z

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

rohityadavcloud · 2020-02-18T16:19:44Z

server/src/main/java/com/cloud/server/ManagementServerImpl.java

        return suitablePools;
    }

+


@Pearl1594 curb adding newlines

rohityadavcloud

LGTM, tested against Primate, and tested all the listed APIs per the test plan.

rohityadavcloud · 2020-02-18T17:27:46Z

@blueorangutan package

blueorangutan · 2020-02-18T17:28:38Z

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

blueorangutan · 2020-02-18T17:45:21Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-896

rohityadavcloud · 2020-02-18T18:11:58Z

@blueorangutan test

blueorangutan · 2020-02-18T18:12:38Z

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

rohityadavcloud · 2020-02-18T19:21:50Z

@blueorangutan package

blueorangutan · 2020-02-18T19:22:38Z

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

blueorangutan · 2020-02-18T19:38:40Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-897

rohityadavcloud · 2020-02-19T07:35:28Z

@blueorangutan test

blueorangutan · 2020-02-19T07:36:04Z

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

weizhouapache · 2020-02-19T08:27:11Z

@rhtyd can you revert the commits related to list projects resource ?
It seems to be different issue

blueorangutan · 2020-02-24T05:53:08Z

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

weizhouapache · 2020-02-24T11:34:09Z

tested with the following setting
(1) login as root admin, create a network/vm
(2) login as domain admin of domain test1, create a network/vm.
(3) login as domain admin of domain test1, create a project, create a network/vm in project
(4) login as user1 in test1, create a network/vm.
(5) login as user1 in test1, create a project, create a network/vm in project

verified following apis
(1) list virtualmachines
(2) list volumes
(3) list nics
(5) list publicipaddresses
(6) list networks
(7) list routers

results
(1) root admin is able to list resources in projects of sub-domains, with projectid=-1 (but listall=false, @rhtyd is this expected result ? )
if listall =true and projectid=-1, it returns resources in projects and non-projects. seems good.

(2) list networks as root admin,
projectid=-1/listall=false, return nothing (need to be fixed)
projectid=-1/listall=true, return isolated networks in projects, and shared network for projects
listall=true, return networks not in/for projects.

(3) list routers as domain admin,
projectid=-1, returns routers of isolated networks in projects. should it be allowed ?

(4) list resource as domain admin
projectid=-1, returns vms/volumes in projects
listall=true, returns vms/volumes in non-projects
listall =true and projectid=-1, it returns vms/volumes in projects (vms in non-projects are not returned. @rhtyd it this expected result ? )

(5) list networks as domain admin
listall=true/false, returns all isolated/shared networks this domain admin can access.
projectid=-1, returns all isolated/shared networks a project in the domain can access.
projectid=-1 and listall=true, same as above (projectid=-1).

(6) list resources as normal user (same as domain admin)
projectid=-1, returns vms/volumes in projects
listall=true, returns vms/volumes in non-projects
listall =true and projectid=-1, it returns vms/volumes in projects (vms in non-projects are not returned. @rhtyd it this expected result ? )

(7) list networks as normal user (same as domain admin)
listall=true/false, returns all isolated/shared networks this user can access.
projectid=-1, returns all isolated/shared networks a project this user manages can access.
projectid=-1 and listall=true, same as above (projectid=-1).

weizhouapache · 2020-02-24T11:41:58Z

@rhtyd @DaanHoogland @Pearl1594
again, I would suggest you to split it to two PRs.
The change to list all resource in projects is small, but it requires much more testing.
unless you want to merge this into master now, and fix the issues afterwards.

If possible, please add a component/smoke test.

rohityadavcloud · 2020-02-24T11:50:11Z

@weizhouapache let me back to you on your comments soon, did you test/compare against 4.13/master? Wrt the listall=true&project-id change, this restricted to the root admin here: https://github.com/apache/cloudstack/pull/3894/files#diff-44c2e394637af1d7c556dac5575ba8d3R2683 Given this fix, there shouldn't be any change any non-admin accounts

blueorangutan · 2020-02-24T20:10:23Z

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

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	182.66	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	171.22	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	260.92	test_privategw_acl.py

rohityadavcloud · 2020-02-25T08:58:34Z

Signed-off-by: Rohit Yadav <[email protected]>

rohityadavcloud · 2020-02-25T09:21:02Z

@weizhouapache I tried to look at git history, there was an attempt to fix similar issue for listVpcs #2352 but as a domain admin I'm still able to listVpcs of projects not owned/managed by the domain admin.

I'll be updating http://primate-qa.cloudstack.cloud:8080/client/ shortly with this PR and carry tests on the APIs listed in previous comment (will tick those which passes excepted result).

rohityadavcloud · 2020-02-25T09:21:15Z

@blueorangutan package

blueorangutan · 2020-02-25T09:22:13Z

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

weizhouapache · 2020-02-25T09:33:03Z

@rhtyd thanks for your update and quick fix !

blueorangutan · 2020-02-25T09:41:03Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-947

rohityadavcloud · 2020-02-25T10:52:49Z

@blueorangutan test

blueorangutan · 2020-02-25T10:53:14Z

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

rohityadavcloud · 2020-02-25T12:17:03Z

Quick test, for example the listing of ips:

Before: (domain admin sees ips from other projects)

(qaprimate) 🐱 > list publicipaddresses  listall=true projectid=-1
{
  "count": 3,
  "publicipaddress": [
    {
      "allocated": "2020-02-17T16:33:27+0100",
      "associatednetworkid": "0ac48c1b-e2bc-4462-8dfd-cbdb3bd71390",
      "associatednetworkname": "TestUserProjectNetwork",
      "domain": "ROOT",
      "domainid": "fa323836-4f04-11ea-b408-1e006800018c",
      "forvirtualnetwork": true,
      "id": "26e2d0f7-476d-4598-a2c8-4b59d29aaf8e",
      "ipaddress": "192.168.2.195",
      "isportable": false,
      "issourcenat": true,
      "isstaticnat": false,
      "issystem": false,
      "networkid": "ecd874e8-9c14-4a66-8528-c4a11960c7be",
      "physicalnetworkid": "08344756-9abc-41c0-b575-8ca1a0a7c77d",
      "project": "TestUserProject",
      "projectid": "5660c407-167c-480d-9825-5b54d63797b7",
      "state": "Allocated",
      "tags": [],
      "zoneid": "91b49be0-5a8b-4f3e-83b8-3bdfee4eba78",
      "zonename": "Sandbox-simulator"
    },
    {
      "allocated": "2020-02-19T13:32:44+0100",
      "associatednetworkid": "22272dc5-65fb-439e-af3d-c2884f3101c8",
      "associatednetworkname": "SamlUserProjectNet",
      "domain": "ROOT",
      "domainid": "fa323836-4f04-11ea-b408-1e006800018c",
      "forvirtualnetwork": true,
      "id": "3574b63a-5dd9-4d1d-8c79-91de2952203d",
      "ipaddress": "192.168.2.168",
      "isportable": false,
      "issourcenat": true,
      "isstaticnat": false,
      "issystem": false,
      "networkid": "ecd874e8-9c14-4a66-8528-c4a11960c7be",
      "physicalnetworkid": "08344756-9abc-41c0-b575-8ca1a0a7c77d",
      "project": "SamlUserProject",
      "projectid": "879a2fe0-639f-4709-a405-5d145d7625f7",
      "state": "Allocated",
      "tags": [],
      "zoneid": "91b49be0-5a8b-4f3e-83b8-3bdfee4eba78",
      "zonename": "Sandbox-simulator"
    },
    {
      "allocated": "2020-02-25T09:39:16+0100",
      "associatednetworkid": "493824ac-12d5-43c6-b6d6-2246270cf0a0",
      "associatednetworkname": "testdomain-project-net",
      "domain": "Domain",
      "domainid": "d56ba667-945e-46be-8829-e21df75af531",
      "forvirtualnetwork": true,
      "id": "aea77e18-99b8-4a66-8cb5-15e6848134f6",
      "ipaddress": "192.168.2.121",
      "isportable": false,
      "issourcenat": true,
      "isstaticnat": false,
      "issystem": false,
      "networkid": "ecd874e8-9c14-4a66-8528-c4a11960c7be",
      "physicalnetworkid": "08344756-9abc-41c0-b575-8ca1a0a7c77d",
      "project": "DomainAdminProject",
      "projectid": "03b2f678-f237-4804-a29d-88503a63817e",
      "state": "Allocated",
      "tags": [],
      "zoneid": "91b49be0-5a8b-4f3e-83b8-3bdfee4eba78",
      "zonename": "Sandbox-simulator"
    }
  ]
}

After the fix: (domain admin only sees ips from their own projects)

(qaprimate) 🐱 > list publicipaddresses  projectid=-1 listall=true 
{
  "count": 1,
  "publicipaddress": [
    {
      "allocated": "2020-02-25T09:39:16+0100",
      "associatednetworkid": "493824ac-12d5-43c6-b6d6-2246270cf0a0",
      "associatednetworkname": "testdomain-project-net",
      "domain": "Domain",
      "domainid": "d56ba667-945e-46be-8829-e21df75af531",
      "forvirtualnetwork": true,
      "id": "aea77e18-99b8-4a66-8cb5-15e6848134f6",
      "ipaddress": "192.168.2.121",
      "isportable": false,
      "issourcenat": true,
      "isstaticnat": false,
      "issystem": false,
      "networkid": "ecd874e8-9c14-4a66-8528-c4a11960c7be",
      "physicalnetworkid": "08344756-9abc-41c0-b575-8ca1a0a7c77d",
      "project": "DomainAdminProject",
      "projectid": "03b2f678-f237-4804-a29d-88503a63817e",
      "state": "Allocated",
      "tags": [],
      "zoneid": "91b49be0-5a8b-4f3e-83b8-3bdfee4eba78",
      "zonename": "Sandbox-simulator"
    }
  ]
}

... and the root admin is able to see everything:

(qaprimate) 🐱 > list publicipaddresses  projectid=-1 listall=true filter=id,ipaddress,domain
{
  "count": 18,
  "publicipaddress": [
    {
      "domain": "ROOT",
      "id": "b3e30c2a-02e9-44b6-b9b2-cd46531d00d7",
      "ipaddress": "192.168.2.99"
    },
    {
      "domain": "ROOT",
      "id": "c70bb8c8-7d41-4686-9740-9626dc216bc9",
      "ipaddress": "192.168.2.98"
    },
    {
      "domain": "ROOT",
      "id": "a5f9da6a-f4cc-44b1-8f5a-4456863d17fb",
      "ipaddress": "192.168.2.97"
    },
    {
      "domain": "ROOT",
      "id": "df296484-5f4c-4b73-a9b9-c14c2900217f",
      "ipaddress": "192.168.2.95"
    },
    {
      "domain": "ROOT",
      "id": "7f53ee95-1ce8-4b02-b4ad-2f7d3c4428d5",
      "ipaddress": "192.168.2.94"
    },
    {
      "domain": "ROOT",
      "id": "28b51886-963c-41d2-9507-3863da658136",
      "ipaddress": "192.168.2.92"
    },
    {
      "domain": "ROOT",
      "id": "2ee8f2bb-c0b1-4a2d-868a-8c28fe9ada66",
      "ipaddress": "192.168.2.4"
    },
    {
      "domain": "ROOT",
      "id": "a1dcfa06-6df4-4710-ad45-83b93e2e6327",
      "ipaddress": "192.168.2.3"
    },
    {
      "domain": "ROOT",
      "id": "26e2d0f7-476d-4598-a2c8-4b59d29aaf8e",
      "ipaddress": "192.168.2.195"
    },
    {
      "domain": "ROOT",
      "id": "3ada9c24-b138-4dd4-ac8a-2260b1865944",
      "ipaddress": "192.168.2.192"
    },
    {
      "domain": "ROOT",
      "id": "b17cc857-e6bd-4cbc-a09e-9f3e4f5c5433",
      "ipaddress": "192.168.2.183"
    },
    {
      "domain": "ROOT",
      "id": "242d048c-fec4-4d02-be32-14e9a29375f5",
      "ipaddress": "192.168.2.181"
    },
    {
      "domain": "ROOT",
      "id": "3574b63a-5dd9-4d1d-8c79-91de2952203d",
      "ipaddress": "192.168.2.168"
    },
    {
      "domain": "ROOT",
      "id": "bdf26431-c879-4750-847f-7303b905f718",
      "ipaddress": "192.168.2.162"
    },
    {
      "domain": "ROOT",
      "id": "8f8fd0cf-5707-4677-ad38-18e3ad8912d1",
      "ipaddress": "192.168.2.155"
    },
    {
      "domain": "ROOT",
      "id": "6330bf8c-d668-413b-9fca-b0ae0111d58d",
      "ipaddress": "192.168.2.127"
    },
    {
      "domain": "Domain",
      "id": "aea77e18-99b8-4a66-8cb5-15e6848134f6",
      "ipaddress": "192.168.2.121"
    },
    {
      "domain": "ROOT",
      "id": "90f7a57f-3dfd-4244-ba08-973bc847d2c0",
      "ipaddress": "192.168.2.103"
    }
  ]
}

blueorangutan · 2020-02-25T18:45:16Z

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

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	175.66	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	176.90	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	238.35	test_privategw_acl.py

DaanHoogland · 2020-02-25T21:55:22Z

@weizhouapache are you satisfied that we can merge this?

rohityadavcloud · 2020-02-26T01:34:17Z

@DaanHoogland @andrijapanic can one of you review/test the final changes?

weizhouapache · 2020-02-26T08:38:41Z

@DaanHoogland @rhtyd I am ok with merging it.
remember to fix the remaining issues

DaanHoogland · 2020-02-26T10:48:17Z

@andrijapanicsb, care to test?

rohityadavcloud · 2020-02-26T11:07:54Z

@DaanHoogland @andrijapanicsb I've tested apis mentioned in #3894 (comment) (see ticked)

I think this is ready for merging once it passes one of your reviews/testings.

andrijapanicsb · 2020-02-26T12:11:42Z

If Rohit tested the security things, I'm fine with it.

DaanHoogland · 2020-02-26T15:13:48Z

merging, assuming #3897 is there to remind us of the remaining issues @weizhouapache is asking for.

rohityadavcloud · 2020-02-26T15:23:57Z

Yes #3897 is to remind us to to system-wide survey/checks for all list APIs.

Pearl1594 requested a review from rohityadavcloud February 18, 2020 13:12

rohityadavcloud added this to the 4.13.1.0 milestone Feb 18, 2020

rohityadavcloud reviewed Feb 18, 2020

View reviewed changes

rohityadavcloud approved these changes Feb 18, 2020

View reviewed changes

weizhouapache reviewed Feb 18, 2020

View reviewed changes

rohityadavcloud reviewed Feb 18, 2020

View reviewed changes

rohityadavcloud requested review from DaanHoogland, andrijapanicsb and borisstoyanov February 18, 2020 18:13

rohityadavcloud mentioned this pull request Feb 18, 2020

[BUG] Wrong pagination on Storage > Volumes apache/cloudstack-primate#164

Closed

rohityadavcloud requested a review from weizhouapache February 19, 2020 07:46

apache deleted a comment from blueorangutan Feb 24, 2020

server: list project resources listing for non-root-admin accounts

e38b087

Signed-off-by: Rohit Yadav <[email protected]>

DaanHoogland merged commit 4d8a2da into apache:4.13 Feb 26, 2020

DaanHoogland deleted the list-api-pagination branch February 26, 2020 15:14

This was referenced Feb 26, 2020

With pagesize=10 the count returned in the API response is wrong #3890

Closed

findHostsForMigration API returns an empty/incomplete host list #3858

Closed

olivierlemasle mentioned this pull request Jul 1, 2020

Wrong count in listUsageRecords response #4191

Closed

api: Fix count and item issues returned by list APIs #3894

api: Fix count and item issues returned by list APIs #3894

Uh oh!

Conversation

Pearl1594 commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

rohityadavcloud Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

weizhouapache Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

rohityadavcloud Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

rohityadavcloud commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

rohityadavcloud commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

blueorangutan commented Feb 18, 2020

Uh oh!

rohityadavcloud commented Feb 19, 2020

Uh oh!

blueorangutan commented Feb 19, 2020

Uh oh!

weizhouapache commented Feb 19, 2020

Uh oh!

blueorangutan commented Feb 24, 2020

Uh oh!

weizhouapache commented Feb 24, 2020

Uh oh!

weizhouapache commented Feb 24, 2020

Uh oh!

rohityadavcloud commented Feb 24, 2020

Uh oh!

blueorangutan commented Feb 24, 2020

Uh oh!

rohityadavcloud commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohityadavcloud commented Feb 25, 2020

Uh oh!

Pearl1594 commented Feb 18, 2020 •

edited

Loading

rohityadavcloud commented Feb 18, 2020 •

edited

Loading

rohityadavcloud commented Feb 25, 2020 •

edited

Loading