#1570 PR `merged`: Add support for Bridge Interfaces¶

Labels: enhancement, fixed / solved / done

rmetrich opened issue at 2017-11-08 14:23:¶

This is a respin of the previous PR I closed because it was suboptimal regarding bridges on virtual interfaces (such as libvirt bridges used with KVM).

Add support for Bridge Interfaces

Bridge interfaces are widely used. This patch enables configurations as
shown below:
1. Bridge over simple Ethernet
2. Bridge over Bond
3. Bridge over Vlan interface

Usually, virtual interfaces are skipped, but for Bridges to work, we
consider Bridges as physical interfaces, because the Bridge interface
holds the IP address, not the physical interface attached to the Bridge.

Commit 0c0ee4a032c1b44135a07e28e89c3da01dde99d5 is identical to the one that has been approved.
Commit ff22996de6ea7035eec4e829053b1ad80a9f390d fixes the support of bridge when using Simplified bonding.
Commit 0958e2aa7a02552e299dd1169c99066e225874c1 limits the bridge interfaces to the ones linked to physical interfaces somehow (purely virtual bridges are useless when recovering).

jsmeix commented at 2017-11-08 15:37:¶

I set same assignees and reviewers here
as in https://github.com/rear/rear/pull/1553

jsmeix commented at 2017-11-17 08:54:¶

@gdha
could you have a look at the code and if it looks o.k.
we could merge it to have it in ReaR 2.3.

gdha commented at 2017-11-17 09:07:¶

@rmetrich @jsmeix Is this pull request still relevant in the sense of PR https://github.com/rear/rear/pull/1574?

rmetrich commented at 2017-11-17 09:16:¶

@gdha yes, please integrate in rear2.3, PR #1574 will be rear2.4 and we already hit such issues

rmetrich commented at 2017-11-28 15:41:¶

Hello @gozora @gdha
I'd like to inform you that the code doesn't work on RHEL6, because it makes use of "ip" commands not understood, such as "ip link add name XXX master YYY".
This is due to @gozora making me remove Commit https://github.com/rear/rear/pull/1553/commits/d675166f3ca92f525d916abfc25dbcd1d7126fe5 ;-)
(use of brctl).
I'll hence reintroduce brctl for older "ip" releases.

QUESTION: Should I submit a new PR or can I reuse this one?

jsmeix commented at 2017-11-29 08:40:¶

@rmetrich
I never tried to re-use an already merged pull request
therefore I would recommend to be on the safe side and
do a subsequent change for an already merged pull request
via a new separated pull request.

Regarding
https://github.com/rear/rear/pull/1553#issuecomment-340003156
and
https://github.com/rear/rear/commit/d675166f3ca92f525d916abfc25dbcd1d7126fe5

if CONDITION ; then
    REQUIRED_PROGS=( "${REQUIRED_PROGS[@]}" "brctl" )
fi

I agree with @gozora that we should aviod to force users
to install 'brctl' if that is not really mandatory.

For example on my SLES11 system I have 'brctl' by default
but on my SLES12 system I do no longer have 'brctl' by default
(I assume because nowadays 'ip' should be preferred).

Accordingly - if possible - the code in ReaR should at runtime
decide if 'brctl' should be called or the 'ip' command.

For example a more conservative approach
via a simple testing if 'brctl' is executable like

# Use 'brctl' if executable (even if also 'ip' is executable)
# because 'brctl' should work backward compatible
# cf. https://github.com/rear/rear/pull/1570
if has_binary brctl ; then
    brctl ...
else
    ip ...
fi

Or an approach that perfers the modern way and only
falls back to the old way if the modern way fails like

# First try 'ip' because 'ip' is the future-proof way
# and fall back to 'brctl' if 'ip' failed
# cf. https://github.com/rear/rear/pull/1570
ip ... || brctl ... || BugError "Neither 'ip' nor 'brctl' worked"

cf. "Dirty hacks welcome" at
https://github.com/rear/rear/wiki/Coding-Style

Additionally you should get 'brctl' into the recovery system
in a non-mandatory way (i.e. only if 'brctl' is installed on the
original system) via:

# Have 'brctl' in the recovery system (if available)
# because 'brctl' is needed on older systems where
# things like "ip link add name XXX master YYY" are
# not supported, cf. https://github.com/rear/rear/pull/1570
if CONDITION ; then
    PROGS=( "${PROGS[@]}" "brctl" )
fi

[Export of Github issue for rear/rear.]

#1570 PR merged: Add support for Bridge Interfaces¶