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

fix vlan interface bring up on boot #44347

Merged
merged 3 commits into from Sep 2, 2018
Merged

fix vlan interface bring up on boot #44347

merged 3 commits into from Sep 2, 2018

Conversation

zhangyoufu
Copy link
Contributor

Motivation for this change

fix #28620

When the parent interface of a vlan interface is not up (yet), ip link cannot bring the vlan interface up. The vlan interface will be automatically brought up when the parent interface is up later.

when the parent interface of a vlan interface is not up (yet), ip link cannot bring the vlan interface up
the vlan interface will be automatically brought up when the parent interface is up later
fix #28620
@coretemp
Copy link
Contributor

coretemp commented Aug 2, 2018

I am against merging this, because it is a hack, not a solution.

@zhangyoufu
Copy link
Contributor Author

Yes, it is a hack. And there're || true hacks all around in network-interfaces-scripted.nix.

In my opinion, a solution would require moving to networkd or a sophisticated refactoring of scripted networking, which is not possible in near future.

Consider #28620 has a severity: blocker label, one more hack inside scripted networking should be acceptable.

@coretemp
Copy link
Contributor

coretemp commented Aug 3, 2018

This hack doesn't even explain why it's doing what it is doing in a comment in the source code. As such anyone who ever wants to implement a better method can only break everything else or hunt down GitHub (if that still is being used by the time someone gets around to implementing this).

Just because the code is bad doesn't mean we need to make it worse.

I am not a code owner, but that's how I -- as one of its users -- think about it.

@infinisil
Copy link
Member

Is it possible to only run that command on a condition? I'd also prefer to to have a random unexplained || true, and only running it on a checked condition automatically documents it.

@yesbox
Copy link
Contributor

yesbox commented Aug 8, 2018

A hack is better than intermittent boot failures. If the code is already using this pattern then using it once more doesn't make it worse if its omission a bug. Call it an intermediate workaround and save it for the refactoring or complete removal after swapping to networkd.

@infinisil
Copy link
Member

Needs at least a comment explaining why that's needed

@coretemp
Copy link
Contributor

What is a master interface? Additionally, this is not correct English.

Copy link
Contributor

@coretemp coretemp left a comment

Choose a reason for hiding this comment

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

As indicated.

@jemilsson
Copy link

I believe the proper networking terminology is "physical interface".

I am suffering from the issue this PR seems to be solving. I have a system that more often than not fails to boot because of the interfaces being brought up in the wrong order. Since the the networking scripts are already littered with similar hacks I have trouble seeing the issue with using the same pattern once more.

English isn't my first language either, but I tried to formulate a better comment below:

"We try to bring up the logical VLAN interface. If the physical interface the logical interface is dependent upon is not up yet we will fail to immediately bring up the logical interface. "|| true" is a hacky workaround for this issue, resulting in the logical interface being brought up later on."

@fpletz
Copy link
Member

fpletz commented Aug 13, 2018

@coretemp I agree with you but that's he mess we currently have. Refactoring those units would be a waste of time because there are already enough network configuration systems out there that do it right. We will probably move to networkd eventually.

@fpletz
Copy link
Member

fpletz commented Aug 13, 2018

@GrahamcOfBorg test networking.scripted.vlan

@fpletz fpletz self-assigned this Aug 13, 2018
@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: tests.networking.scripted.vlan

Partial log (click to expand)

client1: exit status 0
client2: running command: sync
client2: exit status 0
test script finished in 33.83s
cleaning up
killing client1 (pid 631)
killing client2 (pid 644)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/1i858pjg9gd26h0jdvyygmi1mqkyf2ny-vm-test-run-vlan-Networking-Scripted

@GrahamcOfBorg
Copy link

Failure on x86_64-linux (full log)

Attempted: tests.networking.scripted.vlan

Partial log (click to expand)

client2# [  247.095852] systemd-vconsole-setup[605]: Fonts will not be copied to remaining consoles
error: timed out waiting for the VM to connect
timed out waiting for the VM to connect
cleaning up
killing client2 (pid 600)
killing client1 (pid 612)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
builder for '/nix/store/vp55w09szpcx1y67fgl7kdbjsnm60i30-vm-test-run-vlan-Networking-Scripted.drv' failed with exit code 4
error: build of '/nix/store/vp55w09szpcx1y67fgl7kdbjsnm60i30-vm-test-run-vlan-Networking-Scripted.drv' failed

@yesbox
Copy link
Contributor

yesbox commented Sep 1, 2018

@vcunat @samueldr Ping 18.09 release managers, I do believe this PR is crucial.

@grahamc
Copy link
Member

grahamc commented Sep 2, 2018

@GrahamcOfBorg test networking.scripted.vlan

@GrahamcOfBorg

This comment has been minimized.

@GrahamcOfBorg

This comment has been minimized.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: tests.networking.scripted.vlan

Partial log (click to expand)

client2: exit status 0
client1: running command: sync
client1: exit status 0
test script finished in 26.40s
cleaning up
killing client2 (pid 597)
killing client1 (pid 609)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/3rv7lr5lypgh27mfc59rh2carmcq2ypx-vm-test-run-vlan-Networking-Scripted

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: tests.networking.scripted.vlan

Partial log (click to expand)

client2: exit status 0
client1: running command: sync
client1: exit status 0
test script finished in 22.68s
cleaning up
killing client2 (pid 631)
killing client1 (pid 643)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/3bmdbfzgzjxsp0vrhng2wk74l5bkmkdh-vm-test-run-vlan-Networking-Scripted

@grahamc grahamc merged commit 2d5f599 into NixOS:master Sep 2, 2018
@zhangyoufu zhangyoufu deleted the patch-28620 branch September 2, 2018 02:34
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.

networking.vlans fails to bring up interfaces at boot (RTNETLINK answers: Network is down)
9 participants