[pve-devel] applied: [PATCH pve-network 0/2] improvments

[Alexandre DERUMIER]

>>
>>As long as we have a clear dependency releationship between the
>>packages this could be OK. I.e., probably:
>>
>>common <- network  <----------------- manager
>>            ^
>>             `- qemu-server/container <-/ 


Hi,
I have checked in the code, and they are a lot of small sub in Network.pm
called in differents modules

ex:

Cluster.pm:    return PVE::Network::get_ip_from_hostname($nodename, $noerr);
LXC/Setup/Base.pm:	    if ($has_ipv4 && !PVE::Network::is_ip_in_cidr($gw, $d->{ip}, 4)) {
Storage/ISCSIPlugin.pm:    return PVE::Network::tcp_ping($server, $port || 3260, 2);
Firewall.pm:use PVE::Network;
Firewall.pm:		$mask = $PVE::Network::ipv4_mask_hash_localnet->{$entry->{mask}};




subs in Network.pm are mostly helpers and tools, so I think it's better to keep them in common package.
(to avoid cycling depend later).

Code in Inotify.pm is more related to /etc/network/interfaces generation, so it's better to move that
in pve-network.

Same for API2/Network.pm, it's only related to config management, can be move to pve-network.

what do you think about this ?



----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com>
Cc: "Fabian Grünbichler" <f.gruenbichler at proxmox.com>
Envoyé: Jeudi 6 Juin 2019 14:58:27
Objet: Re: [pve-devel] applied: [PATCH pve-network 0/2] improvments

On 6/6/19 10:16 AM, Alexandre DERUMIER wrote: 
>>> On another note, is there any specific reason we have a double "Network" 
>>> in the module namespace? Or could we change it to 
>>> 
>>> PVE/Network.pm 
>>> PVE/Network/VlanPlugin.pm 
>>> ... 
> 
>>> and respective module name-path changes? As now, with not much external 
>>> code using this, it would be still relatively easy to change.. I mean 
>>> it's nothing to important, but it irked me a little bit a few times ^^ 
>>> I could do the change, if you agree and have no objections :-) 
> 
> This is because we already have a PVE/Network.pm. 

ah yeah... 

> 
> I wonder if I could rename this to PVE::Network::SDN instead PVE::Network::Network, to avoid confusion ? 

OK for me. 

> 
> also, as I see that proxmox6 is now the master branch, 
> 
> I think it could be great to move /PVE/API2/Network.pm from pve-manager package 
> and PVE/Network.pm from pve-common 
> to this package. 
> 
> (I would like to move the ifquery code to PVE/Network.pm, and begin to add SDN code in /PVE/API2/Network reload) 
> 
> what do you think about this ? 

As long as we have a clear dependency releationship between the 
packages this could be OK. I.e., probably: 

common <- network <----------------- manager 
^ 
`- qemu-server/container <-/ 

Not sure what info / perl modules you all want to access from 
pve-network, but guest will want to take use of pve-network and 
manager too, so as long as you do not need to access any guest 
(qemu-server, pve-container) and pve-manager modules from ther 
we'd be good, pve-guest-common could probably be used. 

looking at PVE::API2::Network's module usage it seems that it 
would be movable without problems. 

for PVE::Network we would need to either move PVE::INotify too, 
or split it up and move the network part's from it only to 
pve-network package, latter is IMO more reasonable.. 
Only problem here is that our Mailgateway uses INotify too quite 
a bit so we'd need to take a closer look at this part... 




> ----- Mail original ----- 
> De: "Thomas Lamprecht" <t.lamprecht at proxmox.com> 
> À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com> 
> Envoyé: Jeudi 6 Juin 2019 08:32:24 
> Objet: applied: [pve-devel] [PATCH pve-network 0/2] improvments 
> 
> On 6/6/19 8:20 AM, Alexandre Derumier wrote: 
>> - fix multicast vxlan default mtu. (should be 50 bytes lower than physdev) 
>> - clean status sub from Thomas comments 
>> 
>> Alexandre Derumier (2): 
>> vxlanmulticast : fix mtu 
>> cleanup status sub 
>> 
>> PVE/Network/Network.pm | 13 ++++++++----- 
>> PVE/Network/Network/VlanPlugin.pm | 3 +-- 
>> PVE/Network/Network/VxlanMulticastPlugin.pm | 7 +++++-- 
>> test/generateconfig.pl | 3 ++- 
>> 4 files changed, 16 insertions(+), 10 deletions(-) 
>> 
> 
> applied, thanks! 
> 
> On another note, is there any specific reason we have a double "Network" 
> in the module namespace? Or could we change it to 
> 
> PVE/Network.pm 
> PVE/Network/VlanPlugin.pm 
> ... 
> 
> and respective module name-path changes? As now, with not much external 
> code using this, it would be still relatively easy to change.. I mean 
> it's nothing to important, but it irked me a little bit a few times ^^ 
> I could do the change, if you agree and have no objections :-) 
> 
> cheers, 
> Thomas 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
>