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

Revert "Fix service network restart on RHEL-7 / Fedora" #8148

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

mikefaille
Copy link
Contributor

@mikefaille mikefaille commented Dec 22, 2016

Fix theses bugs :
#8142 #8096

For RHEL7, Centos7 and Fedora 25 /etc/init.d/network restart already shutdown interfaces and restart NM if needed.

In start() :

        if [ "$(LANG=C nmcli -t --fields running general status 2>/dev/null)" = "running" ]; then
                nmcli connection reload
        fi

In stop() :

       for i in $vpninterfaces $xdslinterfaces $bridgeinterfaces $vlaninterfaces $remaining; do
                unset DEVICE TYPE
                (. ./ifcfg-$i
                if [ -z "$DEVICE" ] ; then DEVICE="$i"; fi

                if ! check_device_down $DEVICE; then
                   action $"Shutting down interface $i: " ./ifdown $i boot
                   [ $? -ne 0 ] && rc=1
                fi
                )
        done

Where $remaining include all "others" interfaces including eth*

It work for NM_CONTROLLED=X where X = yes or no

This reverts commit 166d10d.

`/etc/init.d/network restart` already restart NM and shutdown interfaces.

In start() :
```
        if [ "$(LANG=C nmcli -t --fields running general status 2>/dev/null)" = "running" ]; then
                nmcli connection reload
        fi

```

In stop() :

```
       for i in $vpninterfaces $xdslinterfaces $bridgeinterfaces $vlaninterfaces $remaining; do
                unset DEVICE TYPE
                (. ./ifcfg-$i
                if [ -z "$DEVICE" ] ; then DEVICE="$i"; fi

                if ! check_device_down $DEVICE; then
                   action $"Shutting down interface $i: " ./ifdown $i boot
                   [ $? -ne 0 ] && rc=1
                fi
                )
        done
```
Where $remaining include all "others" interfaces including eth*

This reverts commit 166d10d.
@karlkfi
Copy link

karlkfi commented Dec 23, 2016

Works for me.

We still don't have an explanation of why the change this reverts was made in the first place. But that shouldn't block the fix tho, imo. Even with an explanation, that change wasn't a valid, well tested fix without side effects.

@mikefaille
Copy link
Contributor Author

mikefaille commented Dec 23, 2016

@karlkfi Here is the original pull request : #8052

@karlkfi
Copy link

karlkfi commented Dec 23, 2016

Yes, I know. I asked the same thing there and got no answer.

@mikefaille
Copy link
Contributor Author

I see. The link on original pull request could help to trace changes.

@karlkfi
Copy link

karlkfi commented Dec 23, 2016

Hmm, you're right. I don't remember seeing that link before.

I wonder if it's the same as #7923. There have been a rash of networking bugs in the last few Vagrant releases. Both issues could have been caused by one of the iface sorting bugs.

@karlkfi
Copy link

karlkfi commented Dec 23, 2016

recent network interface issues: #7844 #7858 #7988 #7935 #7668 #7876 #7458

@mikefaille
Copy link
Contributor Author

mikefaille commented Dec 23, 2016

For the iface sorting bugs. as I understand, network interfaces must be manages appropriately depending on the process owner (ex : vagrant or docker for bridge). It's why we short interfaces. Isn't ? I upgrade Nixos vagrant version from 1.8.6 to 1.8.7 to fix my own issue : NixOS/nixpkgs#20401
Maybe, upgrading vagrant from 1.8.6 to 1.8.7 could fix his issue : #7923

I don't know if all recent network bugs are related but we must accept this pull request fast because I think the root cause of network problems in vagrant 1.9.1 is the commit where I want to revert.

At least, in issues mentioned initially in my pull request we really need to fix them fast because I think it cause more damages than we think. And, vagrant's network configuration for RHEL-like distros in this pull request is clean and predictable as we can get in Suse one : https://github.com/mikefaille/vagrant/blob/96611341a96d7d19fdade5556a110b22c6add22b/plugins/guests/debian/cap/configure_networks.rb

@jairojunior
Copy link

Only saw this PR now. #8166 is also related to this.

Copy link
Member

@chrisroberts chrisroberts left a comment

Choose a reason for hiding this comment

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

@mikefaille Thanks for this PR and explanation. I'm going to merge this in favor of #8121 and revisit it at a later date, if at all. I found the same behavior as the original PR, but I am unsure of the specific box versions in use at the time and can no longer reproduce it locally. The string of predictable network interface name issues and PRs are unrelated to this issue specifically.

Cheers!

@chrisroberts chrisroberts merged commit 5004f37 into hashicorp:master Jan 5, 2017
@mikefaille
Copy link
Contributor Author

Thank you @chrisroberts for this follow up.

@alexivkin
Copy link

still broken in 1.9.1.
The actual fix is to add
/sbin/ifup '#{network[:device]}'
right after
nmcli c reload || true
in
/plugins/guests/redhat/cap/configure_networks.rb

@mikefaille
Copy link
Contributor Author

mikefaille commented Jan 17, 2017

still broken in 1.9.1. 

@alexivkin It makes sense. My fixe was based on 1.9.1. Maybe, you should wait for Vagrant 1.9.2 8e7c267 or use Vagrant 1.9.0 https://releases.hashicorp.com/vagrant/1.9.0/

@meekrosoft
Copy link

meekrosoft commented Feb 17, 2017

It seems like this is also a bug on centos/7 - should I open a new issue for this?

@mikefaille
Copy link
Contributor Author

mikefaille commented Feb 17, 2017

@meekrosoft Nope :-) Since you got the same bugs as RHEL and RHEL == Centos, I prefer to modify my issue/PR description.

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

Successfully merging this pull request may close these issues.

6 participants