include all VMs when projects selected not just for admins by DaanHoogland · Pull Request #7667 · apache/cloudstack

DaanHoogland · 2023-06-21T12:12:43Z

Description

This PR makes so that when project=-1 and listall=true, listVMsMetricsCmd returns all VMs for the user, the same as for admin.

Fixes: #7642

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

DaanHoogland · 2023-06-21T12:13:03Z

@blueorangutan package

blueorangutan · 2023-06-21T12:14:03Z

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

codecov · 2023-06-21T13:09:33Z

Codecov Report

Merging #7667 (9a804ee) into 4.18 (a2eb103) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##               4.18    #7667   +/-   ##
=========================================
  Coverage     13.02%   13.02%           
- Complexity     9030     9031    +1     
=========================================
  Files          2720     2720           
  Lines        257001   257001           
  Branches      40083    40083           
=========================================
+ Hits          33463    33465    +2     
+ Misses       219337   219334    -3     
- Partials       4201     4202    +1

Files Changed	Coverage Δ
...ain/java/com/cloud/api/query/QueryManagerImpl.java	`2.99% <0.00%> (ø)`
...c/main/java/com/cloud/user/AccountManagerImpl.java	`20.40% <0.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

blueorangutan · 2023-06-21T13:17:16Z

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

DaanHoogland · 2023-06-21T13:35:52Z

@blueorangutan test keepEnv

blueorangutan · 2023-06-21T13:38:05Z

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

blueorangutan · 2023-06-22T03:20:21Z

[SF] Trillian test result (tid-6798)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47781 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7667-t6798-kvm-centos7.zip
Smoke tests completed. 107 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.89	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	60.55	test_vm_life_cycle.py

yadvr · 2023-06-27T09:18:50Z

                        }
                    }
+                    if (listAll) {
+                        domainIdRecursiveListProject.third(ListProjectResourcesCriteria.ListAllIncludingProjectResources);


LGTM, should we worry if there could be negative consequences? (I'm asking to understand why this case was previously suported for admin only).

yadvr

Code LGTM, the listAll check should certainly work for all users but I'm not sure about any side-effects. Any manual QA/review would be great (as different user types/roles - both account and project roles).

kiranchavala · 2023-07-20T06:37:45Z

@blueorangutan package

blueorangutan · 2023-07-20T06:50:05Z

@kiranchavala 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 · 2023-07-20T07:54:35Z

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

kiranchavala

@DaanHoogland

During Manual testing, I was unable to view the VMs and from my project's vm

Steps followed

1. Create a normal user role account under root domain
2. Login with the normal user account
3. Create a project
4. Change the view to project
5. Deploy a vm

6. Change the view back to normal
7. Deploy a vm

unable to view the 2 vm in normal view

DaanHoogland · 2023-07-21T11:51:06Z

@DaanHoogland

During Manual testing, I was unable to view the VMs and from my project's vm

Steps followed
1. Create a normal user role account under root domain
2. Login with the normal user account
3. Create a project
4. Change the view to project
5. Deploy a vm

6. Change the view back to normal
7. Deploy a vm 
unable to view the 2 vm in normal view

I see what you mean @kiranchavala . this fix is API only (so far) I will look ate the UI as well.

weizhouapache

code lgtm

DaanHoogland · 2023-07-31T06:49:35Z

@blueorangutan package

blueorangutan · 2023-07-31T06:50:03Z

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

DaanHoogland · 2023-07-31T06:55:57Z

@kiranchavala , I tested this with the API. It should work as expected now. Sorry for the extra work.
ua : user/account
up : user/project
dp : domain/project
da : not visible ;)

(luser) 🐱 > list virtualmachines listall=true projectid=-1 filter=name
{
  "count": 3,
  "virtualmachine": [
    {
      "name": "up"
    },
    {
      "name": "ua"
    },
    {
      "name": "dp"
    }
  ]
}

blueorangutan · 2023-07-31T07:52:36Z

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

DaanHoogland · 2023-07-31T09:13:36Z

@blueorangutan test

blueorangutan · 2023-07-31T09:14:03Z

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

kiranchavala · 2023-08-01T08:38:38Z

@kiranchavala , I tested this with the API. It should work as expected now. Sorry for the extra work. ua : user/account up : user/project dp : domain/project da : not visible ;)
(luser) 🐱 > list virtualmachines listall=true projectid=-1 filter=name
{
  "count": 3,
  "virtualmachine": [
    {
      "name": "up"
    },
    {
      "name": "ua"
    },
    {
      "name": "dp"
    }
  ]
}

@DaanHoogland is the fix only for API or is UI also included ?

DaanHoogland · 2023-08-01T09:24:02Z

@DaanHoogland is the fix only for API or is UI also included ?

The issue is API only, I checked the UI and it works without changes

kiranchavala

@DaanHoogland

The api calls are working fine but the in the UI (default view) , I am not able to view the vm's which belonging to a project view

list virtualmachines listall=true projectid=-1 filter=name
list virtualmachinesmetrics listall=true projectid=-1 filter=name

DaanHoogland · 2023-08-01T15:57:33Z

The api calls are working fine but the in the UI (default view) , I am not able to view the vm's which belonging to a project view

I just tested again and it works. Can you expand on your test procedure?

kiranchavala · 2023-08-02T05:52:18Z

@DaanHoogland

Please find the steps that i followed

Create a normal user-role account
Login as the user account
Create a project
Change the view to project
Launch a vm

Change the view back to the default view
I am not able to view the vm which was launched from the project in the default view

kiranchavala

LGTM, Tested manually

Able to view the project view instances instances in the default view

kiranchavala · 2023-08-02T07:45:23Z

@DaanHoogland

Please find the steps that i followed

1. Create a normal user-role account

2. Login as the user account

3. Create a project

4. Change the view to project

5. Launch a vm

6. Change the view back to the default view

7. I am not able to view the vm which was launched from the project in the default view

Its working fine forgot to toggle the projects

DaanHoogland added the component:api label Jun 21, 2023

DaanHoogland added this to the 4.18.1.0 milestone Jun 21, 2023

DaanHoogland changed the title ~~include all files when project selected not just for admins~~ include all VMs when projects selected not just for admins Jun 21, 2023

DaanHoogland linked an issue Jun 22, 2023 that may be closed by this pull request

UI/API: Not showing all my VMs as normal users when projects is selected #7642

Closed

weizhouapache added the Severity:Major label Jun 26, 2023

yadvr added the needs-testing label Jun 27, 2023

yadvr reviewed Jun 27, 2023

View reviewed changes

yadvr approved these changes Jun 27, 2023

View reviewed changes

DaanHoogland marked this pull request as ready for review July 3, 2023 09:16

yadvr assigned kiranchavala, vladimirpetrov and borisstoyanov Jul 7, 2023

kiranchavala reviewed Jul 21, 2023

View reviewed changes

weizhouapache approved these changes Jul 26, 2023

View reviewed changes

DaanHoogland marked this pull request as draft July 27, 2023 09:05

DaanHoogland force-pushed the projectAndNormalVMs branch from 8841145 to c8e2f3d Compare July 27, 2023 09:11

DaanHoogland added 2 commits July 31, 2023 08:49

include all files when project selected not just for admins

11eac09

add the caller account id when seraching for all and projects

9a804ee

DaanHoogland force-pushed the projectAndNormalVMs branch from 2becf79 to 9a804ee Compare July 31, 2023 06:49

DaanHoogland marked this pull request as ready for review July 31, 2023 06:50

kiranchavala reviewed Aug 1, 2023

View reviewed changes

kiranchavala approved these changes Aug 2, 2023

View reviewed changes

DaanHoogland added status:ready-for-merge and removed needs-testing labels Aug 2, 2023

DaanHoogland merged commit b4032d9 into apache:4.18 Aug 3, 2023

DaanHoogland deleted the projectAndNormalVMs branch August 3, 2023 14:06

DaanHoogland mentioned this pull request Aug 3, 2023

UI/API: Not showing all my VMs as normal users when projects is selected #7642

Closed

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Conversation

DaanHoogland commented Jun 21, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

DaanHoogland commented Jun 21, 2023

Uh oh!

blueorangutan commented Jun 21, 2023

Uh oh!

codecov Bot commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jun 21, 2023

Uh oh!

DaanHoogland commented Jun 21, 2023

Uh oh!

blueorangutan commented Jun 21, 2023

Uh oh!

blueorangutan commented Jun 22, 2023

Uh oh!

yadvr Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala commented Jul 20, 2023

Uh oh!

blueorangutan commented Jul 20, 2023

Uh oh!

blueorangutan commented Jul 20, 2023

Uh oh!

kiranchavala left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 21, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 31, 2023

Uh oh!

blueorangutan commented Jul 31, 2023

Uh oh!

DaanHoogland commented Jul 31, 2023

Uh oh!

blueorangutan commented Jul 31, 2023

Uh oh!

DaanHoogland commented Jul 31, 2023

Uh oh!

blueorangutan commented Jul 31, 2023

Uh oh!

kiranchavala commented Aug 1, 2023

Uh oh!

DaanHoogland commented Aug 1, 2023

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Aug 1, 2023

Uh oh!

kiranchavala commented Aug 2, 2023

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala commented Aug 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov Bot commented Jun 21, 2023 •

edited

Loading

kiranchavala left a comment •

edited

Loading