[UI] Display system partitions in node page #3045

ChengYanJin · 2021-01-15T16:20:28Z

Component: ui, node

Context: We would like to add a tab in the Node page to display the information of system partitions.

Summary:

Update:
Will address hiding the warning alert in the next PR.

Acceptance criteria:

Closes: #2932
Closes: #2173

bert-e · 2021-01-15T16:20:29Z

Hello chengyanjin,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2021-01-15T16:20:34Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

JBWatenbergScality

Hello,
Good job for introducing react query and enhanced typing !
I've added a few comment about typing which can be a bit improved, also we have to check if we can merge #3002 before this PR in order to benefit from eea1b97 and useTypedSelector.
I think we may have to review a bit the style implementation because it seems not to be adapted for other screen resolution, eg on my screen :

The tbody height goes beyond my screen height and trigger the visibility of the scrollbar despite that there is no scroll necessity. I think that we should not define the tbody height and instead let the array fills the necessary place according to the number of partitions. Doing so also simplify reuse of the table component as it won't impact the style of the page where it is included.

ui/src/services/NodeVolumesUtils.js

ui/src/components/NodePagePartitionTab.js

ui/src/components/NodePartitionTable.js

ui/src/services/prometheus/fetchMetrics.js

bert-e · 2021-01-18T12:01:57Z

Branches have diverged

This pull request's source branch improvement/2932-display-system-partitions-in-node-page has diverged from
development/2.8 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

Merge origin/development/2.8 into improvement/2932-display-system-partitions-in-node-page
Rebase improvement/2932-display-system-partitions-in-node-page onto origin/development/2.8

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

bert-e · 2021-01-18T12:07:49Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

bert-e · 2021-01-18T19:12:56Z

Conflict

There is a conflict between your branch improvement/2932-display-system-partitions-in-node-page and the
destination branch development/2.8.

Please resolve the conflict on the feature branch (improvement/2932-display-system-partitions-in-node-page).

 $ git fetch
 $ git checkout origin/improvement/2932-display-system-partitions-in-node-page
 $ git merge origin/development/2.8
 $ # <intense conflict resolution>
 $ git commit
 $ git push origin HEAD:improvement/2932-display-system-partitions-in-node-page

ui/src/services/prometheus/fetchMetrics.js

bert-e · 2021-01-20T13:24:55Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

ui/src/components/NodePartitionTable.js

JBWatenbergScality

The change-log entry referring this change seems to be missing

ui/src/components/NodePartitionTable.js

JBWatenbergScality · 2021-01-22T08:58:44Z

ui/src/services/NodeVolumesUtils.js

+  const len = alerts.length;
+  // Record the index to be removed.
+  const removeIndex = [];
+  for (let i = 0, j = 1; i <= len - 2 && j <= len - 1; ) {
+    if (j === len - 1) {
+      i++;
+      j = i + 1;
+    } else if (
+      // Add more label conditions here when we need to address more alerts.
+      (alerts[i].labels.device &&
+        alerts[i].labels.alertname === alerts[j].labels.alertname &&
+        alerts[i].labels.device === alerts[j].labels.device &&
+        alerts[i].labels.instance === alerts[j].labels.instance) ||
+      (alerts[i].labels.persistentvolumeclaim &&
+        ((alerts[i].labels.persistentvolumeclaim ===
+          alerts[j].labels.persistentvolumeclaim) ===
+          alerts[i].labels.alertname) ===
+          alerts[j].labels.alertname)
+    ) {
+      if (alerts[i].labels.severity === 'warning') {
+        removeIndex.push(i);
+        i++;
+        j = i + 1;
+      } else if (alerts[j].labels.severity === 'warning') {
+        removeIndex.push(j);
+        i++;
+        j = i + 1;
+      }
+    } else {
+      j = j + 1;
+    }
+  }
+
+  let removedWarningAlerts = alerts;
+  removeIndex.forEach((index) => removedWarningAlerts.splice(index, 1));
+
+  return removedWarningAlerts;


In order to make it easier to take new alert rules into account I think it would be preferable to split the function similarly to this approach below :

function getAlertUniqueKey(alert: PrometheusAlert): string | null { if(alert.labels.alertname.startsWith("NodeFilesystem")) { return alert.labels.alertname + alert. labels.device + alert.labels.instance; } if(alert.labels.alertname.startsWith("PersistentVolume")) { return alert.labels.alertname + alert.persistentVolumeClaim; } return null; } export const removeWarningAlerts = ( alerts: Array<PrometheusAlert>, ): Array<PrometheusAlert> => { const unfilteredAlerts: PrometheusAlert[] = []; const indexedAlerts: {[key: string]: PrometheusAlert} = {}; alerts.forEach(alert => { const alertUniqueKey = getAlertUniqueKey(alert); if (!alertUniqueKey){ unfilteredAlerts.push(alert); } else if (alertUniqueKey && ((indexedAlerts[alertUniqueKey] && indexedAlerts[alertUniqueKey].labels.severity === 'warning') || !indexedAlerts[alertUniqueKey])) { indexedAlerts[alertUniqueKey] = alert; } }) return [...unfilteredAlerts, ...Object.values(indexedAlerts)]; }

JBWatenbergScality · 2021-01-22T08:59:43Z

ui/src/services/NodeVolumesUtils.test.js

+  },
+];
+
+it('should remove the wanring alerts', () => {


small typo here warning

JBWatenbergScality · 2021-01-22T09:04:47Z

ui/src/services/NodeVolumesUtils.test.js

+
+it('should remove the wanring alerts', () => {
+  const result = removeDiskWarningAlerts(alerts);
+  expect(result.length).toEqual(3);


You may want to test if result array is the expecting one using

expect(result).toStrictEqual([ { annotations: { description: 'Filesystem on /dev/vdc2 at 192.168.1.6:9100 has only 0.00% available space left.', runbook_url: 'https://github.com/kubernetes-monitoring/kubernetes-mixin/tree/master/runbook.md#alert-name-nodefilesystemalmostoutofspace', summary: 'Filesystem has less than 3% space left.', }, endsAt: '2021-01-21T09:32:35.358Z', fingerprint: '5ef4c93e954ac2c9', receivers: [ { name: 'null', }, ], startsAt: '2021-01-18T16:43:35.358Z', status: { inhibitedBy: [], silencedBy: [], state: 'active', }, updatedAt: '2021-01-21T09:28:40.099Z', generatorURL: 'http://prometheus-operator-prometheus.metalk8s-monitoring:9090/graph?g0.expr=%28node_filesystem_avail_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2F+node_filesystem_size_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2A+100+%3C+3+and+node_filesystem_readonly%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%3D%3D+0%29&g0.tab=1', labels: { alertname: 'NodeFilesystemAlmostOutOfSpace', container: 'node-exporter', device: '/dev/vdc2', endpoint: 'metrics', fstype: 'ext4', instance: '192.168.1.6:9100', job: 'node-exporter', mountpoint: '/mnt/testpart2', namespace: 'metalk8s-monitoring', pod: 'prometheus-operator-prometheus-node-exporter-m9qcj', prometheus: 'metalk8s-monitoring/prometheus-operator-prometheus', service: 'prometheus-operator-prometheus-node-exporter', severity: 'critical', }, }, { annotations: { description: 'Filesystem on /dev/vdc2 at 192.168.1.6:9100 has only 0.00% available space left.', runbook_url: 'https://github.com/kubernetes-monitoring/kubernetes-mixin/tree/master/runbook.md#alert-name-nodefilesystemalmostoutofspace', summary: 'Filesystem has less than 5% space left.', }, endsAt: '2021-01-21T09:32:35.358Z', fingerprint: '6331188d982cdcfc', receivers: [ { name: 'null', }, ], startsAt: '2021-01-18T16:43:35.358Z', status: { inhibitedBy: [], silencedBy: [], state: 'active', }, updatedAt: '2021-01-21T09:28:40.092Z', generatorURL: 'http://prometheus-operator-prometheus.metalk8s-monitoring:9090/graph?g0.expr=%28node_filesystem_avail_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2F+node_filesystem_size_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2A+100+%3C+5+and+node_filesystem_readonly%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%3D%3D+0%29&g0.tab=1', labels: { alertname: 'NodeFilesystemAlmostOutOfSpace', container: 'node-exporter', device: '/dev/vdc2', endpoint: 'metrics', fstype: 'ext4', instance: '192.168.1.6:9100', job: 'node-exporter', mountpoint: '/mnt/testpart2', namespace: 'metalk8s-monitoring', pod: 'prometheus-operator-prometheus-node-exporter-m9qcj', prometheus: 'metalk8s-monitoring/prometheus-operator-prometheus', service: 'prometheus-operator-prometheus-node-exporter', severity: 'warning', }, }, { annotations: { description: 'Filesystem on /dev/vdc1 at 192.168.1.6:9100 has only 3.28% available space left.', runbook_url: 'https://github.com/kubernetes-monitoring/kubernetes-mixin/tree/master/runbook.md#alert-name-nodefilesystemalmostoutofspace', summary: 'Filesystem has less than 5% space left.', }, endsAt: '2021-01-21T09:32:35.358Z', fingerprint: '6c9be09b96993ce4', receivers: [ { name: 'null', }, ], startsAt: '2021-01-18T16:43:35.358Z', status: { inhibitedBy: [], silencedBy: [], state: 'active', }, updatedAt: '2021-01-21T09:28:40.092Z', generatorURL: 'http://prometheus-operator-prometheus.metalk8s-monitoring:9090/graph?g0.expr=%28node_filesystem_avail_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2F+node_filesystem_size_bytes%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%2A+100+%3C+5+and+node_filesystem_readonly%7Bfstype%21%3D%22%22%2Cjob%3D%22node-exporter%22%7D+%3D%3D+0%29&g0.tab=1', labels: { alertname: 'NodeFilesystemAlmostOutOfSpace', container: 'node-exporter', device: '/dev/vdc1', endpoint: 'metrics', fstype: 'xfs', instance: '192.168.1.6:9100', job: 'node-exporter', mountpoint: '/mnt/testpart1', namespace: 'metalk8s-monitoring', pod: 'prometheus-operator-prometheus-node-exporter-m9qcj', prometheus: 'metalk8s-monitoring/prometheus-operator-prometheus', service: 'prometheus-operator-prometheus-node-exporter', severity: 'warning', }, }, ])

additionally your test seems to lack coverage on persistentvolume alerts and alerts of other kinds that should not be filtered

ui/src/components/NodePartitionTable.js

bert-e · 2021-01-22T09:24:44Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@JBWatenbergScality

bert-e · 2021-01-22T09:38:02Z

Conflict

There is a conflict between your branch improvement/2932-display-system-partitions-in-node-page and the
destination branch development/2.8.

Please resolve the conflict on the feature branch (improvement/2932-display-system-partitions-in-node-page).

 $ git fetch
 $ git checkout origin/improvement/2932-display-system-partitions-in-node-page
 $ git merge origin/development/2.8
 $ # <intense conflict resolution>
 $ git commit
 $ git push origin HEAD:improvement/2932-display-system-partitions-in-node-page

bert-e · 2021-01-22T09:44:18Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@JBWatenbergScality

Refs: #2932

TODO: We should throw error immediately rather than catch the error and return This may change the behavior of the error handling of the APIs. Refs: #2932

Refs: #2932

To make sure the functions beforeAll, afterEach, afterAll function in the correct place. Refs: #2932

Refs: #2932

To make the data a little more accurate Refs: #2932

When installing kubernetes client, the library is being built in the context of the consumer project meaning that it has access to the consumer node_modules. That bring the issue that typescript was trying to build metalk8s types dependency to build kubernetes client leading to a build failure. The issue has been resolved by this commit : scality/kubernetes-client-javascript@579d066 which lists explicitly the types dependency of kubernetes client.

JBWatenbergScality

Congrats, LGTM !

bert-e · 2021-02-02T14:42:25Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

ChengYanJin · 2021-02-02T15:37:23Z

/approve

bert-e · 2021-02-02T15:37:33Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.8

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2021-02-02T15:37:40Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.8

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7

Please check the status of the associated issue None.

Goodbye chengyanjin.

ChengYanJin requested a review from a team as a code owner January 15, 2021 16:20

ChengYanJin changed the title ~~Improvement/2932 display system partitions in node page~~ [UI] Display system partitions in node page Jan 15, 2021

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch 2 times, most recently from 1679c59 to 3277a91 Compare January 15, 2021 16:33

JBWatenbergScality reviewed Jan 18, 2021

View reviewed changes

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch from 354380f to 30d99ff Compare January 18, 2021 12:07

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch from 30d99ff to 0662278 Compare January 18, 2021 14:54

JBWatenbergScality reviewed Jan 19, 2021

View reviewed changes

ui/src/services/prometheus/fetchMetrics.js Outdated Show resolved Hide resolved

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch from bbd60c7 to 660558f Compare January 20, 2021 13:24

JBWatenbergScality reviewed Jan 20, 2021

View reviewed changes

ui/src/components/NodePartitionTable.js Outdated Show resolved Hide resolved

ui/src/components/NodePartitionTable.js Outdated Show resolved Hide resolved

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch 3 times, most recently from ccfe468 to 7252781 Compare January 21, 2021 15:15

JBWatenbergScality requested changes Jan 22, 2021

View reviewed changes

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch from a07aff7 to 8365d01 Compare January 22, 2021 09:44

ChengYanJin mentioned this pull request Jan 26, 2021

NodeFilesystemSpaceFillingUp alert does not show node name in message #2173

Closed

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch from 8365d01 to f222ca8 Compare January 27, 2021 10:25

ChengYanJin requested a review from JBWatenbergScality January 27, 2021 10:28

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch 3 times, most recently from 78031a1 to 6dd67ab Compare January 31, 2021 09:40

ChengYanJin added 10 commits February 1, 2021 10:32

ui/test: Add the test for nodepartition table using RTL

2201dd0

Refs: #2932

ui/component: Add aria-label for the status component

85c29a6

Refs: #2932

ui/test: Fix the issue of NodeVolumeUtils

cbbc89c

Refs: #2932

ui/component: Add aria-label in NodePartitionTable component

61591f2

Refs: #2932

ui/service/apiclient: Add comment for ApiClient

8949931

TODO: We should throw error immediately rather than catch the error and return This may change the behavior of the error handling of the APIs. Refs: #2932

ui/components: Throw error in NodePartitionTable

e83f9f0

Refs: #2932

ui/typing: Remove the never used code thanks to typing

39e63d4

Refs: #2932

ui/test: Add describe block for the tests to limit the scope of function

7c642f5

To make sure the functions beforeAll, afterEach, afterAll function in the correct place. Refs: #2932

ui: Throw out the error to trigger Promise reject

f49332b

Refs: #2932

ui/utils: Round the number instead of pareseInt

7831650

To make the data a little more accurate Refs: #2932

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch 9 times, most recently from 7bba179 to 72f96b6 Compare February 2, 2021 10:55

JBWatenbergScality force-pushed the improvement/2932-display-system-partitions-in-node-page branch from 72f96b6 to fa698ca Compare February 2, 2021 13:07

ChengYanJin force-pushed the improvement/2932-display-system-partitions-in-node-page branch 2 times, most recently from 2ed5b6f to 5d779b8 Compare February 2, 2021 13:24

JBWatenbergScality approved these changes Feb 2, 2021

View reviewed changes

bert-e merged commit 5d779b8 into development/2.8 Feb 2, 2021

bert-e deleted the improvement/2932-display-system-partitions-in-node-page branch February 2, 2021 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI] Display system partitions in node page #3045

[UI] Display system partitions in node page #3045

ChengYanJin commented Jan 15, 2021 •

edited

Loading

bert-e commented Jan 15, 2021

bert-e commented Jan 15, 2021

JBWatenbergScality left a comment

bert-e commented Jan 18, 2021

bert-e commented Jan 18, 2021

bert-e commented Jan 18, 2021

bert-e commented Jan 20, 2021

JBWatenbergScality left a comment

JBWatenbergScality Jan 22, 2021

JBWatenbergScality Jan 22, 2021

JBWatenbergScality Jan 22, 2021

JBWatenbergScality Jan 22, 2021

bert-e commented Jan 22, 2021

bert-e commented Jan 22, 2021

bert-e commented Jan 22, 2021

JBWatenbergScality left a comment

bert-e commented Feb 2, 2021

ChengYanJin commented Feb 2, 2021

bert-e commented Feb 2, 2021

bert-e commented Feb 2, 2021

[UI] Display system partitions in node page #3045

[UI] Display system partitions in node page #3045

Conversation

ChengYanJin commented Jan 15, 2021 • edited Loading

bert-e commented Jan 15, 2021

Hello chengyanjin,

bert-e commented Jan 15, 2021

Waiting for approval

JBWatenbergScality left a comment

Choose a reason for hiding this comment

bert-e commented Jan 18, 2021

Branches have diverged

bert-e commented Jan 18, 2021

Waiting for approval

bert-e commented Jan 18, 2021

Conflict

bert-e commented Jan 20, 2021

Waiting for approval

JBWatenbergScality left a comment

Choose a reason for hiding this comment

JBWatenbergScality Jan 22, 2021

Choose a reason for hiding this comment

JBWatenbergScality Jan 22, 2021

Choose a reason for hiding this comment

JBWatenbergScality Jan 22, 2021

Choose a reason for hiding this comment

JBWatenbergScality Jan 22, 2021

Choose a reason for hiding this comment

bert-e commented Jan 22, 2021

Waiting for approval

bert-e commented Jan 22, 2021

Conflict

bert-e commented Jan 22, 2021

Waiting for approval

JBWatenbergScality left a comment

Choose a reason for hiding this comment

bert-e commented Feb 2, 2021

Waiting for approval

ChengYanJin commented Feb 2, 2021

bert-e commented Feb 2, 2021

In the queue

bert-e commented Feb 2, 2021

ChengYanJin commented Jan 15, 2021 •

edited

Loading