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

mgr/dashboard: orchestrator integration initial works #29127

Open
wants to merge 13 commits into
base: master
from

Conversation

@bk201
Copy link
Contributor

commented Jul 19, 2019

  • Display hosts, inventory, and services from orchestrator
  • Allow adding/removing hosts

Fixes: https://tracker.ceph.com/issues/40337
Fixes: https://tracker.ceph.com/issues/40336
Fixes: https://tracker.ceph.com/issues/38233

Signed-off-by: Kiefer Chang kiefer.chang@suse.com

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/host.py
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/host.py
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/orchestrator.py Outdated
def handle_orchestrator_error(component):
try:
yield
except RuntimeError as e:

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Jul 19, 2019

Member

See http://docs.ceph.com/docs/master/mgr/orchestrator_modules/#error-handling

Everything that is not an OrchestratorError should be an internal server error.

Every exception that is raised that should be handled gracefully should be converted to an OrchestratorError in the orchestrator modules.

This comment has been minimized.

Copy link
@bk201

bk201 Aug 6, 2019

Author Contributor

Looks like an exception on remote is always converted to RuntimeError.

:raises RuntimeError: **Any** error raised within the method is converted to a RuntimeError

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Aug 6, 2019

Member

Yeah, I wrote that one. Within _Completions, exceptions are carefully transferred into the current subinterpreter and then re-raised.

This comment has been minimized.

Copy link
@bk201

bk201 Aug 6, 2019

Author Contributor

Thanks, I saw that. But it's weird I always catch a RuntimeError on an unimplemented method. I need to investigate further.

Show resolved Hide resolved src/pybind/mgr/dashboard/services/orchestrator.py Outdated
@epuertat
Copy link
Contributor

left a comment

It's great to see the orchestrator getting closer to the dashboard. Thanks a lot for your work, @bk201! I left some comments over there. Don't hesitate to ping me if you need further clarification.

return len(mgr.list_servers())
all_hosts = mgr.list_servers()
if self._has_permissions(Permission.READ, Scope.ORCHESTRATOR):
all_hosts = merge_with_orchestrator(all_hosts)

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 24, 2019

Contributor

While I think it's risky to return different representations for the same resource based on the authentication context, there's no RESTful guideline against that (however, we should immediately start to explicitly set caching policies for these resources; otherwise, an unprivileged user might end up receiving the same response as an user with higher permissions).

However, this goes beyond a question of more or less rich representations of the underlying resource, but implies changing semantics on the fly (the property host_count of this resource means different things depending on who's viewing that resource).

Instead, I'd suggest adding a new property for encompassing inventory hosts.

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Jul 24, 2019

Member

👍 I think this is a problem of too fine grained permissions.

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 31, 2019

Contributor

Yes, I agree. I think with the current number of scopes (or even a few less ones) we are ok.

return len(mgr.list_servers())
all_hosts = mgr.list_servers()
if self._has_permissions(Permission.READ, Scope.ORCHESTRATOR):
all_hosts = merge_with_orchestrator(all_hosts)

This comment has been minimized.

Copy link
@epuertat

epuertat Jul 24, 2019

Contributor

If we are sharing logic between controllers (merge_with_orchestrator()), that probably means:

  • The corresponding REST resource is a hoarding one (it does not map to a real resource, but it's a mere collection of unrelated metadata). health endpoint clearly fits into this category (and we should remove it sooner than later).
  • We need to move that logic to a service, so it can be reused. Controllers should contain minimal logic ("thin controllers, fat models" is the saying).
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/health.py Outdated
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/host.py Outdated
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/host.py Outdated
Show resolved Hide resolved src/pybind/mgr/dashboard/services/orchestrator.py
Show resolved Hide resolved src/pybind/mgr/dashboard/services/orchestrator.py
@bk201

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Why do we need a persistent shim to the orchestrator?

Switch to a class getter for single instance. (1b49673)

@epuertat
Copy link
Contributor

left a comment

Thanks for addressing most of my comments. Regarding the new scopes (Inventory/Services), I'm still dubious about them: I think we shouldn't add more scopes whenever possible, but trying to stick new functionality to existing scopes. The originally proposed Orchestrator Scope was Ok to cover Orchestrator management workflow. For Inventory I think we may reuse Host Scope.

@rjfd

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@bk201 I agree with @epuertat inventory operations should use the hosts scope, and services should use the scope that is related to.

@bk201

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@bk201 I agree with @epuertat inventory operations should use the hosts scope, and services should use the scope that is related to.

Thank @epuertat and @rjfd for comments.

Inventory is attached to hosts, so it's sane to leverage HOSTS scope.

As for services, it's nice to use scopes related to them too.
According to current orchestrator implementations, I summarize service types in orchestrator and existing Dashboard scopes might be used in the following table:

Service in Orchestrator Scope in Dashboard
mgr MANAGER
mon MONITOR
iscsi ISCSI
mds CEPHFS
nfs NFS_GANESHA
rbd-mirror RBD_MIRRORING
rgw RGW

Operations for these services might be implemented in pages related to services rather than main services page. For example, if a user wants to add a mon service, we can add this feature in Monitors page (a button on monitors table).

Another question: what to do when listing services? Return only those services that users have READ permission on? Or always allow read and apply permission checks when users want to perform create/delete/update operation on services?

@epuertat

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Another question: what to do when listing services? Return only those services that users have READ permission on? Or always allow read and apply permission checks when users want to perform create/delete/update operation on services?

Looks great, thanks!

On the listing services, where is that going to be displayed? If if's in come Orchestrator page, let's stick to Orch Scope; if it's in each service page (RGW, CephFS, etc), let's use each service scope. Nevertheless, it might happen that there's no easy way to handle that with the current scopes (for example, Options has a separate scope, while ideally users should only have access to the Options related to their enabled Scopes; unfortunately that's not easy to do as not all options affect single services).

@bk201

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

On the listing services, where is that going to be displayed?

Services of a host will be displayed on Service tab of host and a new page for all services.
Please see the links, thanks!

@epuertat

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

On the listing services, where is that going to be displayed?

Services of a host will be displayed on Service tab of host and a new page for all services.
Please see the links, thanks!

I think all users with Read perm to Hosts page (until now no other perm than Read made sense) should see all services available. However if the Hosts page is going to be used for deploying/scaling/decomissioning service nodes on a per-host basis, then we should enable them based on the corresponding services (RGW, RBD, etc).

Nonetheless, I'm thinking how to improve this, as this might end up being quite complicate to handle (with components that fully served to a specific purpose that was easy, but with complex workflows involving multiple back-end entities it's becoming more intricate).

The current approach is:

<div *ngIf="permissions.ComponentA.read || permissions.ComponentA.write || permissions.ComponentB.read || ...">
  <!-- UI stuff showing outputs from SomeServiceA.method1() and SomeServiceB.method2() >
</div>

The above may often lead to complicate condition checking, and developers having to know before-hand and code scattered authorization statements in the Views. As UI is basically consuming and invoking Service methods, we may just leave Service methods tell us whether our current permissions are enough to go on with them.

So the alterntive approach would be:

<div *ngIf="SomeServiceA.method1.allowed && SomeServiceB.method2.allowed">
  <!-- UI stuff showing outputs from SomeServiceA.method1() and SomeServiceB.method2() >
</div>
// some-service-a.service.ts
@Scope(Scope.SOME_BACKEND_SCOPE)
@Injectable({                                                                                        
  providedIn: WhateverModule                                                                              
})                                                                                                   
export class SomeServiceA {                                                                      
  
  @requires_permissions(Permissions.READ)
  method1(...) {

  }

For back-end driven services this could also be offloaded to the back-end rest api endpoints, which already know their scope and permissions. By implementing HTTP OPTIONS call:

# A user with RGW read only permissions issues an `OPTIONS /api/rgw/buckets` 
HTTP/1.1 204 No Content
Allow: OPTIONS, GET, HEAD
...
X-Dashboard-Scope: RGW

And, in the front-end:

// smart-api.service.ts
export abstract class SmartAPIService {
  private url:string;
                                                                      
  constructor(private http: HttpClient) {
    response = this.options(); // if Forbidden, all CRUD methods are disabled
    this.scope = response.headers.get('X-Dashboard-Scope');
    this.allow = response.headers.get('Allow');
    // Apply permissions metadata to CRUD get/post/put/delete/patch methods
  }
}

// api/rgw-bucket.service.ts
@Injectable({                                                                                        
  providedIn: ApiModule                                                                              
})                                                                                                   
export class RgwBucketService extends SmartAPIService {                                                                      
  private url = 'api/rgw/bucket';
  // Inheritance would implement scope & permission from child url member
  }

@bk201 bk201 force-pushed the bk201:wip-40337 branch from e614a7d to ff2adbb Aug 15, 2019

@bk201 bk201 marked this pull request as ready for review Aug 15, 2019

@bk201 bk201 requested a review from ceph/dashboard as a code owner Aug 15, 2019

@LenzGr LenzGr added the needs-review label Aug 16, 2019

private setComplexValidators() {
this.hostForm.get('hostname').setValidators([
this.hostForm.get('hostname').validator,
CdValidators.custom('uniqueName', (hostname: string) => {

This comment has been minimized.

Copy link
@tspmelo

tspmelo Aug 16, 2019

Contributor

Why not add this directly during createForm?

This comment has been minimized.

Copy link
@bk201

bk201 Aug 19, 2019

Author Contributor

Thanks, fixed in 9d9a79c

<a href="{{ docsUrl }}"
target="_blank">documentation</a> on how to
configure and enable the orchestrator functionality.
</cd-info-panel>

This comment has been minimized.

Copy link
@tspmelo

tspmelo Aug 16, 2019

Contributor

This should be in the end of the previous line.

This comment has been minimized.

Copy link
@bk201

bk201 Aug 19, 2019

Author Contributor

Fixed in 9d9a79c

<a href="{{ docsUrl }}"
target="_blank">documentation</a> on how to
configure and enable the orchestrator functionality.
</cd-info-panel>

This comment has been minimized.

Copy link
@tspmelo

tspmelo Aug 16, 2019

Contributor

This should be in the end of previous line.

This comment has been minimized.

Copy link
@bk201

bk201 Aug 19, 2019

Author Contributor

Fixed in 9d9a79c

@@ -92,13 +92,27 @@
class="dropdown-item"
routerLink="/hosts">Hosts</a>
</li>
<li routerLinkActive="active"
class="tc_submenuitem tc_submenuitem_cluster_inventory"
*ngIf="permissions.hosts.read">

This comment has been minimized.

Copy link
@tspmelo

tspmelo Aug 16, 2019

Contributor

Not sure if we should hide it if the orchestrator is not available.
For example for the Prometheus configuration we hide the menus.

This comment has been minimized.

Copy link
@bk201

bk201 Aug 19, 2019

Author Contributor

These two menu items are hidden when the user has no HOSTS.read permission. If orchestrator is unavailable, the information about how to enable it is displayed.

bk201 added some commits Jul 19, 2019

mgr/dashboard: orchestrator integration initial works
- Display hosts, inventory, and services from orchestrator
- Allow adding/removing hosts

Fixes: https://tracker.ceph.com/issues/40337
Fixes: https://tracker.ceph.com/issues/40336
Fixes: https://tracker.ceph.com/issues/38233

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Report orchestrator status description and refine wait_api_result dec…
…orator

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Update security scopes
- Remove ORCHESTRATOR scope, a user with HOST permission can view hosts
in orchestrator
- Create INVENTORY and SERVICE scope for inventory/service resources in
orchestrator

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Client code uses OrchClient with a module level getter
Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Add sources filter for hosts
User can pass `sources` query parameter when querying `api/host`
- `ceph`: for hosts already running Ceph services
- `orchestrator`: for hosts in Orchestrator

Without this parameter, both ceph and orchestrator hosts are returned

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Remove module getter for Orchestrator client
Use a classmethod instead

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Update Scopes
Remove INVENTORY and SERVICE scope.
- inventory access is bound to HOSTS
- services access is bound to HOSTS for read. For other operations like
create, update, delete, scope related to services should be used.

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Add backend tests for orchestrator endpoints
Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

bk201 added some commits Aug 12, 2019

Add Teuthology tests
- `/api/host`: list hosts (with orchestrator hosts)
- `/api/orchestrator/inventory`: list inventory
- `/api/orchestrator/services`: list services

Depends on #29595

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Add frontend unit tests
- Add unit tests for hosts, inventory, and services
- refine permission of host-details component

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
fix `@ViewChild` parameter change
Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
move inventory and service component from orchestrator to cluster
Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>
Addressing change requests
- Refine </cd-info-panel> close tag position
- Add hostname validator when creating host-from

Signed-off-by: Kiefer Chang <kiefer.chang@suse.com>

@bk201 bk201 force-pushed the bk201:wip-40337 branch from ff2adbb to 9d9a79c Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.