ncm-network: allow NetworkManager configuration for EL9 compatibility#1585
ncm-network: allow NetworkManager configuration for EL9 compatibility#1585victor-mendoza wants to merge 2 commits intoquattor:masterfrom
Conversation
| # main network config | ||
| return if ! defined($self->mk_bu($NETWORKCFG)); | ||
|
|
||
| # is NetworkManager is allowed?, false by default |
There was a problem hiding this comment.
it's not false by default, because you have defined(allow_nm) in all the logic above.
There was a problem hiding this comment.
allow_nm was already part of the schema, one must explicitly define it as true in the profile otherwise it will be set to false by : my $allow_nm = $nwtree->{allow_nm};.
There was a problem hiding this comment.
ok, but then you don't need the defined() tests in this code
There was a problem hiding this comment.
OK, I'll remove them
| if (!$self->_is_executable($CHKCONFIG_CMD) && defined($allow_nm) && $allow_nm) { | ||
| $service_name = "NetworkManager"; | ||
| } else { | ||
| $self->error("chkconfig unavailable, NetworkManager not allowed"); |
There was a problem hiding this comment.
what does chkconfig have to do with NetworkManager?
There was a problem hiding this comment.
nothing, if chkconfig isn't found (as is the case on EL9), it will fallback to NetworkManager, if allow_nm is set to true in the profile.
There was a problem hiding this comment.
chkconfig can still be present because of other tools on the system that need it
There was a problem hiding this comment.
In that case I'll try adding a new schema entry to unambiguously choose the desired network service, and stop checking for the presence of chkconfig.
There was a problem hiding this comment.
nothing, if chkconfig isn't found (as is the case on EL9), it will fallback to NetworkManager, if
allow_nmis set totruein the profile.
chkconfig is found in rhel9, what fails is "chkconfig --level 2345 network on", as there is no service Network on rhel9. on our rhel9 build we have to use chkconfig for other services (legacy), so this code will fail.
| my @cmds; | ||
| foreach my $iface (@ifaces) { | ||
| push(@cmds, ["/sbin/ifup", $iface, "boot"]); | ||
| if ($self->_is_executable($IFUP_CMD)) { |
There was a problem hiding this comment.
this requires the networkmanager compatibility rpm?
There was a problem hiding this comment.
ok, not requires, but does it work when that rpm is present?
There was a problem hiding this comment.
yes, it will use the legacy ifup/ifdown scripts if they are installed, otherwise it will use nmcli (if allowed)
There was a problem hiding this comment.
tbh i would require the ifup/ifdown being present, and not implement your own version of it like the link removal code if ifdown is not present.
There was a problem hiding this comment.
OK, I'll revert this.
| if (@ifaces) { | ||
| my @cmds; | ||
| # delete NetworkManager connections | ||
| foreach my $file ( glob "/etc/NetworkManager/system-connections/*.nmconnection" ) { |
There was a problem hiding this comment.
i don't know how networkmanager works, but ncm-network will try it's best to restore previous working network if the component would hit some error.
if these files are required for such recover, you best not delete them; but eg rename and make sure they get restored if needed (and cleaned up when working and really not needed anymore)
There was a problem hiding this comment.
Any automatically (during install) or manually created files in that path will take precedence, they are deleted so the legacy 'ifcfg' files are used by NetworkManager instead, the backup/recovery will continue to work.
There was a problem hiding this comment.
not if this is the initial install, then recovery will be broken without these files.
There was a problem hiding this comment.
OK, I'll try to add these files to the recovery instead
| # how do we actually know that the device was up? | ||
| # eg for non-existing device eth4: /sbin/ifdown eth4 --> usage: ifdown <device name> | ||
| push(@cmds, ["/sbin/ifdown", $iface]); | ||
| if ($self->_is_executable($IFDOWN_CMD)) { |
There was a problem hiding this comment.
you asume that ifup is present below, but you check it is missing ifdown?
There was a problem hiding this comment.
you lost me, ifup is only used in sub start
| push(@cmds, ["ip", "link", "set", $iface, "down"]); | ||
| } | ||
| } else { | ||
| # stop the interface |
There was a problem hiding this comment.
at the very least log an error here. because this is not permanent removal
There was a problem hiding this comment.
can you please elaborate? this is just trying to reset the interface to a default state so the new configuration can be applied with a greater chance of success
| $self->runrun(@disablenm_cmds); | ||
| if ($self->_is_executable($CHKCONFIG_CMD)) { | ||
| # TODO: do something smart with 'require NCM::Component::Systemd::...' to turn it off | ||
| return $self->runrun([qw(/sbin/chkconfig --level 2345 network off)]); |
There was a problem hiding this comment.
why do you stop network, while the comments only mention networkmanager
and also, what does this have to do with chkconfig being present
There was a problem hiding this comment.
my mistake, I'll fix this
|
Dropping this mess in favor of a better solution as suggested in #1586 |
3 participants
Justification:
chkconfigand the legacynetworkservice have been deprecated.nmcli/ipcommands to start and stop the network interfaces if the legacyifup/ifdownscripts aren't installed.Incompatibilities:
Acknowledgment: