If a route is not in platform, doesn't have force-commit and the
commit type is not "reapply", we still need to apply it if it failed
previously. Otherwise, e.g. a route with a src address that is
temporarily not available is never applied again after the first
failure.
The change in behavior introduced by the patch departs from the policy
that NM had for long time of trying not to interfere with user
modifications. This seems a fundamental aspect of how NM works and
indeed we got already one report:
https://bugzilla.redhat.com/show_bug.cgi?id=2218866
of a user that was affected by the change. The specific case is about
routes from DHCP, but also static routes are affected. When a user
removes the route added by NM, NM now can add it back at any time.
Changing behavior is bad, it causes pain for users and for people who
need to support them. However, if the new behavior provides clear
advantages to users, that might be ok. This doesn't seem the case in
my opinion. Commit 7ca95cee describes a couple of scenarios:
> - kernel can automatically remove routes. For example, deleting an
> IPv4 address that is the prefsrc of a route, will cause kernel to
> delete that route. Sure, we may be unable to re-configure the
> route at this moment, but we shouldn't remember indefinitely that
> the route is supposed to be absent. Rather, we should re-add it
> when possible
> - kernel is a pain with validating consistencies of routes. For
> example, when a route has a nexthop gateway, then the gateway must
> be onlink (directly reachable), or kernel refuses to add it with
> "Nexthop has invalid gateway". Of course, when removing the onlink
> route kernel is fine leaving the gateway route behind, which it
> would otherwise refuse to add.
> Anyway. Such interdependencies for when kernel rejects adding a
> route with "Nexthop has invalid gateway" are non-trivial. We try
> to work around that by always adding the necessary onlink
> routes. See nm_l3_config_data_add_dependent_onlink_routes(). But
> if the user externally removed the dependent onlink route, and
> NetworkManager remembers to not re-adding it, then the efforts
> from nm_l3_config_data_add_dependent_onlink_routes() are
> ignored. This causes ripple effects and NetworkManager will also
> be unable to add the nexthop route.
Kernel usually removes addresses as consequence of user actions. If
not (e.g. DHCP lease expiring) we have solutions in place for that to
re-add the route.
If the route removal is the consequence of a user action, then the
user must do something to undo it. For example, if the user removes an
address on the same interface, a route using the address as prefsrc
will be deleted. If the user wants it back, it must be re-added
manually together with the address; I don't see any problem with this.
The prefsrc address could be on another interface; in such case by
simply deactivating the connection providing the address, a dependent
route could be removed on another interface and never readded. This
doesn't look as a setup that anybody would use; in case we need to
support it, it is better to find alternative solutions.
So, my opinion is that the change in behavior potentially breaks many
users and doesn't bring clear advantages. Therefore, restore the old
behavior.
This reverts commit 7ca95cee15.
Revert conflicts:
- the following code was removed from _obj_states_sync_filter() in
nm-l3cfg.c because the mechanism to set temporarily-unavailable
routes was changed in 1feaf427d2
('platform: rework handling of failed routes during
nm_platform_ip_route_sync()'), and so
`os_temporary_not_available_timestamp_msec` no longer exists:
if (obj_state->os_temporary_not_available_timestamp_msec > 0) {
/* we currently try to configure this address (but failed earlier).
* Definitely retry. */
return TRUE;
}
The next commit will revert 7ca95cee15 ('platform: always
reconfigure IP routes even if removed externally'), so that we don't
re-add routes removed externally. After that, we still need the
force-commit flag to indicate which routes must be forcibly added even
if removed externally.
This reverts commit 7fa63c23b4.
Move the warning about the presence of ifcfg-rh profiles from the
plugin to NMSettings. In this way, it will be easier to implement the
migration option in the next commit.
When activating a port connection it will require the controller
connection is active or a valid controller device candidate is available
for activation.
One of the conditions we consider for a controller device to be a valid
candidate for the connection is that it is not active, therefore we
should also consider as valid a device that is currently deactivating.
Otherwise, we could fail during the port activation just because the
deactivation of the controller device candidate didn't finish yet.
https://bugzilla.redhat.com/show_bug.cgi?id=2125615https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1693
Kernel's dev_valid_name() calls isspace(), which also rejects '\v'
and '\240'.
As this tightens the check, the change can break code that partly worked
before. It surely didn't work to the point, where an interface with such
name could be created in kernel.
# ip link add name $'foo\240bar' type dummy
RTNETLINK answers: Invalid argument
If --offline and --ask were used at the same time, and endless loop
showing the readline's prompt but without waiting for user's input
happened.
This was because when using --offline, all arguments are parsed and
resolved before running the g_main_loop. In nmc_readline_helper it was
checked that the main loop is running, so if g_main_loop_quit is called
we can stop waiting for user's input.
Fix this bug by continue polling for user input if the main loop is
running or if we are in offline mode. Cancelling the user input is
still possible both in normal and offline mode with Ctrl+C or Ctrl+D.
Added a test case to verify that this still works after future changes.
This flag is a setting that changes the behaviour of nmcli, it's not
only the current state of the program, so it makes more sense to put it
in NmcConfig than in NmCli.
Furthermore, it's needed to fix a bug in next commit, too.
The `nm_device_hw_addr_reset()` should only set MAC address on NIC
with valid(>0) interface index.
The failure was found by `ovs_mtu` test of NMCI, failed to reproduce
the original problem (`ovs_mtu` test of NMCI) with 100 times retry.
And no trace log found for original test failure, hence cannot tell why
`nm_device_hw_addr_reset()` been invoked with iface index 0.
Signed-off-by: Gris Ge <fge@redhat.com>
We delete devices when the connection goes down and NetworkManager
created the device earlier.
Software devices like bond/bridge/team default to ignoring carrier.
However, when configuring them to not ignore carrier
([device].ignore-carrier), they were not deleted when deactivating the
devices.
This adjusts commit d0c2a24b71 ('device: do not remove software devices
on initial disconnected (rh #1035814)'). Note that back then there was
no check whether the device has an activation queued, so it behaved
differently then.
When the software device enters the UNAVAILABLE state from UNMANAGED,
during cleanup we shouldn't delete the link.
Co-Authored-By: Beniamino Galvani <bgalvani@redhat.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1686
When matching two connections one might be using UUID and the other one
could be using interface-name for the controller property. When
recovering from a fresh start NM does not have any context and when
generating a connection we are using UUID as the controller.
It is always hard to guess what is the right candidate to pick but at
least something NM can do is checking if the UUID matches a connection
with the same controller interface-name. If there are no other
conflicts, then we can assume that is a good canditate to activate.
This is a follow up to `dc254f90e2b306700a0b81f7194e9b0438c62f4c`.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1684
The default behavior is not to rename profiles. I guess, that makes
sense, as renaming a file when changing the "connection.id" could break
users who rely on the name.
My use case is the following. When I connect a Wi-Fi hotspot I use
`nmcli device wifi connect $SSID`, which -- as expected -- persists the
profile to "/etc/NetworkManager/system-connections/$SSID.nmconnection".
Later, I always update the profile's name to "w_$SSID" so I can see on
the name that this is wireless profile. I also want the filename to
reflect that change of name.
Add a configuration option for that. All the infrastructure
("force_rename" parameter) already exists.
There was already a force_rename argument to nms_keyfile_writer_connection(), which
-- if TRUE -- means to always rename the file, if it exists.
What we also want, is to follow the change of a connection.id. So we don't want
to force a rename, if we already use the preferred name, but we also want to rename
otherwise.
Extend the boolean "force_rename" argument to a NMTernary, where NM_TERNARY_DEFAULT
now means to follow the preferred name.
"force_rename" parameter was not used previously, but it also was broken.
Fix it. We need to create a new NMSettingsStorage instance when the filename
changes, as the storage's filename is immutable.
No bad effects so far, it was unused.
But as it wasn't used, also no longer set the update_reason flag
NM_SETTINGS_CONNECTION_UPDATE_REASON_FORCE_RENAME. We didn't have the
force-rename behavior so far. This makes the flag totally unused, and
maybe should be dropped. It's kept for now, if only to show what could
be done.
We have some interal code that is only used to expose functionality for
the tests. Those functions should be easily distinguishable from code
that is used by the "real" code. Give a "nmtst" prefix. Rename
nms_keyfile_writer_test_connection() to nmtst_keyfile_writer_test_connection().
When user are changing SR-IOV VF settings for options like `max-tx-rate`
which some hardware not supported yet, the failure of this VF will fail
the whole activation, then the SR-IOV will be disabled means all the VFs
will be deleted.
Deleting VFs might break network connectivity and this collateral
damage of VF option failure is not acceptable for OpenShift use cases
even they have checkpoint protection.
This patch only log warn message on failure of VF options and will not
fail the activation.
NetworkManager also ignore MTU failure during activation, I believe this
fit into the same assumption.
User case reference: https://bugzilla.redhat.com/show_bug.cgi?id=2210164
Signed-off-by: Gris Ge <fge@redhat.com>
Fail to save a connection with a 'link' setting instead of just
ignoring it. Now:
$ nmcli connection add type ethernet ifname foobar
Connection 'ethernet-foobar' (c3f6f067-e1d5-4bb1-8d67-e09109253a79) successfully added.
$ nmcli connection modify ethernet-foobar link.tx-queue-length 1234
Error: Failed to modify connection 'ethernet-foobar': failed to update connection: The ifcfg-rh plugin doesn't support setting 'link'. If you are modifying an existing connection profile saved in ifcfg-rh format, please migrate the connection to keyfile using 'nmcli connection migrate c3f6f067-e1d5-4bb1-8d67-e09109253a79' or via the Update2() D-Bus API and try again.
$ nmcli connection migrate c3f6f067-e1d5-4bb1-8d67-e09109253a79
Connection 'ethernet-foobar' (c3f6f067-e1d5-4bb1-8d67-e09109253a79) successfully migrated.
$ nmcli connection modify ethernet-foobar link.tx-queue-length 1234
$
Fixes: 39bfcf7aab ('all: add "link" setting')
The ifcfg-rh plugin is now deprecated and in bugfixes-only mode. When
users try to set a property that is not supported by the plugin, we
need to report an error.
Add an helper function to set such error. Also, introduce a new error
code so that the situation can be detected and dealt with
programmatically.
