On April 16, 2021 12:17 pm, Thomas Lamprecht wrote:
> On 13.04.21 14:16, Fabian Grünbichler wrote:
>> Signed-off-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
>> ---
>> PVE/API2/Nodes.pm | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 70 insertions(+)
>>
>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>> index ba6621c6..b30d0739 100644
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -222,6 +222,7 @@ __PACKAGE__->register_method ({
>> my ($param) = @_;
>>
>> my $result = [
>> + { name => 'addr' },
>> { name => 'aplinfo' },
>> { name => 'apt' },
>> { name => 'capabilities' },
>> @@ -2183,6 +2184,75 @@ __PACKAGE__->register_method ({
>> return undef;
>> }});
>>
>> +__PACKAGE__->register_method ({
>> + name => 'get_node_addr',
>> + path => 'addr',
>
> hmm, not so sure if this the best path choice, if network wouldn't be
> templated by
> the {iface} param that would be a better choice to avoid crowding here.
>
> I pondered over moving that one a level deeper to network/if/{name} for a
> major releas,
> and mode some things like netstat, possible even dns, under there - but it's
> work and
> the gain was rather small - with this call which could fit there ROI would be
> slightly
> bigger, but not too sure if worth it, just throwing out the idea there.
yeah, I would have liked to put it under network/, but didn't want to
bother with the breaking change. with 7.0 it might be a good opportunity
though.. maybe there is SDN stuff that could fit in as well?
> Besides that, the name could be slightly more descriptive `ip-addr` or even
> ip-addresses`?
sure, could be done.
>> + method => 'GET',
>> + proxyto => 'node',
>> + permissions => {
>> + check => ['perm', '/', [ 'Sys.Audit' ]],
>
> why not `/nodes/{node}` ?
not sure, but probably a good idea (also required to read the interface
config which is about the same level of "secrecy" I'd say.
>
>> + },
>> + description => "Get the content of /etc/hosts.",
>
> above is wrong? probably left-over from copying?
yes.
>
>> + parameters => {
>> + additionalProperties => 0,
>> + properties => {
>> + node => get_standard_option('pve-node'),
>> + cidr => {
>> + type => 'string',
>> + format => 'CIDR',
>> + format_description => 'CIDR',
>> + description => 'Extra network for which to retrieve local
>> address(es).',
>> + optional => 1,
>> + },
>> + },
>> + },
>> + returns => {
>> + type => 'object',
>> + properties => {
>> + default => {
>> + type => 'string',
>> + description => 'Default node IP.',
>> + format => 'ip',
>> + },
>> + migration => {
>> + type => 'array',
>> + items => {
>> + type => 'string',
>> + description => 'Migration network IP(s).',
>> + format => 'ip',
>> + },
>> + optional => 1,
>> + },
>> + extra => {
>> + type => 'array',
>> + items => {
>> + type => 'string',
>> + description => 'Extra network IP(s).',
>> + format => 'ip',
>> + },
>> + optional => 1,
>> + },
>> + },
>> + },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + my $data = {};
>> +
>> + my $default = PVE::Cluster::remote_node_ip($param->{node});
>> +
>> + my $dc_conf = cfs_read_file('datacenter.cfg');
>> + my $migration = $dc_conf->{migration}->{network};
>> +
>> + $data->{default} = $default if defined($default);
>> + $data->{migration} = PVE::Network::get_local_ip_from_cidr($migration, 1)
>> + if $migration;
>
> style nit, probably opinionated and thus really no hard feelings, but maybe
> the
> following is lightly more legible due to declaration being nearer to usage:
>
> if (my $default = PVE::Cluster::remote_node_ip($param->{node}) {
> $data->{default} = $default;
> }
>
> my $dc_conf = cfs_read_file('datacenter.cfg');
> if (my $migration = $dc_conf->{migration}->{network}) {
> $data->{migration} = PVE::Network::get_local_ip_from_cidr($migration, 1);
> }
>
> if (my $cidr = $param->{cidr}) {
> $data->{extra} = PVE::Network::get_local_ip_from_cidr($param->{cidr}, 1)
> }
LGTM
>
>> + $data->{extra} = PVE::Network::get_local_ip_from_cidr($param->{cidr}, 1)
>> + if $param->{cidr};
>> +
>> + return $data;
>> + }});
>> +
>> # bash completion helper
>>
>> sub complete_templet_repo {
>>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
