Improve ReaR network migration #1510

schabrolles · 2017-09-21T15:01:36Z

The objective of this PR is to update the network migration process in ReaR.

Before this PR, network migration was based on udev rules files to detect and update mac or interface name.

A) The problem is that newer Linux distro (rhel7, ubuntu) does not automatically update anymore the udev network rule files.

This prevent ReaR :

to propose new interface in case of migration
to rename interface and properly activate network in recovery mode
to perform Network Migration on the restored image.

=> Migration is also currently impossible with biosdevname network interface name format (which is the new default for ubuntu/redhat.)

B) Network parameter migration (ip/routes via /etc/rear/mapping/ip_addresses or /etc/rear/mapping/routes) are only compatible with RedHat/Suse sysconfig file format. (not debian/ubuntu)

This PR propose a migration compatible with latest RH / ubuntu (not based in udev rules but biosdevname) while keeping compatibility with udev rules for older distro.

always propose new interface (even when udev rules is not used or empty)
keep udev rules renaming as default (for compatibility)
perform a direct migration of network system-setup scripts (only if udev migration cannot be done.) => to activate network in rescue mode
Allow a migration of debian/ubuntu network configuration files (currently only redhat/suse)

Tested in migration on POWER with rhel 7.3 / suse 11 / suse 12 / ubuntu 16.04 (with and without net.ifnames=0)

jsmeix

"Tested ... with ... suse 11 / suse 12"
means I just trust you.

jsmeix · 2017-09-22T09:12:02Z

Only for the fun of it:
Perhaps the initial comment in
finalize/GNU/Linux/320_migrate_network_configuration_files.sh
is no longer fully right:
"rewrite all the network configuration files for SUSE LINUX"
;-)

jsmeix · 2017-09-22T09:23:23Z

usr/share/rear/finalize/GNU/Linux/320_migrate_network_configuration_files.sh

@@ -3,12 +3,11 @@

 # because the bash option nullglob is set in rear (see usr/sbin/rear)
 # PATCH_FILES is empty if nothing matches $TARGET_FS_ROOT/etc/sysconfig/*/ifcfg-*
-PATCH_FILES=( $TARGET_FS_ROOT/etc/sysconfig/*/ifcfg-* )
+PATCH_FILES=( $TARGET_FS_ROOT/etc/sysconfig/*/ifcfg-* $TARGET_FS_ROOT/etc/network/inter[f]aces $TARGET_FS_ROOT/etc/network/interfaces.d/* )


Why do you use globbing for a single character as in inter[f]aces?
I think globbing for a single character is useless because
it matches only this single character
(it is neither case-insensitive nor optional):

# touch /tmp/bar # touch /tmp/bAr # touch /tmp/br # echo /tmp/b[a]r /tmp/bar # ( shopt -s nullglob extglob ; echo /tmp/b[a]r ) /tmp/bar

I thought it also permit to take only the file if it exists on the server.
But maybe I misunderstood the comment:

# because the bash option nullglob is set in rear (see usr/sbin/rear) # PATCH_FILES is empty if nothing matches $TARGET_FS_ROOT/etc/sysconfig/*/ifcfg-*

I just don't want to add /etc/network/interfaces to the PATCH_FILES array if it does not exists (like suse/redhat)

Perfect!
Thanks for the explanation.
You are right:

# arr=( foo /etc/network/inter[f]aces bar ) ; echo ${arr[*]} foo /etc/network/inter[f]aces bar # ls /etc/network/interfaces ls: cannot access /etc/network/interfaces: No such file or directory # ( shopt -s nullglob extglob ; arr=( foo /etc/network/inter[f]aces bar ) ; echo ${arr[*]} ) foo bar

Perhaps an additional comment why
that "artificial" looking globbing is used here?

schabrolles · 2017-09-22T09:49:32Z

@jsmeix thanks for the quick review and trust :-) ... But I will wait the other to test this one with vagrant to cross check and hunt any regression / issues.
Especially @gdha as I think it should solves some of the issues he had with ubuntu and biosdevames (#1400, #951 and #1020)

jsmeix · 2017-09-22T10:04:41Z

@schabrolles
don't overestimate my trust here ;-)
Basically all I can do is to "bindly trust" you here
because I am mostly clueless in this area
so better do not trust my review here.

jsmeix · 2017-09-22T11:34:22Z

@schabrolles
because you added your functions to lib/network-functions.sh
I had a first look at that script (I never had something to do with it)
and I saw its strange looking functions (some of them even
just call 'exit' which is totally unusual in "normal" ReaR scripts).

Now I am wondering (in particular after reading its initial comment)
if lib/network-functions.sh was ever meant to be sourced
by usr/sbin/rear to make its strange functions avaiable
to all ReaR scripts?

In current (i.e. without this pull request) code
lib/network-functions.sh is explicitly sourced by
skel/default/bin/dhclient-script and
skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh
which matches the initial comment in lib/network-functions.sh

Currently the only "normal" ReaR script that calls
a function from lib/network-functions.sh is
finalize/GNU/Linux/320_migrate_network_configuration_files.sh
which calls only the prefix2netmask function and that call
was added by @gdha via
8472c46

From my current point of view lib/network-functions.sh
was never meant to be sourced by usr/sbin/rear to have
its functions available in all "normal" ReaR scripts.

From my current point of view lib/network-functions.sh
is only meant to be sourced and used to setup dhclient
during recovery system initialization via the 'skel' scripts.

Simply put:
I think some general cleanup is needed here.
I think this should be done as a separated issue:
I think network-functions.sh should be renamed into
somethinmg like dhclient-setup-functions.sh
and moved away from usr/share/rear/lib
probably into usr/share/rear/skel and the two 'skel' scritps
that source it should be adapted.

@schabrolles
accordingly I think that your new networking functions
should be implemented in a new separated file
so that later we can do the dhclient setup functions
cleanup.

The problem is that lib/network-functions.sh is the exact right name
for generic networking functions for normal ReaR scripts
so that the current lib/network-functions.sh should be right now
renamed e.g. into dhclient-setup-functions.sh but then the two
'skel' scripts need to be adapted right now which means
that general cleanup needs to be done right now :-(

@schabrolles
what do you think?

jsmeix · 2017-09-22T12:14:56Z

@gdha
I see now that lib/network-functions.sh was added via your
38d5bd2

Can you explain if that strange looking functions therein
are really meant to be always available in all normal
ReaR scripts?

What I mean is:
None of the functions in lib/network-functions.sh
uses any of the standard ReaR functions that we have
but instead those functions in lib/network-functions.sh
implement all on their own so that I think those functions
basically come from a foreign world and live in that world
which makes them not well suitable to be used by normal
ReaR scripts.

jsmeix · 2017-09-22T12:19:47Z

@schabrolles
I really dislike it when a specific issue like this one
out of a sudden "explodes" into a monstrous task
to overhaul major parts of ReaR.
I do want to Keep Separated Issues Separated (KSIS).
Therefore it should be best when you first merge
this pull request as soon as possible.
Afterwards we can do the lib/network-functions.sh
cleanup as time permits.

jsmeix · 2017-09-22T12:30:21Z

I missed two functions in lib/network-functions.sh
that are currently called by a normal ReaR script:
is_ip and get_ip_from_fqdn are called in lib/bootloader-functions.sh
which was implemented by @schabrolles via
ef02562

schabrolles · 2017-09-25T13:27:38Z

@jsmeix
I've source lib/network-functions.sh in the skel 00-functions.sh to have access to get_device_by_hwaddr() function in 55-migrate-network-devices.sh (also part of skel).
=> The purpose is to have to IP set to the good interface (recognized by its IP) during boot in rescue mode. (when /etc/scripts/system-setup and /etc/scripts/system-setup.d/* scripts are executed.)

I'm not sure that usr/sbin/rear will execute /etc/scripts/system-setup.d/00-functions.sh.... But I may be wrong.

jsmeix · 2017-09-26T09:22:31Z

@schabrolles
just proceed here as you like and as it works for you
because as I wrote above we should do any
"grand cleanup" via a separated issue/pull-request
later at any time as time permits because currently
nothing goes wrong so that there is no need to do
such a "grand cleanup" right now.

gdha · 2017-09-26T09:24:44Z

@schabrolles @jsmeix I agree that network-functions.sh is collection I grabbed from an old RHEL system to get the job done as easy as possible (at that time - many, many years ago).
I agree we better split it apart (network piece required for DHCP in skel and network functions which are required by main rear script). That will make it easier afterwards to clean it up.
I also agree to merge this PR asap to get things going as the PR is a good thing to have.
@schabrolles rear will not execute /etc/scripts/system-setup.d/00-functions.sh or source it as it is part of the booting up part of the rescue image (it is sourced by /etc/scripts/system-setup script)

schabrolles · 2017-09-26T09:39:04Z

@gdha thanks for your review.
Did you try it on your ubuntu 16.04 systems (where you have biosdevname) ? It solves the issues on my side; I just wanted to know if it solves yours.

jsmeix · 2017-09-26T11:15:53Z

Regarding the lib/network-functions.sh cleanup I submitted
#1517

gdha · 2017-09-26T15:54:47Z

@schabrolles No, I did not test it yet with ubuntu16 yet as I was struggling with ubuntu14 provisioning (just got it working without errors). Pff - what people need to do to earn some money...
I would say just merge it and I'll test it upcoming Friday with Ubuntu16 (if I did not break the Ubuntu16 provisioning now as everything is now one cookbook).

schabrolles added 7 commits September 19, 2017 10:31

Allow migration of inet not listed udev-rules

98b8b23

Always get new inet name from mac

d19af85

source network-functions.sh

b01c8a9

Migrate Debian network interfaces files

a7797e4

fix device renaming

a5a999a

Update comments

137c2bd

Remove unused TOTAL_MACS variable

13971b9

schabrolles added the enhancement Adaptions and new features label Sep 21, 2017

schabrolles self-assigned this Sep 21, 2017

schabrolles requested review from schlomo, gdha, jsmeix and gozora September 21, 2017 15:01

schabrolles changed the title ~~Update ReaR network migration~~ Improve ReaR network migration Sep 21, 2017

jsmeix approved these changes Sep 22, 2017

View reviewed changes

jsmeix reviewed Sep 22, 2017

View reviewed changes

Update comments

137449d

jsmeix added this to the ReaR v2.3 milestone Sep 22, 2017

gdha approved these changes Sep 26, 2017

View reviewed changes

jsmeix mentioned this pull request Sep 26, 2017

Split up lib/network-functions.sh into DHCP setup and general ReaR functions #1517

Closed

schabrolles merged commit f242a1f into rear:master Sep 26, 2017

schabrolles deleted the debian_network_migration branch September 27, 2017 08:00

jsmeix added the fixed / solved / done label Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ReaR network migration #1510

Improve ReaR network migration #1510

schabrolles commented Sep 21, 2017 •

edited

jsmeix left a comment

jsmeix commented Sep 22, 2017

jsmeix Sep 22, 2017 •

edited

schabrolles Sep 22, 2017

jsmeix Sep 22, 2017

jsmeix Sep 22, 2017 •

edited

schabrolles Sep 22, 2017

schabrolles commented Sep 22, 2017

jsmeix commented Sep 22, 2017

jsmeix commented Sep 22, 2017

jsmeix commented Sep 22, 2017 •

edited

jsmeix commented Sep 22, 2017 •

edited

jsmeix commented Sep 22, 2017

schabrolles commented Sep 25, 2017

jsmeix commented Sep 26, 2017

gdha commented Sep 26, 2017

schabrolles commented Sep 26, 2017

jsmeix commented Sep 26, 2017

gdha commented Sep 26, 2017

Improve ReaR network migration #1510

Improve ReaR network migration #1510

Conversation

schabrolles commented Sep 21, 2017 • edited

jsmeix left a comment

Choose a reason for hiding this comment

jsmeix commented Sep 22, 2017

jsmeix Sep 22, 2017 • edited

Choose a reason for hiding this comment

schabrolles Sep 22, 2017

Choose a reason for hiding this comment

jsmeix Sep 22, 2017

Choose a reason for hiding this comment

jsmeix Sep 22, 2017 • edited

Choose a reason for hiding this comment

schabrolles Sep 22, 2017

Choose a reason for hiding this comment

schabrolles commented Sep 22, 2017

jsmeix commented Sep 22, 2017

jsmeix commented Sep 22, 2017

jsmeix commented Sep 22, 2017 • edited

jsmeix commented Sep 22, 2017 • edited

jsmeix commented Sep 22, 2017

schabrolles commented Sep 25, 2017

jsmeix commented Sep 26, 2017

gdha commented Sep 26, 2017

schabrolles commented Sep 26, 2017

jsmeix commented Sep 26, 2017

gdha commented Sep 26, 2017

schabrolles commented Sep 21, 2017 •

edited

jsmeix Sep 22, 2017 •

edited

jsmeix Sep 22, 2017 •

edited

jsmeix commented Sep 22, 2017 •

edited

jsmeix commented Sep 22, 2017 •

edited