PR#50700: Issue 50699 - Add Disk Monitor to CLI and UI - 389-ds-base

389-ds-base

#50700 Issue 50699 - Add Disk Monitor to CLI and UI

Closed 14 days ago by spichugi. Opened 10 months ago by mreynolds.

mreynolds/389-ds-base issue50699 into master

Issue 50699 - Add Disk Monitor to CLI and UI

Mark Reynolds • 10 months ago

0493b01

src/cockpit/389-console/src/lib/monitor/monitorTables.jsx

file modified

+117 -1

		`@@ -8,7 +8,7 @@`
		`tableCellFormatter,`
		`noop`
		`} from "patternfly-react";`
		`- import { DSTable } from "../dsTable.jsx";`
		`+ import { DSTable, DSShortTable } from "../dsTable.jsx";`
		`import PropTypes from "prop-types";`
		`import "../../css/ds.css";`
		`import { get_date_string } from "../tools.jsx";`
		`@@ -1584,6 +1584,121 @@`
		`}`
		`}`

		`+ class DiskTable extends React.Component {`
		`+ constructor(props) {`
		`+ super(props);`
		`+`
		`+ this.state = {`
		`+ rowKey: "mount",`
		`+ columns: [`
		`+ {`
		`+ property: "mount",`
		`+ header: {`
		`+ label: "Disk Partition",`
		`+ props: {`
		`+ index: 0,`
		`+ rowSpan: 1,`
		`+ colSpan: 1,`
		`+ sort: true`
		`+ },`
		`+ transforms: [],`
		`+ formatters: [],`
		`+ customFormatters: [sortableHeaderCellFormatter]`
		`+ },`
		`+ cell: {`
		`+ props: {`
		`+ index: 0`
		`+ },`
		`+ formatters: [tableCellFormatter]`
		`+ }`
		`+ },`
		`+ {`
		`+ property: "size",`
		`+ header: {`
		`+ label: "Disk Size",`
		`+ props: {`
		`+ index: 1,`
		`+ rowSpan: 1,`
		`+ colSpan: 1,`
		`+ sort: true`
		`+ },`
		`+ transforms: [],`
		`+ formatters: [],`
		`+ customFormatters: [sortableHeaderCellFormatter]`
		`+ },`
		`+ cell: {`
		`+ props: {`
		`+ index: 1`
		`+ },`
		`+ formatters: [tableCellFormatter]`
		`+ }`
		`+ },`
		`+ {`
		`+ property: "used",`
		`+ header: {`
		`+ label: "Used Space",`
		`+ props: {`
		`+ index: 2,`
		`+ rowSpan: 1,`
		`+ colSpan: 1,`
		`+ sort: true`
		`+ },`
		`+ transforms: [],`
		`+ formatters: [],`
		`+ customFormatters: [sortableHeaderCellFormatter]`
		`+ },`
		`+ cell: {`
		`+ props: {`
		`+ index: 2`
		`+ },`
		`+ formatters: [tableCellFormatter]`
		`+ }`
		`+ },`
		`+ {`
		`+ property: "avail",`
		`+ header: {`
		`+ label: "Available Space",`
		`+ props: {`
		`+ index: 3,`
		`+ rowSpan: 1,`
		`+ colSpan: 1,`
		`+ sort: true`
		`+ },`
		`+ transforms: [],`
		`+ formatters: [],`
		`+ customFormatters: [sortableHeaderCellFormatter]`
		`+ },`
		`+ cell: {`
		`+ props: {`
		`+ index: 3`
		`+ },`
		`+ formatters: [tableCellFormatter]`
		`+ }`
		`+ },`
		`+`
		`+ ]`
		`+ };`
		`+ this.getColumns = this.getColumns.bind(this);`
		`+ }`
		`+`
		`+ getColumns() {`
		`+ return this.state.columns;`
		`+ }`
		`+`
		`+ render() {`
		`+ return (`
		`+ <div className="ds-margin-top-xlg">`
		`+ <DSShortTable`
		`+ getColumns={this.getColumns}`
		`+ rowKey={this.state.rowKey}`
		`+ rows={this.props.disks}`
		`+ disableLoadingSpinner`
		`+ />`
		`+ </div>`
		`+ );`
		`+ }`
		`+ }`
		`+`
		`// Proptypes and defaults`

		`LagReportTable.propTypes = {`
		`@@ -1681,4 +1796,5 @@`
		`AbortCleanALLRUVTable,`
		`ConflictTable,`
		`GlueTable,`
		`+ DiskTable,`
		`};`

src/cockpit/389-console/src/lib/monitor/serverMonitor.jsx

file modified

+17 -1

		`@@ -3,9 +3,11 @@`
		`import "../../css/ds.css";`
		`import { get_date_string, get_date_diff } from "../tools.jsx";`
		`import {`
		`- ConnectionTable`
		`+ ConnectionTable,`
		`+ DiskTable,`
		`} from "./monitorTables.jsx";`
		`import {`
		`+ Button,`
		`Nav,`
		`NavItem,`
		`TabContent,`
		`@@ -66,6 +68,9 @@`
		`<NavItem eventKey={2}>`
		`<div dangerouslySetInnerHTML={{__html: 'Connection Table'}} />`
		`</NavItem>`
		`+ <NavItem eventKey={3}>`
		`+ <div dangerouslySetInnerHTML={{__html: 'Disk Space'}} />`
		`+ </NavItem>`
		`</Nav>`
		`<TabContent>`

		`@@ -191,6 +196,17 @@`
		`<TabPane eventKey={2}>`
		`<ConnectionTable conns={this.props.data.connection} />`
		`</TabPane>`
		`+ <TabPane eventKey={3}>`
		`+ <DiskTable`
		`+ disks={this.props.disks}`
		`+ />`
		`+ <Button`
		`+ className="ds-margin-top"`
		`+ onClick={this.props.reloadDisks}`
		`+ >`
		`+ Refresh`
		`+ </Button>`
		`+ </TabPane>`
		`</TabContent>`
		`</div>`
		`</TabContainer>`

src/cockpit/389-console/src/monitor.jsx

file modified

+45 -2

		`@@ -39,6 +39,7 @@`
		`snmpData: {},`
		`ldbmData: {},`
		`serverData: {},`
		`+ disks: [],`
		`loadingMsg: "",`
		`notifications: [],`
		`disableTree: false,`
		`@@ -124,6 +125,8 @@`
		`this.loadMonitorServer = this.loadMonitorServer.bind(this);`
		`this.reloadServer = this.reloadServer.bind(this);`
		`this.loadMonitorChaining = this.loadMonitorChaining.bind(this);`
		`+ this.loadDiskSpace = this.loadDiskSpace.bind(this);`
		`+ this.reloadDisks = this.reloadDisks.bind(this);`
		`// Replication`
		`this.loadMonitorReplication = this.loadMonitorReplication.bind(this);`
		`this.loadCleanTasks = this.loadCleanTasks.bind(this);`
		`@@ -457,7 +460,7 @@`
		`replicatedSuffixes: config.items,`
		`replSuffix: replSuffix,`
		`});`
		`- }, this.loadMonitorLDBM());`
		`+ }, this.loadDiskSpace());`
		`}`

		`loadMonitorLDBM() {`
		`@@ -528,7 +531,7 @@`
		`this.setState({`
		`serverLoading: false,`
		`serverData: config.attrs`
		`- });`
		`+ }, this.reloadDisks());`
		`});`
		`}`

		`@@ -548,6 +551,44 @@`
		`});`
		`}`

		`+ loadDiskSpace() {`
		`+ let cmd = [`
		`+ "dsconf", "-j", "ldapi://%2fvar%2frun%2fslapd-" + this.props.serverId + ".socket",`
		`+ "monitor", "disk"`
		`+ ];`
		`+ log_cmd("loadDiskSpace", "Load disk space info", cmd);`
		`+ cockpit`
		`+ .spawn(cmd, { superuser: true, err: "message" })`
		`+ .done(content => {`
		`+ let disks = JSON.parse(content);`
		`+ for (let disk of disks.items) {`
		`+ disk.used = disk.used + " (" + disk.percent + "%)";`
		`+ }`
		`+ this.setState({`
		`+ disks: disks.items`
		`+ });`
		`+ }, this.loadMonitorLDBM());`
		`+ }`
		`+`
		`+ reloadDisks () {`
		`+ let cmd = [`
		`+ "dsconf", "-j", "ldapi://%2fvar%2frun%2fslapd-" + this.props.serverId + ".socket",`
		`+ "monitor", "disk"`
		`+ ];`
		`+ log_cmd("reloadDisks", "Reload disk stats", cmd);`
		`+ cockpit`
		`+ .spawn(cmd, { superuser: true, err: "message" })`
		`+ .done(content => {`
		`+ let disks = JSON.parse(content);`
		`+ for (let disk of disks.items) {`
		`+ disk.used = disk.used + " (" + disk.percent + "%)";`
		`+ }`
		`+ this.setState({`
		`+ disks: disks.items,`
		`+ });`
		`+ });`
		`+ }`
		`+`
		`reloadSNMP() {`
		`this.setState({`
		`snmpLoading: true`
		`@@ -1053,6 +1094,8 @@`
		`data={this.state.serverData}`
		`reload={this.reloadServer}`
		`serverId={this.props.serverId}`
		`+ disks={this.state.disks}`
		`+ reloadDisks={this.reloadDisks}`
		`enableTree={this.enableTree}`
		`/>;`
		`}`

src/lib389/lib389/cli_conf/monitor.py

file modified

+37 -2

		`@@ -7,9 +7,11 @@`
		`# See LICENSE for details.`
		`# --- END COPYRIGHT BLOCK ---`

		`- from lib389.monitor import (Monitor, MonitorLDBM, MonitorSNMP)`
		`+ import json`
		`+ from lib389.monitor import (Monitor, MonitorLDBM, MonitorSNMP, MonitorDiskSpace)`
		`from lib389.chaining import (ChainingLinks)`
		`from lib389.backend import Backends`
		`+ from lib389.utils import convert_bytes`


		`def _format_status(log, mtype, json=False):`
		`@@ -63,9 +65,39 @@`
		`for link in links.list():`
		`link_monitor = link.get_monitor()`
		`_format_status(log, link_monitor, args.json)`
		`- # Inejct a new line for now ... see https://pagure.io/389-ds-base/issue/50189`
		`+ # Inject a new line for now ... see https://pagure.io/389-ds-base/issue/50189`
		`log.info("")`

		`+ def disk_monitor(inst, basedn, log, args):`
		`+ disk_space_mon = MonitorDiskSpace(inst)`
		`+ disks = disk_space_mon.get_disks()`
		`+ disk_list = []`
		`+ for disk in disks:`
		`+ # partition="/" size="52576092160" used="25305038848" available="27271053312" use%="48"`
		`+ parts = disk.split()`
		`+ mount = parts[0].split('=')[1].strip('"')`
		`+ disk_size = convert_bytes(parts[1].split('=')[1].strip('"'))`
		`+ used = convert_bytes(parts[2].split('=')[1].strip('"'))`
		`+ avail = convert_bytes(parts[3].split('=')[1].strip('"'))`
		`+ percent = parts[4].split('=')[1].strip('"')`
		`+ if args.json:`
		`+ disk_list.append({`
		`+ 'mount': mount,`
		`+ 'size': disk_size,`
		`+ 'used': used,`
		`+ 'avail': avail,`
		`+ 'percent': percent`
		`+ })`
		`+ else:`
		`+ log.info("Partition: " + mount)`
		`+ log.info("Size: " + disk_size)`
		`+ log.info("Used Space: " + used)`
		`+ log.info("Available Space: " + avail)`
		`+ log.info("Percentage Used: " + percent + "%\n")`
		`+`
		`+ if args.json:`
		`+ log.info(json.dumps({"type": "list", "items": disk_list}))`
		`+`

		`def create_parser(subparsers):`
		`monitor_parser = subparsers.add_parser('monitor', help="Monitor the state of the instance")`
		`@@ -87,3 +119,6 @@`
		`chaining_parser = subcommands.add_parser('chaining', help="Monitor database chaining statistics")`
		`chaining_parser.add_argument('backend', nargs='?', help="Optional name of the chaining backend to monitor")`
		`chaining_parser.set_defaults(func=chaining_monitor)`
		`+`
		`+ disk_parser = subcommands.add_parser('disk', help="Disk space statistics. All values are in bytes")`
		`+ disk_parser.set_defaults(func=disk_monitor)`

src/lib389/lib389/utils.py

file modified

+12

		`@@ -39,6 +39,7 @@`
		`import shlex`
		`import operator`
		`import subprocess`
		`+ import math`
		`from packaging.version import LegacyVersion`
		`from socket import getfqdn`
		`from ldapurl import LDAPUrl`
		`@@ -1302,3 +1303,14 @@`
		`def display_log_data(data, hide_sensitive=True):`
		`# Take a dict and mask all the sensitive data`
		`return {a: display_log_value(a, v, hide_sensitive) for a, v in data.items()}`
		`+`
		`+`
		`+ def convert_bytes(bytes):`
		`+ bytes = int(bytes)`
		`+ if bytes == 0:`
		`+ return "0 B"`
		`+ size_name = ["B", "KB", "MB", "GB", "TB", "PB"]`
		`+ i = int(math.floor(math.log(bytes, 1024)))`
		`+ pow = math.pow(1024, i)`
		`+ siz = round(bytes / pow, 2)`
		`+ return "{} {}".format(siz, size_name[i])`

mreynolds commented 10 months ago

Description: Add the disk monitoring to the CLI and UI

relates: https://pagure.io/389-ds-base/issue/50699

spichugi commented 10 months ago

I think it is simple and good enough for a minimal representation.

Ideally, I think, we better have a 'human-readable" format option for both UI and CLI.

What do you think?

spichugi commented 10 months ago

Like we can add a setting for choosing units. Or just make a switch for representing in Mb (and for CLI it will be a true-false option)

mreynolds commented 10 months ago

I think it is simple and good enough for a minimal representation.
Ideally, I think, we better have a 'human-readable" format option for both UI and CLI.
What do you think?

The UI is human readable, how is it not? Now, the CLI could definitely be more readable

Like we can add a setting for choosing units. Or just make a switch for representing in Mb (and for CLI it will be a true-false option)

Yeah I can do that

rebased onto 012887a1972c9473abd9d05dfb391ee7935bf74f

10 months ago

mreynolds commented 10 months ago

@spichugi So to fix this I always convert bytes to a more friendly value, and I improved the CLI human readable format. Please review...

spichugi commented 10 months ago

Minor thing - loadDiskSpace and reloadDisks have nearly the same code (I think only the log message is different). I think it would be better while encasulated...

The rest looks good. Ack.

mreynolds commented 10 months ago

Minor thing - loadDiskSpace and reloadDisks have nearly the same code (I think only the log message is different). I think it would be better while encasulated...
The rest looks good. Ack.

Sorry not sure what you mean. The big difference between the load and reload functions is that the load function calls other loading functions once its finishes setting the state. This is how it's done throughout the rest of the UI code.

spichugi commented 10 months ago

I wonder if we can do something like this:

loadDiskSpace(firstLoad) {
    let cmd = [
        "dsconf", "-j", "ldapi://%2fvar%2frun%2fslapd-" + this.props.serverId + ".socket",
        "monitor", "disk"
    ];
    log_cmd("loadDiskSpace", "Load disk space info", cmd);
    cockpit
            .spawn(cmd, { superuser: true, err: "message" })
            .done(content => {
                let disks = JSON.parse(content);
                for (let disk of disks.items) {
                    disk.used = disk.used + " (" + disk.percent + "%)";
                }
                this.setState({
                    disks: disks.items
                });
                if (firstLoad) {
                    this.loadMonitorLDBM());
                }
             }
}

loadDisks() {
    this.loadDiskSpace(true);
}

reloadDisks() {
    this.loadDiskSpace(false);
}

I think it should work even if we put the this.loadMonitorLDBM()); callback inside of the callback from done().

mreynolds commented 10 months ago

That won't work correctly. The nested loading function must be "within" the setState call:

this.setState({
....
}, this.loadNextFunc);

I think it's best to leave the functions as separate functions. IMHO having similar code in two functions (that could be merged into a single function) is not as bad as it's made out to be. I feel ideology of avoiding duplicate code at all costs is antiquated. There are benefits to it, especially if the needs of the reload verses nested loading changes in the future. I feel it's more flexible and easier to read than adding a lot of wrappers and if/then/else conditions.

If you feel that strongly about it I will change it, but it also means we have to rewrite a lot of other code to be consistent with that model.

spichugi commented 10 months ago

Okay, I got you. I think it makes sense to leave it like this here.

I think it's worth to have it in other places though... Like when we modify/add some modal window. It may have a lot of boilerplate react code and it can be nicely encapsulated for these kinds of similar functions.

You have my ack here! Thanks for the explanation!

mreynolds commented 10 months ago

Okay, I got you. I think it makes sense to leave it like this here.
I think it's worth to have it in other places though... Like when we modify/add some modal window. It may have a lot of boilerplate react code and it can be nicely encapsulated for these kinds of similar functions.

Please file an issue to address these areas.