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

nixos/dhcpcd: optimize exitHook #209506

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Jan 7, 2023

Description of changes

Small optimize exit hook.
Fixed this message log:

dhcpcd[149086]: Failed to reload-or-try-restart ntpd.service: Unit ntpd.service not found.
dhcpcd[149086]: Failed to reload-or-try-restart openntpd.service: Unit openntpd.service not found.
dhcpcd[149086]: Failed to reload-or-try-restart chronyd.service: Unit chronyd.service not found.

cc @SuperSandro2000 @ncfavier @blitz @flokli

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Izorkin Izorkin force-pushed the update-dhcpcd-exit-hook branch 2 times, most recently from bb6a11a to ab11e27 Compare January 7, 2023 16:22
@Izorkin Izorkin force-pushed the update-dhcpcd-exit-hook branch 3 times, most recently from 7f0f720 to f911229 Compare January 9, 2023 10:47
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 9, 2023

Uhm, apparently it's a syntax error having an empty then clause:

client # [   13.485210] dhcpcd[614]: /etc/dhcpcd.exit-hook: line 10: syntax error near unexpected token `fi'
client # [   13.498172] dhcpcd[614]: /etc/dhcpcd.exit-hook: line 10: `fi'

The easy way out is to put a noop : before the optionalStrings.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 9, 2023

An alternative could be something like this:

let daemons = with config.services;
     lib.optional chrony.enable "chrony"
  ++ lib.optional ntp.enable "ntpd"
  ++ lib.optional openntpd.enable "openntpd";
in
pkgs.writeText "dhcpcd.exit-hook" ''
  if [ "$reason" = BOUND -o "$reason" = REBOOT ]; then
    # Restart ntpd. We need to restart it to make sure that it will actually do something:
    # if ntpd cannot resolve the server hostnames in its config file, then it will never do
    # anything ever again ("couldn't resolve ..., giving up on it"), so we silently lose
    # time synchronisation. This also applies to openntpd.
    for daemon in ${toString daemons}; do
      systemctl try-reload-or-restart "$daemon".service
    done
  fi

  ${cfg.runHook}
''

@Izorkin
Copy link
Contributor Author

Izorkin commented Jan 10, 2023

Returned a working variant.

Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

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

The last two checks are still redundant, but it works now. So, it's fine by me.

Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

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

I think we went full circle to the redundant checks again. What happened?

@Izorkin
Copy link
Contributor Author

Izorkin commented Jan 12, 2023

I think we went full circle to the redundant checks again. What happened?

#209506 (comment)

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I know to little about dhcpd and how it interacts with the other services to know if we should merge this or not.

@Izorkin
Copy link
Contributor Author

Izorkin commented Feb 9, 2023

Rebased PR.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1993

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 30, 2023

Rebased PR.

@Izorkin
Copy link
Contributor Author

Izorkin commented May 9, 2023

It is possible to include this PR in the 23.05 release?

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 15, 2023

Rebased PR.

@Izorkin Izorkin requested review from SuperSandro2000, ncfavier and rnhmjoj and removed request for rnhmjoj and ncfavier February 16, 2024 07:14
@SuperSandro2000 SuperSandro2000 merged commit f9477e3 into NixOS:master Feb 27, 2024
20 checks passed
@Izorkin Izorkin deleted the update-dhcpcd-exit-hook branch February 27, 2024 22:04
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.

7 participants