[pve-devel] [PATCH manager 1/3] pvereport: fix multipath inclusion

[pve-devel] [PATCH manager 1/3] pvereport: fix multipath inclusion

[Aaron Lauterer]

we now push it to the correct hash if it is installed

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/Report.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Report.pm b/PVE/Report.pm
index a4a3d779..5ee3453d 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -79,7 +79,8 @@ my $init_report_cmds = sub {
 	push @{$report_def->{volumes}}, 'ceph status', 'ceph osd status', 'ceph df', 'pveceph status', 'pveceph pool ls';
     }
 
-    push @{$report_def->{disk}}, 'multipath -ll', 'multipath -v3' if cmd_exists('multipath');
+    push @{$report_def->{disks}}, 'multipath -ll', 'multipath -v3'
+	if cmd_exists('multipath');
 
     return $report_def;
 };
-- 
2.20.1

[pve-devel] [PATCH manager 2/3] pvereport: rework report contents

[Aaron Lauterer]

add:
* HA status
* ceph osd df tree
* ceph conf file and conf db
* ceph versions

removed:
* ceph status, as pveceph status is now printing the same information

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

@Thomas, we did discuss using the cluster/ceph/metadata endpoint off
list for more information about running services and other stuff like
needed restarts after updates.

Since it returns a lot of JSON that needs to be filtered to be useful
and not littering the report, I will need a bit more time to see what
would be needed and how to filter for that.

For now with the changed pveceph status (ceph -s) output we already get
an overview if any expected services are not running and with `ceph
versions` we get an idea which versions and if multiple versions are
present in the cluster.

I think this is okay for now to get a good impression and quite a lot
more hints where to investigate further.

 PVE/Report.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/PVE/Report.pm b/PVE/Report.pm
index 5ee3453d..f8d5e663 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -51,7 +51,8 @@ my $init_report_cmds = sub {
 	cluster => [
 	    'pvecm nodes',
 	    'pvecm status',
-	    'cat /etc/pve/corosync.conf 2>/dev/null'
+	    'cat /etc/pve/corosync.conf 2>/dev/null',
+	    'ha-manager status',
 	],
 	bios => [
 	    'dmidecode -t bios',
@@ -76,7 +77,9 @@ my $init_report_cmds = sub {
 
     if (-e '/etc/ceph/ceph.conf') {
 	# TODO: add (now working) rdb ls over all pools? really needed?
-	push @{$report_def->{volumes}}, 'ceph status', 'ceph osd status', 'ceph df', 'pveceph status', 'pveceph pool ls';
+	push @{$report_def->{volumes}}, 'pveceph status', 'ceph osd status',
+		'ceph df', 'ceph osd df tree', 'cat /etc/ceph/ceph.conf',
+		'ceph config dump', 'pveceph pool ls', 'ceph versions';
     }
 
     push @{$report_def->{disks}}, 'multipath -ll', 'multipath -v3'
-- 
2.20.1

[pve-devel] applied: Re: [PATCH manager 2/3] pvereport: rework report contents

[Thomas Lamprecht]

On 21/12/2020 16:13, Aaron Lauterer wrote:
> add:
> * HA status
> * ceph osd df tree
> * ceph conf file and conf db
> * ceph versions
> 
> removed:
> * ceph status, as pveceph status is now printing the same information
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> @Thomas, we did discuss using the cluster/ceph/metadata endpoint off
> list for more information about running services and other stuff like
> needed restarts after updates.
> 
> Since it returns a lot of JSON that needs to be filtered to be useful
> and not littering the report, I will need a bit more time to see what
> would be needed and how to filter for that.

yes, sure, that's what I said when proposing it - so yes, a filter is
required.
You could take a look at the web interface for relevant keys, there we
use that information already.

> 
> For now with the changed pveceph status (ceph -s) output we already get
> an overview if any expected services are not running and with `ceph
> versions` we get an idea which versions and if multiple versions are
> present in the cluster.
> 
> I think this is okay for now to get a good impression and quite a lot
> more hints where to investigate further.

I mean you get at least the running versions, which is probably the most
important thing, so this is fine for me for now.

> 
>  PVE/Report.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH manager 2/3] pvereport: rework report contents

[Thomas Lamprecht]

On 21/12/2020 16:13, Aaron Lauterer wrote:
> @@ -76,7 +77,9 @@ my $init_report_cmds = sub {
>  
>      if (-e '/etc/ceph/ceph.conf') {
>  	# TODO: add (now working) rdb ls over all pools? really needed?
> -	push @{$report_def->{volumes}}, 'ceph status', 'ceph osd status', 'ceph df', 'pveceph status', 'pveceph pool ls';
> +	push @{$report_def->{volumes}}, 'pveceph status', 'ceph osd status',
> +		'ceph df', 'ceph osd df tree', 'cat /etc/ceph/ceph.conf',
> +		'ceph config dump', 'pveceph pool ls', 'ceph versions';
>      }

Oh, and I followed up on this one. I know, it's tempting to do "partial splits"
once a line is to long, but it's better to consequently move all atoms to different
lines once that is needed in general.

E.g.:

push @{$report_def->{volumes}},
    'pveceph status',
    'ceph osd status',
    'ceph df',
    '...',
    'ceph versions',
    ;

One could "cheat" by use multiple push operations, each adding a few commands, but it's
not that many lines here, so...
Oh, and actually, I'm not 100% sure about the push and @array being in the same line,
I treated this now as single atom as it's a perl built-in and always "push on X" with
the elements as argument. If it was a "normal" sub, the first argument would need to go
on it's line too.

In short, mirror that what rustfmt would do for similar rust code not breaking
any of our specific style rules (like indentation).

mostly just noting as we had a few style exchanges/discussion in the last days
already ^^

[pve-devel] [PATCH manager 3/3] pvereport: code cleanup, line length

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/Report.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Report.pm b/PVE/Report.pm
index f8d5e663..228fbb29 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -73,7 +73,8 @@ my $init_report_cmds = sub {
 	],
     };
 
-    push @{$report_def->{volumes}}, 'zpool status', 'zpool list -v', 'zfs list' if cmd_exists('zfs');
+    push @{$report_def->{volumes}}, 'zpool status', 'zpool list -v', 'zfs list'
+	if cmd_exists('zfs');
 
     if (-e '/etc/ceph/ceph.conf') {
 	# TODO: add (now working) rdb ls over all pools? really needed?
-- 
2.20.1

[pve-devel] applied: Re: [PATCH manager 3/3] pvereport: code cleanup, line length

[Thomas Lamprecht]

On 21/12/2020 16:13, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  PVE/Report.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: Re: [PATCH manager 1/3] pvereport: fix multipath inclusion

[Thomas Lamprecht]

On 21/12/2020 16:13, Aaron Lauterer wrote:
> we now push it to the correct hash if it is installed
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  PVE/Report.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>
good find!

applied, thanks!