On 7/5/22 11:58, Thomas Lamprecht wrote:
Replying here albeit a few things also apply on the 2/5 patch. Note that I didn't checked the series as a whole (meaning, full UI and backend integration) but noticed a API issues here that has to be fixed anyway, so that plus some drive by reviews inline.
I can send a V2 with the suggested changes. [...]
+__PACKAGE__->register_method ({+ name => 'osdvolume', + path => '{osdid}/volume',adding a API path that isn't returned by any index breaks API schema generation, and the UX is also somewhat affected, NAK. For this to work the `{osdid}` API call would need to return the sub-paths too and set the href property to ensure the API stacks links them and allows to do the common completion in pvesh and correct display in the API viewer, among other things. But, that'd be naturally ugly too because you need to mix some generated data specific to the OSD with some static API path router/schema stuff. Instead, please transform {osdid} to a plain GET index call returning only the sub routes, and then add a separate sub-route in there for the details endpoint from patch 2/5, iow. the end result would be: {osdid}/ {osdid}/details {osdid}/volume Or with slightly changed route path names {osdid}/ {osdid}/metadata {osdid}/lv-info That would also allow to adding further routes in there in the future.
Thanks for the explanation. Will change the API to the latter variant, plus the index endpoint for {osdid} itself.
+ method => 'GET', + description => "Get OSD volume details", + proxyto => 'node', + protected => 1, + permissions => { + check => ['perm', '/', [ 'Sys.Audit', 'Datastore.Audit' ], any => 1],I know we use Datastore.Audit for some other ceph API calls, but that is mostly for higher level info about Proxmox VE registered storage clients, to be specific, the docs declare it as "view/browse a datastore". So I'd like to avoid expanding that usage further, especially for such detailed API calls that return a lot of specific system information. (same applies for patch 2/3)
Okay, so IIUC, drop the Datastore.Audit and only keep the Sys.Audit?
[...]
+ properties => { + creation_time => { + type => 'string', + description => "Creation time as reported by 'lvs'.",I'd use backticks for CLI tools, as this is interpreted by asciidoc too for manpage generation.
Will do.
[...]
+ my $cmd = ['/usr/sbin/ceph-volume', 'lvm', 'list', '--format', 'json']; + eval { run_command($cmd, errmsg => 'ceph volume error', outfunc => $parser) }; + die $@ if $@;what's the point in eval'ing if you do a plain die then anyway on $@?
Being unsure ;) Looking closer at run_command, it will die if there is some issue running the specified command, not found or returning an error. So this would only be needed if I would like to catch the error to handle it in some way, right?
+ + my $result; + if ($raw =~ m/^(\{.*\})$/s) { #untaint + $result = JSON::decode_json($1); + } else { + die "got unexpected data from ceph-volume: '${raw}'\n"; + } + die "OSD '${osdid}' not found in 'ceph-volume lvm list' on node '${nodename}'\n" + if !$result->{$osdid}; + + my $volume_data = {}; + %{$volume_data} = map { $_->{type} => $_ } @{$result->{$osdid}};huh? why not just my $volume_data = { map { $_->{type} => $_ } @{$result->{$osdid} } };
yeah... missed that when cleaning up the code
+ die "volume type '${type}' not found for OSD ${osdid}\n" if !$volume_data->{$type}; + + my $volume = $volume_data->{$type};just fyi, we can do a conditional die on variable declaration just fine, compared to the always bad conditionally `my $foo = "bar" if $condition` declaration. The former isn't touching the conditionality of the declaration (i.e., code will never be executed with a unsound value in there), while the latter means that there is an unsound value used when the condition evaluates to false. E.g., can be fine and sometimes shorter (albeit no hard feelings, I just like to combine extraction and error in one, like rust can do much more nicely) my $volume = $volume_data->{$type} || die "volume type '${type}' not found for OSD ${osdid}\n"
will change that
+ + $raw = ''; + $cmd = ['/sbin/lvs', $volume->{lv_path}, '--reportformat', 'json', '-o', 'lv_time']; + eval { run_command($cmd, errmsg => 'lvs error', outfunc => $parser) }; + die $@ if $@;same as above wrt. probably useless eval+ + if ($raw =~ m/(\{.*\})$/s) { #untaint, lvs has whitespace at beginning + $result = JSON::decode_json($1); + } else { + die "got unexpected data from lvs: '${raw}'\n"; + } + my $data = {}; + + my $keys = [ 'lv_name', 'lv_path', 'lv_uuid', 'vg_name' ]; + for my $key (@$keys) { + $data->{$key} = $volume->{$key}; + }could avoid about four lines with: my $data = { map { $_ => $volume->{$_} } qw(lv_name lv_path lv_uuid vg_name) };
true :) [...] _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
