Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Bridge Interfaces #1553

Closed
wants to merge 4 commits into from
Closed

Conversation

rmetrich
Copy link
Contributor

Bridge interfaces are widely used. This patch enables configurations as
shown below:

  1. Bridge over simple Ethernet
  2. Bridge over Bond
  3. Bridger over Vlan interface

Bridges require the 'brctl' utility. This patch automatically includes
the utility upon need.
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.

Signed-off-by: Renaud Métrich rmetrich@redhat.com

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

Bridges require the 'brctl' utility. This patch automatically includes
the utility upon need.
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.

Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
@gozora
Copy link
Member

gozora commented Oct 27, 2017

Hello @rmetrich
First and foremost thanks for this patch!

I have one question though, imagine that original system was setup without bridge-utils (containing brctl) and iproute2 was used instead (for whatever reason).
Do we want to force users to install additional packages despite they are not really needed for restore?

V.

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function bridge_handling should be added to lib/network-functions.sh instead of keeping it in script 310_network_devices.sh.
The content of the pull request seems correct, but I cannot test it.

@rmetrich
Copy link
Contributor Author

@gozora You are right, I've rewritten the code to use iproute2 only.
@gdha I don't understand your point, the bridge_handling function is used to create the bridge commands in the 60-network-devices.sh generated script.

@gozora
Copy link
Member

gozora commented Oct 30, 2017

Hello @rmetrich,

Just one more remark, bridge configuration requires bridge.ko kernel module, which I'm afraid is not bundled into ReaR recovery system by default. So you might consider using MODULES (or similar) configuration option somewhere in your code.

V.

@rmetrich
Copy link
Contributor Author

@gozora thanks, good point. Was already included in my kernel but who knows ...

@rmetrich
Copy link
Contributor Author

@gdha hello, please advise regarding the bridge_handling function: I don't understand your point, the bridge_handling function is used to create the bridge commands in the 60-network-devices.sh generated script.

@gdha
Copy link
Member

gdha commented Oct 31, 2017

@rmetrich What I meant with the bridge_handling function is to move the function code itself to a library script (e.g. the lib/network-functions.sh) instead of keeping it inside script 310_network_devices.sh (pure cosmetic and easier to read). Functions should never (well almost never) be part of a script. We have a special location for it under lib/ directory.
Hopefully I expressed myself better then before?

@gdha gdha added this to the ReaR v2.3 milestone Oct 31, 2017
@gdha gdha added the enhancement Adaptions and new features label Oct 31, 2017
@gdha gdha assigned gdha and gozora Oct 31, 2017
@rmetrich
Copy link
Contributor Author

@gdha hmm, here the function adds to generated script ($network_devices_setup_script), same as function vlan_setup and bond_setup, so should likely remain local.

@gdha
Copy link
Member

gdha commented Oct 31, 2017

@rmetrich OK then - keep it together if you prefer that.

@rmetrich
Copy link
Contributor Author

rmetrich commented Nov 3, 2017

@gdha Is there something special required from me? Such as squashing the commits?

@jsmeix
Copy link
Member

jsmeix commented Nov 6, 2017

A side note regarding functions that are used only in one script:

I prefer to keep local stuff local so that I prefer to have
such functions defined only in the script where needed
and not globally defined.

In particular because such functions are often a bit
"quick and dirty" which is perfectly o.k. for a specific
script-local function that is not intended to be usable
in whatever generic ways by arbitrary scripts.

Because bash does not support

local function ...

such script-local function should be better 'unset -f'
when leaving the script (also care about 'return'), e.g. see
usr/share/rear/build/GNU/Linux/390_copy_binaries_libraries.sh
and
usr/share/rear/build/GNU/Linux/400_copy_modules.sh

@rmetrich
Copy link
Contributor Author

rmetrich commented Nov 6, 2017

@jsmeix Sounds reasonable. Added the unset for all local functions in 310_network_devices.sh

@rmetrich
Copy link
Contributor Author

rmetrich commented Nov 8, 2017

I'm not satisfied with my code, specially the bridges purely virtual (e.g. libvirt) get configured, which is useless.

@jsmeix
Copy link
Member

jsmeix commented Nov 8, 2017

Superseded by #1570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants