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: CRUSH map viewer #24766

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
5 participants
@familyuu

familyuu commented Oct 26, 2018

mgr/dashboard: Add CRUSH map viewer
Fixes: http://tracker.ceph.com/issues/35684
Signed-off-by: familyuu guodan1@lenovo.com

@familyuu familyuu force-pushed the familyuu:crush branch 3 times, most recently from 02f53e9 to 5688dff Oct 26, 2018

@tspmelo

This comment has been minimized.

Contributor

tspmelo commented Oct 26, 2018

I would prefer not to include jquery.
Could try to use ng2-tree instead of the bootstrap-treeview?

@familyuu

This comment has been minimized.

familyuu commented Oct 28, 2018

@tspmelo Ok, I will try, and give the implementation as soon as possible.

@familyuu familyuu force-pushed the familyuu:crush branch 4 times, most recently from be93b8e to 4b8b48f Oct 29, 2018

@votdev votdev removed the needs-rebase label Oct 29, 2018

@votdev

This comment has been minimized.

Contributor

votdev commented Oct 29, 2018

Would be great to use the TableKeyValueComponent data table to display the values.

auswahl_001

@familyuu

This comment has been minimized.

familyuu commented Oct 29, 2018

@votdev Yeah, I think so, for the perfect display, I will implement it as soon as possible.

return { value: 'No nodes!' };
}
for (let i = nodesLength - 1; i > -1; i--) {

This comment has been minimized.

@Devp00l

Devp00l Oct 29, 2018

Contributor

How about using forEach instead?

Suggested change Beta
for (let i = nodesLength - 1; i > -1; i--) {
cephNodes.reverse().forEach((node) => {

This comment has been minimized.

@familyuu

familyuu Oct 30, 2018

Your suggestion is better, I have modified it.

Show resolved Hide resolved ...r/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.ts

@familyuu familyuu force-pushed the familyuu:crush branch 5 times, most recently from 24cc43b to 0fdf53e Oct 30, 2018

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Oct 30, 2018

Thanks for addressing the review feedback so far. Good progress! Displaying the metadata in table format looks much better, but I wonder if it should be displayed below the tree view, to be in line with how we display an object's detail on other pages?

I tested your latest changes:
peek 2018-10-30 18-10
Some comments:

  • Would it make sense to reverse the order in which the OSDs are sorted?
  • Not sure if I would display the crush weight in the tree view on the left

What's next? Do you have any other enhancements in mind, or is this good to go as the first implementation and we enhance this feature with follow-up pull requests?

@familyuu familyuu force-pushed the familyuu:crush branch from 0fdf53e to 5f05bf9 Oct 31, 2018

@familyuu

This comment has been minimized.

familyuu commented Oct 31, 2018

@LenzGr Thanks for your suggestion, and I have modified code in terms of your comments, including: changing the location to display the metadata, replacing the "crush weight" information with the "status" information of osds.
For the order of OSDS, however, I have to say it is a vice effect that came with the implementation of build the data structure of tree-view. As we known, we need build a special data object from the metadata returned from python controller, like this:
{ value: 'parent', children:[ { value: 'osd1' }, { value: 'osd2' }] }
For build an object like this structure, that mean, when we build a node which have children, all children node have been built already. So we need to traverse the "nodes" list from the end, and so, the order of OSDS is reverse. While, for now, I don't think this order will affect the tree view of the CRUSH map.
Finally, I think this can be the first implementation of CRUSH map, what do you think about it?

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 1, 2018

I would prefer to have the table inside the crush panel, otherwise it seems to separate things.

Good point, @tspmelo - I'm fine with this approach. Sorry for the back-and-forth about this, @familyuu :/

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 1, 2018

@LenzGr Thanks for your suggestion, and I have modified code in terms of your comments, including: changing the location to display the metadata, replacing the "crush weight" information with the "status" information of osds.

Thank you!

For the order of OSDS, however, I have to say it is a vice effect that came with the implementation of build the data structure of tree-view. As we known, we need build a special data object from the metadata returned from python controller, like this:
{ value: 'parent', children:[ { value: 'osd1' }, { value: 'osd2' }] }
For build an object like this structure, that mean, when we build a node which have children, all children node have been built already. So we need to traverse the "nodes" list from the end, and so, the order of OSDS is reverse. While, for now, I don't think this order will affect the tree view of the CRUSH map.

OK, thanks for the explanation. I'm fine with keeping the current order for now, it it's too involved to sort it.

Finally, I think this can be the first implementation of CRUSH map, what do you think about it?

I agree, this is a good start. Would you mind changing the layout as suggested by @tspmelo in #24766 (comment) and adding a few tests as requested? From a functionality POV, this would be a good first step (a "CRUSH map viewer") to get merged.

@familyuu familyuu force-pushed the familyuu:crush branch from 08e4c21 to 707abe2 Nov 2, 2018

@familyuu

This comment has been minimized.

familyuu commented Nov 2, 2018

@LenzGr @tspmelo @Devp00l Hi, I've modified the code as your suggested, include: add a test for create the correct tree, put the metadata table to the left of the tree view, and change the misleading function name in the code. Please review again. Thank you for your patience and advises.

@LenzGr LenzGr changed the title from mgr/dashboard: crush map to mgr/dashboard: CRUSH map viewer Nov 2, 2018

const children: any[] = [];
if (node.children) {
node.children.forEach((childId) => {

This comment has been minimized.

@Devp00l

Devp00l Nov 2, 2018

Contributor

How about sorting the children by ID, using node.children.sort() ?

Suggested change Beta
node.children.forEach((childId) => {
node.children.sort().forEach((childId) => {
};
const data = { osd_map };
spyOn(dashboardService, 'getHealth').and.returnValue(of(data));
fixture.detectChanges();

This comment has been minimized.

@Devp00l

Devp00l Nov 2, 2018

Contributor

Hi, I looked through your tests and refactored and extend them a bit, hopefully you will like my suggestion, to minimize the preparation per test.

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.spec.ts
index 3395120b72..704416c47e 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.spec.ts
@@ -16,6 +16,7 @@ describe('CrushmapComponent', () => {
   let component: CrushmapComponent;
   let fixture: ComponentFixture<CrushmapComponent>;
   let debugElement: DebugElement;
+
   configureTestBed({
     imports: [HttpClientTestingModule, TreeModule, TabsModule.forRoot(), SharedModule],
     declarations: [CrushmapComponent],
@@ -38,33 +39,52 @@ describe('CrushmapComponent', () => {
     expect(span.textContent).toContain(component.panelTitle);
   });
 
-  it('should display "No nodes!" if ceph tree nodes is empty array', () => {
-    const dashboardService = debugElement.injector.get(DashboardService);
-    const osd_map = { tree: {} };
-    const data = { osd_map };
-    const getHealthSpy = spyOn(dashboardService, 'getHealth');
-    getHealthSpy.and.returnValue(of(data));
-    fixture.detectChanges();
+  describe('tests tree', () => {
+    let dashboardService: DashboardService;
 
-    expect(getHealthSpy).toHaveBeenCalled();
-    expect(component.tree.value).toEqual('No nodes!');
-  });
-
-  it('should has correct tree data structure after transform the metadata', () => {
-    const dashboardService = debugElement.injector.get(DashboardService);
-    const osd_map = {
-      tree: {
-        nodes: [
-          { children: [-1], type: 'root', name: 'Root', id: 1 },
-          { children: [-2], type: 'host', name: 'Host', id: -1 },
-          { status: 'up', type: 'osd', name: 'Osd', id: -2 }
-        ]
-      }
+    const prepareGetHealth = (nodes: object[]) => {
+      spyOn(dashboardService, 'getHealth').and.returnValue(
+        of({ osd_map: { tree: { nodes: nodes } } })
+      );
+      fixture.detectChanges();
     };
-    const data = { osd_map };
-    spyOn(dashboardService, 'getHealth').and.returnValue(of(data));
-    fixture.detectChanges();
 
-    expect(component.tree.children[0].children[0].value).toBe('Osd (osd)--up');
+    beforeEach(() => {
+      dashboardService = debugElement.injector.get(DashboardService);
+    });
+
+    it('should display "No nodes!" if ceph tree nodes is empty array', () => {
+      prepareGetHealth([]);
+      expect(dashboardService.getHealth).toHaveBeenCalled();
+      expect(component.tree.value).toEqual('No nodes!');
+    });
+
+    describe('with data', () => {
+      beforeEach(() => {
+        prepareGetHealth([
+          { children: [-2], type: 'root', name: 'default', id: -1 },
+          { children: [1, 0, 2], type: 'host', name: 'my-host', id: -2 },
+          { status: 'up', type: 'osd', name: 'osd.0', id: 0 },
+          { status: 'down', type: 'osd', name: 'osd.1', id: 1 },
+          { status: 'up', type: 'osd', name: 'osd.2', id: 2 }
+        ]);
+      });
+
+      it('has a tree structure deriving from root', () => {
+        expect(component.tree.value).toBe('default (root)');
+      });
+
+      it('has a tree structure with one host child with 3 children', () => {
+        expect(component.tree.children.length).toBe(1);
+        expect(component.tree.children[0].value).toBe('my-host (host)');
+        expect(component.tree.children[0].children.length).toBe(3);
+      });
+
+      it('has sorted all host children by id', () => {
+        expect(component.tree.children[0].children[0].value).toBe('osd.0 (osd)--up');
+        expect(component.tree.children[0].children[1].value).toBe('osd.1 (osd)--down');
+        expect(component.tree.children[0].children[2].value).toBe('osd.2 (osd)--up');
+      });
+    });
   });
 });

This comment has been minimized.

@familyuu

familyuu Nov 3, 2018

Thanks for your suggestion. Actually, I am not good at write tests, so, just forgive my poor test cases, and I have modified the code as you suggested, thanks again for your advise.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 2, 2018

Thank you, looks good to me. Can you please amend the git commit message to match the title and description of this PR and to include a reference to the tracker issue?
screenshot from 2018-11-02 13-25-05
Some minor observations:

  • For consistency, I suggest to always print "CRUSH" in capital letters (as done in the documentation), e.g. in the breadcrumb text or heading above the tree view.
  • There seems to be an extra empty row on the bottom of the table?
@votdev

This comment has been minimized.

Contributor

votdev commented Nov 2, 2018

* There seems to be an extra empty row on the bottom of the table?

This is something we can not fix in the Swimlane ngx datatable. We have many tables (e.g. in the details tables) in the UI with the same behaviour.

@familyuu familyuu force-pushed the familyuu:crush branch from 707abe2 to f3157db Nov 3, 2018

@familyuu

This comment has been minimized.

familyuu commented Nov 3, 2018

Thank you, looks good to me. Can you please amend the git commit message to match the title and description of this PR and to include a reference to the tracker issue?

Ok, no problem.

@familyuu familyuu force-pushed the familyuu:crush branch 2 times, most recently from a305ec0 to 1ed9086 Nov 5, 2018

@Devp00l

Devp00l approved these changes Nov 5, 2018

LGTM :-)

let value: string = node.name + ' (' + node.type + ')';
if (node.status) {
value += '--' + node.status;

This comment has been minimized.

@votdev

votdev Nov 5, 2018

Contributor

Is this a special Ceph convention? What about ods.1 (osd) - up or ods.1 (osd) [up]? Using colors might be the best solution in my opinion.

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 5, 2018

Ok, no problem.

Sorry to be such a pain, @familyuu - but the git commit message is still not in the correct format :/

Can you please amend it so it also contains a reference to the tracker issue?

mgr/dashboard: Add CRUSH map viewer
Fixes: http://tracker.ceph.com/issues/35684
Signed-off-by: familyuu <guodan1@lenovo.com>
@votdev

This comment has been minimized.

Contributor

votdev commented Nov 5, 2018

What about this?
auswahl_002

--- src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.html	(date 1540782268000)
+++ src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.html	(date 1541420464000)
@@ -9,7 +9,14 @@
       <div class="panel-body">
         <div class="col-sm-6 col-lg-6">
           <tree [tree]="tree"
-                (nodeSelected)="onNodeSelected($event)"></tree>
+                (nodeSelected)="onNodeSelected($event)">
+            <ng-template let-node>
+              <span class="node-name" [innerHTML]="node.value"></span>
+              <span>&nbsp;</span>
+              <span class="label"
+                    [ngClass]="{'label-success': ['in', 'up'].includes(node.status), 'label-danger': ['down', 'out'].includes(node.status)}">{{ node.status }}</span>
+            </ng-template>
+          </tree>
         </div>
         <div class="col-sm-6 col-lg-6 metadata">
           <cd-table-key-value *ngIf="metadata"
@@ -19,4 +26,4 @@
       </div>
     </div>
   </div>

--- src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.ts	(date 1540782268000)
+++ src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/crushmap/crushmap.component.ts	(date 1541419606000)
@@ -50,9 +50,7 @@
     const settings = { static: true };
 
     let value: string = node.name + ' (' + node.type + ')';
-    if (node.status) {
-      value += '--' + node.status;
-    }
+    let status: string = node.status;
 
     const children: any[] = [];
     if (node.children) {
@@ -60,10 +58,10 @@
         children.push(treeNodeMap[childId]);
       });
 
-      return { value, settings, id, children };
+      return { value, status, settings, id, children };
     }
 
-    return { value, settings, id };
+    return { value, status, settings, id };
   }
 
   onNodeSelected(e: NodeEvent) {
@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 5, 2018

What about this?

Looks good to me @votdev - thanks for the suggestion. @familyuu are you fine with including this suggestion?

Please let's focus on getting the existing implementation reviewed and merged and refrain from making additional visual/functionality improvements in this PR; let's open new PRs for additional changes/enhancements instead. Thanks!

@tspmelo

tspmelo approved these changes Nov 5, 2018

@votdev votdev referenced this pull request Nov 5, 2018

Merged

mgr/dashboard: Improve CRUSH map viewer #24934

0 of 3 tasks complete
@votdev

This comment has been minimized.

Contributor

votdev commented Nov 5, 2018

What about this?

Looks good to me @votdev - thanks for the suggestion. @familyuu are you fine with including this suggestion?

Please let's focus on getting the existing implementation reviewed and merged and refrain from making additional visual/functionality improvements in this PR; let's open new PRs for additional changes/enhancements instead. Thanks!

To get the PR merged asap i've created a separate PR with my suggestions.

@familyuu familyuu force-pushed the familyuu:crush branch from 1ed9086 to a02da83 Nov 6, 2018

guodan1
mgr/dashboard: Add CRUSH map viewer
Fixes: http://tracker.ceph.com/issues/35684

Signed-off-by: familyuu <guodan1@lenovo.com>

@familyuu familyuu force-pushed the familyuu:crush branch from a02da83 to 53fa735 Nov 6, 2018

@familyuu

This comment has been minimized.

familyuu commented Nov 6, 2018

Ok, no problem.

Sorry to be such a pain, @familyuu - but the git commit message is still not in the correct format :/

Can you please amend it so it also contains a reference to the tracker issue?

mgr/dashboard: Add CRUSH map viewer
Fixes: http://tracker.ceph.com/issues/35684
Signed-off-by: familyuu <guodan1@lenovo.com>

Oh, it looks like I misunderstood some of the things you talked about, I modified the git commit message and it looks right now. I am sorry for my carelessness.

@familyuu

This comment has been minimized.

familyuu commented Nov 6, 2018

What about this?

Looks good to me @votdev - thanks for the suggestion. @familyuu are you fine with including this suggestion?
Please let's focus on getting the existing implementation reviewed and merged and refrain from making additional visual/functionality improvements in this PR; let's open new PRs for additional changes/enhancements instead. Thanks!

To get the PR merged asap i've created a separate PR with my suggestions.

Sounds like I don't need to add code about this topic to my PR. Thank you very much for your advice that make me more capable of developing better apps. I think I have learned a lot of skills in this progress.

@votdev

votdev approved these changes Nov 6, 2018

@LenzGr LenzGr removed the needs-review label Nov 6, 2018

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Nov 6, 2018

I am sorry for my carelessness.

Nothing to be sorry about! Thanks a lot for addressing all of the suggestions and for your contribution! Much appreciated.

@LenzGr LenzGr merged commit 100e15f into ceph:master Nov 6, 2018

5 of 6 checks passed

ceph dashboard tests ceph dashboard tests failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment