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

New network script generator (60-network-devices.sh) #1574

Merged
merged 8 commits into from Jan 17, 2018

Conversation

rmetrich
Copy link
Contributor

This work is a solution for Issue #1561.

It consists in a full rewrite of the 310_network_devices.sh script generating network interfaces for use during the ReaR recovery early boot.
It also handles corner cases/odd setups that can be found from time to time, typically when the administrator uses bonding + bridges + vlans as well as teaming.
With that new code, adding new configurations will be very easy in the future.

I also added interface mapping code to the corresponding 350_routing.sh script generating routes.

In the tests/ directory, you will 2 setups that I used to verify the network generation code. It covers quite a lot of setups found.

@jsmeix jsmeix self-assigned this Nov 15, 2017
@jsmeix jsmeix requested review from gdha and gozora November 15, 2017 13:31
@jsmeix jsmeix added cleanup documentation enhancement Adaptions and new features labels Nov 15, 2017
@jsmeix jsmeix added this to the ReaR v2.4 milestone Nov 15, 2017
@jsmeix
Copy link
Member

jsmeix commented Nov 15, 2017

@rmetrich
many thanks for your substantial work!

According to
#1561 (comment)
I set it to "ReaR 2.4"

@gdha @gozora
I dared to set you as reviewers here. I would very much
appreciate it if you could have a look if there are perhaps
immediately visible possible issues with the new code.
Many thanks in advance!

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.

The change is code is quite huge, and therefore, not easy to decide if everything is ok or not.
I suppose the first tests will tell us very quickly. On first sight the code looks clean and well written with good documentation. A job well done (I should do that more often:)

@gdha
Copy link
Member

gdha commented Nov 15, 2017

The only thing that bothers me are the Unit Tests - nice to have but do they belong in the code? There are pros and contras for it. We can leave it here for the moment

@jsmeix
Copy link
Member

jsmeix commented Nov 15, 2017

@gdha
does your approval mean that from your current point of view
we could even merge it into ReaR 2.3 or should we wait in any
case and merge it directly after ReaR 2.3 was released (to get
it tested by ReaR users until ReaR 2.4 will be released).

@gdha
Copy link
Member

gdha commented Nov 15, 2017

@jsmeix I would prefer that we release 2.3 first and then do the merge as then we will have plenty of time to test the code base.

@rmetrich
Copy link
Contributor Author

@gdha I put the unit tests here just for reference.
Since network configurations can be very tricky, I believe having unit tests may help in the future, making sure nothing breaks.

@jsmeix
Copy link
Member

jsmeix commented Nov 15, 2017

I also prefer to release ReaR 2.3 first to stay on the safe side.

@gozora
Copy link
Member

gozora commented Nov 15, 2017

@rmetrich I can't tell nothing less then IMPRESSIVE! ;-)

As i'm not much of a "code reader", I'd need to test this directly by clone and run ...
I will do that over this weekend and you guys know how it went ...

V.

Copy link
Member

@gozora gozora left a comment

Choose a reason for hiding this comment

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

I've run several tests, and for me this PR works sufficiently well.

V.

@rmetrich
Copy link
Contributor Author

rmetrich commented Nov 18, 2017 via email

@gozora
Copy link
Member

gozora commented Nov 18, 2017

:-), no but I never actually used all those fancy ReaRs networking options and if my network is not up properly, I just configure it manually ;-).
I've found some problems with 320_migrate_network_configuration_files.sh where I got following error:

2017-11-18 11:21:39.162298984 SED_SCRIPT: ';s/08:00:27:2a:4f:0a/08:00:27:76:fa:e7/g;s/eth0/eth0
eth1
bond0
vlan100@bond0/g;s/08:00:27:2A:4F:0A/08:00:27:76:FA:E7/g;s/Eth0/eth0
eth1
bond0
vlan100@bond0/g;s/08:00:27:e8:3e:07/08:00:27:0f:00:78/g;s/08:00:27:E8:3E:07/08:00:27:0F:00:78/g'
sed: -e expression #1, char 52: unterminated `s' command

I'm pretty sure that this is not related to your work though.

I've tested your PR in vlan over bonded device and it worked well, and as I did not had time to test it much further I've choose word "Sufficiently" hope you don't mind ;-)

V.

@hpannenb
Copy link
Contributor

@rmetrich At a RHEL6.5 server with several bridges and bonding interfaces here the ip link command mentions an

bridge: unkown command "stp_state"?

The version in use is "ip utility, iproute2-ss091226".
During recovery the created script /etc/scripts/system-setup.d/60-network-devices.sh thus will not create the bridges here.

@rmetrich
Copy link
Contributor Author

@hpannenb Thanks for reporting. I'll test on RHEL6 and adjust.

@rmetrich rmetrich changed the title New network script generator (60-network-devices.sh) WIP: New network script generator (60-network-devices.sh) Nov 29, 2017
@rmetrich
Copy link
Contributor Author

@hpannenb Please try the latest code.
I reworked many things for "ancient" releases.

@rmetrich rmetrich changed the title WIP: New network script generator (60-network-devices.sh) New network script generator (60-network-devices.sh) Nov 30, 2017
@hpannenb
Copy link
Contributor

@rmetrich I tested with Your pull request #1605 and on the RHEL6 Server the interfaces, bonding and bridges were created as supposed to. Thanks for Your coding effort.

@rmetrich
Copy link
Contributor Author

@hpannenb Thanks for feedback. I initially didn't test on RHEL6 because we (Red Hat) do not ship ReaR 2.x on RHEL6 ...

@gdha
Copy link
Member

gdha commented Jan 9, 2018

@rmetrich Could you fix the conflicts then we are able to merge it without issues?

…eam' and 'physdev'

- Removed current teaming code
- Have routes use interface mapping
- Added explanations
- Adapt search for lower interfaces when no 'lower_*' symlink exists (RHEL6)
- Always set device for bonding using 'echo "+dev" > .../bond/bonding/slaves'
- Detect support for 'master' in 'ip link' (no support in RHEL6)
- Replace 'readlink -f' by 'readlink -e'
- Use 'brctl' when 'ip link' doesn't support 'bridge' type (RHEL6)
- Use function 'resolve' to wrap 'readlink' when it doesn't support more than 1 filename (RHEL6)
@rmetrich
Copy link
Contributor Author

rmetrich commented Jan 9, 2018

@gdha Done!

@gdha
Copy link
Member

gdha commented Jan 16, 2018

@jsmeix can we merge it?

@jsmeix
Copy link
Member

jsmeix commented Jan 17, 2018

@gdha
yes, we can merge it.

@jsmeix jsmeix merged commit 0293785 into rear:master Jan 17, 2018
@jsmeix
Copy link
Member

jsmeix commented Jan 17, 2018

@rmetrich
many thanks for your substantial contribution to ReaR!

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

Successfully merging this pull request may close these issues.

None yet

6 participants