Migrate MAC addresses and interface names in NetworkManager keyfiles during network configuration migration #3179
Conversation
See https://fedoramagazine.org/converting-networkmanager-from-ifcfg-to-keyfiles/ for more details on NetworkManager keyfiles. Only MAC addresses and interface names are migrated for now. TODO: migrate also IP addresses and routes.
Otherwise patching fails on files with spaces in them, often used by NetworkManager: Failed to rewrite MAC addresses and network interfaces in /mnt/local/etc/NetworkManager/system-connections/ZSSK Failed to rewrite MAC addresses and network interfaces in WIFI.nmconnection (the original file is named 'ZSSK WIFI.nmconnection').
| network_config_file="$( valid_restored_file_for_patching "$restored_file" )" || continue | ||
| network_config_files+=( $network_config_file ) | ||
| network_config_files+=( "$network_config_file" ) |
There was a problem hiding this comment.
@pcahyna
only out of curiosity:
Are there network config files with $IFS characters
or other problematic characters in their file names
or did you add quoting only to be on the safe side?
There was a problem hiding this comment.
they are, see the example in the commit message of fe60e55.
Such filenames do not occur for ifcfg files, but do with keyfiles - apparently the convention for keyfiles is to name them simply according to the ESSID of the wireless connection verbatim, while ifcfg file names are normalized to contain only "usual" characters.
There was a problem hiding this comment.
Since a longer time I am waiting for basic system files
with $IFS characters or other problematic characters
in their file names.
Hooray :-( now I know about the first real-world example.
The current ReaR code will break at very many places
when basic system values contain $IFS characters
or other problematic characters.
Basically this means all usage of strings to store
more than a single value must be replaced with arrays.
Doable but a huge pile of legwork.
It also means that such values must be properly marked
as as single value in user output messages,
e.g. instead of
File not found: this that
where it is not clear if this that is one or two files
better
File not found: 'this that'
which looks "interesting" when the value contains '
# files=( "Mike's letter" 'Say "Hello"' "File not found" )
# for f in "${files[@]}" ; do echo "File not found: '$f'" ; done
File not found: 'Mike's letter'
File not found: 'Say "Hello"'
File not found: 'File not found'
Sigh!
There was a problem hiding this comment.
Basically this means all usage of strings to store more than a single value must be replaced with arrays. Doable but a huge pile of legwork.
yes, we should gradually change the code this way. It will help with consistency anyway if the variable is meant to be user-settable, because we have a lot of array configuration variables already and it is simpler for the user to know that variables that store multiple values are always arrays than to remember when the variable is an array and when it is a single string of space-separated values.
It also means that such values must be properly marked as as single value in user output messages, e.g. instead of
File not found: this thatwhere it is not clear if
this thatis one or two files betterFile not found: 'this that'
We have been already doing that AFAICT, at least that has been my experience with existing log or terminal messages - they are usually quoted this way and I tried to respect it.
which looks "interesting" when the value contains
'# files=( "Mike's letter" 'Say "Hello"' "File not found" ) # for f in "${files[@]}" ; do echo "File not found: '$f'" ; done File not found: 'Mike's letter' File not found: 'Say "Hello"' File not found: 'File not found'Sigh!
True. I don't expect this to occur often, but it is true that the "saxonic genitive" makes it more likely to happen.
There was a problem hiding this comment.
I experimented with several variable expansion flags, hoping that they would lead to quoting of such strange characters, but I have not been successful:
$ network_config_files=( "Pavel's WIFI" )
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in '$network_config_file'"; done
Wrote new MAC addresses and network interfaces in 'Pavel's WIFI'
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in '${network_config_file:Q}'"; done
Wrote new MAC addresses and network interfaces in 'Pavel's WIFI'
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in '${network_config_file:K}'"; done
Wrote new MAC addresses and network interfaces in 'Pavel's WIFI'
I want the code being correct and safe for any possible input, but in the case of code used purely for display / informational purposes I am willing to relax this requirement: I don't want to spend too much time and make the code more complicated / less readable just to get a better formatting of log messages in corner cases.
There was a problem hiding this comment.
@pcahyna
I didn't mean my
looks "interesting" when the value contains '
part that we should implement sophisticated code
to make such corner cases look nicer in ReaR.
Simple enclosing values with ' in user messages like
LogPrint "... '$variable' ..."
is sufficient.
It is also in general useful to clearly show
when $variable is empty or blanks.
Forthermore simple ' is predictable what the output
will look like from plain looking at the code.
Because at least I cannot find right now where
${var:Q} and ${var:K} are documented
(searching for Q or K is not very helpful ;-)
So far up to now I only found ${var@Q} and ${var@K} in
https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
There was a problem hiding this comment.
@jsmeix oops! I meant the stuff with @, but I used :! Now it looks better:
$ network_config_files=( "Pavel's WIFI" )
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in '${network_config_file@Q}'"; done
Wrote new MAC addresses and network interfaces in ''Pavel'\''s WIFI''
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in '${network_config_file@K}'"; done
Wrote new MAC addresses and network interfaces in ''Pavel'\''s WIFI''
There was a problem hiding this comment.
One should drop the single quotes then, as apparently @Q produces an always-quoted version:
$ network_config_files=( "Pavel's WIFI" "Pavels WIFI" "WIFI" )
$ for network_config_file in "${network_config_files[@]}" ; do echo "Wrote new MAC addresses and network interfaces in ${network_config_file@Q}"; done
Wrote new MAC addresses and network interfaces in 'Pavel'\''s WIFI'
Wrote new MAC addresses and network interfaces in 'Pavels WIFI'
Wrote new MAC addresses and network interfaces in 'WIFI'
I would say this is simple enough to be used instead of the single quotes, but of course it has the disadvantage of diminished readability of the code.
More seriously, even in RHEL 7 this does not work (bash is too old), so I think I will drop the idea.
There was a problem hiding this comment.
For user info messages I find
... in 'Pavel'\''s WIFI'
much harder to understand what the value actually is than
... in 'Pavel's WIFI'
where I think the human's brain "fuzzy logic"
does "the right magic" to deduce that
the correct meaning is Pavel's WIFI
And 'Pavel'\''s WIFI' may even confuse some users
when something goes wrong related to Pavel's WIFI
that the reason is that Pavel'\''s WIFI is not
the expected Pavel's WIFI - e.g. think about
File not found: '/path/to/Pavel'\''s WIFI'
in contrast to
File not found: '/path/to/Pavel's WIFI'
where the latter looks more clear (at least to me)
that the file /path/to/Pavel's WIFI is meant.
There was a problem hiding this comment.
Basically the use for 'Pavel'\''s WIFI' is that you can copy and paste it to the command line and it will work without any modification, which is perhaps not that important for our purposes.
|
Regarding "TODO: migrate also IP addresses and routes." Is it worth supporting migration of IP addresses and routes? What is the use case? I suppose it is for machine cloning to several not completely identical (different network address) copies, as I don't see how it is useful for the typical use of ReaR (disaster and recovery) where you need an identical address after recovery. Would it be acceptable to remove this feature (say in ReaR 3.0)? |
|
@pcahyna My guess is similar as yours that at some time in the past Perhaps (also only a guess) the idea behind is to use ReaR On the one hand ReaR has too many (often half-baked) features On the other hand such features can be a starting point for I think the solution out of this conflict is This way we learn via somewhat enforced user feedback |
|
@pcahyna As far as I see it is those two code places that should be enhanced to |
I supposed the idea was to use ReaR for cloning, i.e. backup a machine once and then restore it to multiple instances, each one will then need a different IP address. It seems though that according to the commit message of 844d50b by @schlomo where this functionality was introduced, they had indeed rather the idea of "migrating bare metal hardware in one network environment to a VM in another network environment" in mind. |
|
Oh yes, that was the first version of migration support developed for P2V. |
It is our common practice to log and show messages with single-quoted file names like "Processed the file 'file name'" and it is especially useful with file names containing spaces.
Instead of chjecking whether the first member of the array is empty, check whether the array is actually empty.
|
|
I now pushed another change, 5ea58a7, for a proper test for an empty array. Unfortunately, the code is now untested, as I have not tested with this change. |
pcahyna
changed the title
There was a problem hiding this comment.
@pcahyna this looks good to me and I think it is a great idea.
I'd suggest to just merge it as it doesn't seem harmful.
Personally I wouldn't optimise ReaR for machine cloning use cases but rather point people to the ability to use cloud-init (or other tools) after the recovery to further customise the system, and this can be added to ReaR simply via configuration without coding.
|
Ping @rear/contributors can we merge this or should we drop this? |
Pull Request Details:
Type: Enhancement
Impact: Normal
Reference to related issue (URL):
How was this pull request tested?
Migration of Fedora Linux system to different hardware (different NICs). With this patch, the network connections are preserved and functional after recovery, even though the NICs have different names and different MAC adresses.
Description of the changes in this pull request:
Migrate NM keyfiles during network conf migration. See https://fedoramagazine.org/converting-networkmanager-from-ifcfg-to-keyfiles/ for more details on NetworkManager keyfiles.
Only MAC addresses and interface names are migrated for now.
TODO: migrate also IP addresses and routes.