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

treewide: deprecate ip-up.target #18319

Merged
merged 6 commits into from
Sep 10, 2016
Merged

treewide: deprecate ip-up.target #18319

merged 6 commits into from
Sep 10, 2016

Conversation

groxxda
Copy link
Contributor

@groxxda groxxda commented Sep 5, 2016

Motivation for this change

Systemd upstream provides targets for networking. This also includes a target network-online.target.

I would like to see ip-up.target completely gone.

In this PR I remove / replace most occurrences since some of them were even wrong and could delay startup.

There are still references that I'm unsure about:

  • nixos/modules/services/networking/dhcpcd.nix: is this hook really needed?
  • nixos/modules/services/networking/avahi-daemon.nix: has ordering wantedBy and before if-up.target which looks plain wrong to me. We should get avahi-daemon closer to upstream definition that also includes socket-activation, but this is out of scope of this PR
  • nixos/modules/services/networking/networkmanager.nix: is this really needed?
  • nixos/modules/tasks/network-interfaces-scripted.nix: is this needed?
  • nixos/modules/virtualisation/nova.nix: this module still uses the jobs interface, which was deprecated when I started using nixos
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

I would be thankful for comments about the remaining occurrences as well as deprecating ip-up.target in general.

@mention-bot
Copy link

@groxxda, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @globin and @rbvermaa to be potential reviewers

@grahamc
Copy link
Member

grahamc commented Sep 5, 2016

:lgtm:


Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


nixos/modules/services/network-filesystems/drbd.nix, line 58 [r1] (raw file):

      after = [ "systemd-udev.settle.service" "network.target" ];
      wants = [ "systemd-udev.settle.service" ];
      wantedBy = [ "multi-user.target" ];

Verified upstream: https://apt-browse.org/browse/ubuntu/trusty/main/i386/drbd8-utils/2:8.4.4-1ubuntu1/file/lib/systemd/system/drbd.service


nixos/modules/virtualisation/azure-agent.nix, line 182 [r1] (raw file):

      wantedBy = [ "multi-user.target" ];
      after = [ "network-online.target" "sshd.service" ];
      wants = [ "network-online.target" ];

I was suspicious this needed to be up sooner, but verified otherwise: https://github.com/Azure/WALinuxAgent/blob/master/init/coreos/cloud-config.yml


Comments from Reviewable

@edolstra
Copy link
Member

edolstra commented Sep 5, 2016

Looks good to me.

What did you mean by the dhcpcd hook? The start ip-up.target can obviously be removed, but the try-restart of ntpd is still necessary unless ntpd has suddenly learned how to detect interface changes.

@groxxda
Copy link
Contributor Author

groxxda commented Sep 5, 2016

Rebased to resolve merge conflicts and also added modified dhcpcd networkmanager and network-interfaces-scripted.
Please review whether the new commits look semantically good to you, I'm not an expert for the scripted networking stuff.

@grahamc
Copy link
Member

grahamc commented Sep 5, 2016

Reviewed 3 of 3 files at r3.
Review status: 16 of 17 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@grahamc
Copy link
Member

grahamc commented Sep 7, 2016

:lgtm: but untested


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@groxxda groxxda mentioned this pull request Sep 8, 2016
7 tasks
@joachifm
Copy link
Contributor

LGTM. Anything holding this back?

@fpletz fpletz merged commit 27bc34f into NixOS:master Sep 10, 2016
@joachifm
Copy link
Contributor

Just a heads up: some networking related tests are now failing https://hydra.nixos.org/eval/1292016#tabs-now-fail

@fpletz
Copy link
Member

fpletz commented Sep 10, 2016

I'm investigating.

@groxxda
Copy link
Contributor Author

groxxda commented Sep 10, 2016

Thanks @fpletz. Feel free to revert until I find time to look into this.
By the way, is there a sane way to attach to a test vm to debug the failure?

fpletz added a commit that referenced this pull request Sep 11, 2016
See #18319 for details. Starting network-online.target manually does not
work as it hangs indefinitely.

Additionally, don't treat avahi and dhcpcd special and sync their systemd units
with the respective upstream suggestion.
@fpletz
Copy link
Member

fpletz commented Sep 11, 2016

@groxxda Found some info on https://nixos.org/nixos/manual/index.html#sec-running-nixos-tests and was able to debug with the execute function.

Should be fixed with c58654e.

@groxxda
Copy link
Contributor Author

groxxda commented Sep 11, 2016

Thank you @fpletz for looking into this and fixing it!
I'll bring some 🍻 when we meet next time.

@groxxda groxxda deleted the ipupdown branch September 11, 2016 11:17
@groxxda groxxda mentioned this pull request Sep 11, 2016
10 tasks
lbonn added a commit to lbonn/nixpkgs that referenced this pull request Nov 30, 2016
It was deprecated and removed from all modules in the tree by NixOS#18319.

The wireguard module PR (NixOS#17933) was still in the review at the time and
the deprecated usage managed to slip inside.
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.

6 participants