Network component updates - not for merging #1175
Conversation
When an interface has to be renamed, ncm-network used to call ifdown for both the old and the new name. But if the new name did not exist before, that resulted in an error.
The actual renaming is handled by udev, so we need to kick it at the right time.
The fact is: any interfaces can be given any names, so the names are meaningless - at least for the tools, the interface names can be meaningful for humans.
The old-style ifconfig and route commands cannot display all network settings, only the new ip tool knows all the details.
| @@ -6,6 +6,41 @@ include 'pan/types'; | |||
| include 'quattor/functions/network'; | |||
|
|
|||
| @documentation{ | |||
| IPv4/IPv6 prefix notation | |||
| } | |||
| function is_ip_prefix = { | |||
There was a problem hiding this comment.
there is a is_ipv4_prefix_length and similar one for ipv6 in pan/types in template library core. this function should be moved there
| Single byte | ||
| } | ||
| # TODO: This should likely be defined elsewhere | ||
| type type_byte = long with { SELF >= 0 && SELF <= 255; }; |
| "num_grat_arp" ? long | ||
| "num_unsol_na" ? long | ||
| "resend_igmp" ? long | ||
| "mode" : long(0..6) # TODO: allow mode to be given as a string |
There was a problem hiding this comment.
why? dual typed values are not recommended. we have run into it on some other occasions, and i think they are used to migrate from legacy yes/no boolean strings to actual booleans.
There was a problem hiding this comment.
Well, I'd be happy to switch to string unconditionally :-) That's what we use internally, and it is much more readable.
| # Name of the auto-generated udev rule which needs to be removed to allow udev re-consider the interface names | ||
| "udev_rules_file" ? string | ||
| # E.g. "/usr/bin/udevadm trigger", although restricting the scope to just network devices should not hurt | ||
| "udev_trigger_command" ? string |
There was a problem hiding this comment.
what possible values could there be?
There was a problem hiding this comment.
Nowadays I think only what's written in the comments, but I'd need to check. RH4 had an old udev with a different interface, so the value had to be configurable. But the new udevadm CLI is stable for a while, so I'd not mind removing these commands from the templates and just hard-code them in the component.
| @@ -10,6 +10,12 @@ include 'quattor/schema'; | |||
|
|
|||
| type component_network_type = { | |||
| include structure_component | |||
| # Name of the auto-generated udev rule which needs to be removed to allow udev re-consider the interface names | |||
| "udev_rules_file" ? string | |||
There was a problem hiding this comment.
move all the udev_ attributes to a new udev related type
| }, | ||
| ); | ||
|
|
||
| my %mask_to_prefix = ( |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, that should likely happen here. We didn't want to introduce new dependencies when this code was originally written.
| # simple routes to the policy routing syntax | ||
| if (exists($net{$iface}{route})) { | ||
| foreach my $rt (sort keys %{$net{$iface}{route}}) { | ||
| my $route = {"dev" => $iface}; |
There was a problem hiding this comment.
this is not correct, should be devicename like https://github.com/quattor/configuration-modules-core/pull/1087/files#diff-90bc6b2e2a105ae5c62daf55ef95b2a7R1573
but new code makes this much easier; and also support arbitrary commands via cmd attribute
| $was_change = 1; | ||
| } | ||
| } | ||
| ## kick udev to rename devices if needed; do nothing if no files were changed |
There was a problem hiding this comment.
see also #1012
but we had some udev related issues with storage devices too, so maybe a CAF::udev might make sense
| ## kick udev to rename devices if needed; do nothing if no files were changed | ||
| if ($was_change) { | ||
| # Remove the auto-generated rule file if present | ||
| if ($config->elementExists("/software/components/network/udev_rules_file")) { |
There was a problem hiding this comment.
metaconfig has udev related TT files, maybe we can generate the udev files with metaconfig, and call the trigger/settle here?
There was a problem hiding this comment.
I don't want to create any udev files - but I need to remove the one udev generates automatically. If the auto-generated file exists, then udev will not look into the ifcfg-* files, and it will not notice that some interfaces which did not have a configuration before may need to be renamed.
There was a problem hiding this comment.
what autogenerates it? anaconda? i always tought that ifcfg file preceded udev rules if HWADDR was set.
the location of this autogenerated udev file is OS release specific i guess? it would be nice to add the value somewhere (for aii we have os flavours); or do you think we can hardcode the location?
| @@ -1145,6 +1312,9 @@ sub Configure | |||
| my @op; | |||
|
|
|||
| while (my ($k, $v) = each(%$opts)) { | |||
| if ($v->isType(LIST)) { | |||
There was a problem hiding this comment.
i'm surprised this works. i'm going to assume you use a recent CCM?
There was a problem hiding this comment.
It may not work :-) The original code which we still use is much uglier, and I just found this construct in one of the other components and decided it's not that horrible. I think you already have a much more generic solution in #1087, but I did not have time to try that so far.
There was a problem hiding this comment.
the ->getTree call a few lines higher returns arrayrefs for lists, that's why i was asking.
|
@gombasg this doesn't look too hard, iwas worried for something far worse 😄 |
|
@stdweird: I just wanted to push this out to show what we'd like to be included, to make sure the changes you're working on are not outright incompatible. We should have published these changes a long time ago... |
|
@stdweird confirmed at RAL workshop that none of these items are in the latest ncm-network rewrite so they will all need to be reimplemented. |
This branch contains our changes to ncm-network, rebased on top of current master. I cleaned up the obvious conflicts during rebasing, but this is mostly untested. I'm looking into porting these changes on top of #1087, but that may take a while.