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

Jump to bottom

samba: remove redundant dependency on network.target #73061

Merged
merged 1 commit into from Nov 13, 2019
Merged

samba: remove redundant dependency on network.target #73061

merged 1 commit into from Nov 13, 2019

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Nov 8, 2019

This reverts commit 679d5e8.

Motivation for this change

This change not fixed autostart samba service - #41841 (comment)

cc @joachifm

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

flokli
flokli requested changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

Even though #41841 didn't fix the autostart issue, ordering samba after network.target does make a lot of sense:

       network.target
           This unit is supposed to indicate when network functionality is available, but it is only very weakly defined
           what that is supposed to mean, with one exception: at shutdown, a unit that is ordered after network.target
           will be stopped before the network — to whatever level it might be set up then — is shut down. It is hence
           useful when writing service files that require network access on shutdown, which should order themselves
           after this target, but not pull it in. Also see Running Services After the Network is up[1] for more
           information. Also see network-online.target described above.

So this revert should not be merged.

Can you provide a testcase showing the problem? I couldn't get the samba nixos test to fail, so would assume it might be a local service misconfiguration, for example binding on ip adresses there yet.

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 8, 2019

@flokli this code

nixpkgs/nixos/modules/services/network-filesystems/samba.nix

Line 232 in a8b2e30

after = [ "samba-setup.service" "network.target" ];

already adds

Results samba-smbd.service:

[Unit]
After=network.target network.target
Description=Samba Service Daemon smbd
PartOf=samba.target
X-Restart-Triggers=/nix/store/v15knwrimnswxs3xwfkbj380n40lqc86-smb.conf

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 8, 2019

Rechecking the configuration

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 8, 2019

Correct work with this custom configuration

  systemd.services.samba-smbd = {
    wantedBy = [ "multi-user.target" ];
    after = [ "network.target" "samba-nmbd.service" ];
  };

I don’t know how to add it here

nixpkgs/nixos/modules/services/network-filesystems/samba.nix

Lines 237 to 246 in a8b2e30

services = {
samba-smbd = daemonService "smbd" "";
samba-nmbd = mkIf cfg.enableNmbd (daemonService "nmbd" "");
samba-winbindd = mkIf cfg.enableWinbindd (daemonService "winbindd" "");
samba-setup = {
description = "Samba Setup Task";
script = setupScript;
unitConfig.RequiresMountsFor = "/var/lib/samba";
};
};

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 8, 2019 •
edited

@flokli
How to coreect this variant?

diff --git a/nixos/modules/services/network-filesystems/samba.nix b/nixos/modules/services/network-filesystems/samba.nix
index 055508a3224..cbdb5b03b9c 100644
--- a/nixos/modules/services/network-filesystems/samba.nix
+++ b/nixos/modules/services/network-filesystems/samba.nix
@@ -42,9 +42,10 @@ let
   # This may include nss_ldap, needed for samba if it has to use ldap.
   nssModulesPath = config.system.nssModules.path;

-  daemonService = appName: args:
+  daemonService = appName: args: test1:
     { description = "Samba Service Daemon ${appName}";

+      after = [ "${test1}" ];
       requiredBy = [ "samba.target" ];
       partOf = [ "samba.target" ];

@@ -234,7 +235,7 @@ in
           # Refer to https://github.com/samba-team/samba/tree/master/packaging/systemd
           # for correct use with systemd
           services = {
-            samba-smbd = daemonService "smbd" "";
+            samba-smbd = if cfg.enableNmbd then (daemonService "smbd" "" "samba-nmbd.service") else (daemonService "smbd" "" "");
             samba-nmbd = mkIf cfg.enableNmbd (daemonService "nmbd" "");
             samba-winbindd = mkIf cfg.enableWinbindd (daemonService "winbindd" "");
             samba-setup = {


error: cannot coerce a set to a string, at /home/user/works/src-nix/nixpkgs/nixos/modules/services/network-filesystems/samba.nix:48:18

@flokli
Copy link
Contributor

flokli commented Nov 8, 2019

You meant to say that samba-smbd.service, samba-nmbd.service and samba-winbind.service are part of samba.target, which already has an After=network.target, right?

nixpkgs/nixos/modules/services/network-filesystems/samba.nix

Line 232 in a8b2e30

after = [ "samba-setup.service" "network.target" ];

That's indeed correct, so the after = [ "network.target" ]; in

nixpkgs/nixos/modules/services/network-filesystems/samba.nix

Line 48 in a8b2e30

after = [ "network.target" ];
is redundant, and can be removed.

I however don't understand your last 3 comments, sorry. I'm fine to merge this PR, if we change the commit message to explain the redundancy. WDYT?

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 9, 2019

You meant to say that samba-smbd.service, samba-nmbd.service and samba-winbind.service are part of samba.target, which already has an After=network.target, right?

Yes.

How to correctly rename PR?

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 9, 2019

I however don't understand your last 3 comments, sorry

If add to my configuration

  systemd.services.samba-smbd = {
    after = [ "samba-nmbd.service" ];
  };

when the startup OS, the samba service works.
How to add samba-smbd service dependency on samba-nmbd.service.
With #73061 (comment) - bad work

@Izorkin
samba: remove redundant dependency on network.target
d35ba10 
This reverts commit 679d5e8.
Services samba-smbd, samba-nmbd and samba-winbind are part of
samba.target, which already has an After=network.target
@Izorkin Izorkin changed the title Revert "samba: fix autostart service" samba: remove redundant dependency on network.target Nov 10, 2019
@flokli flokli merged commit 1dacc3d into NixOS:master Nov 13, 2019
@flokli
Copy link
Contributor

flokli commented Nov 13, 2019

Thanks! Can you open another PR adding above change

 systemd.services.samba-smbd = {
    after = [ "samba-nmbd.service" ];
  };

to the samba module?

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 14, 2019

@flokli if i can rewrite the module.

@Izorkin Izorkin deleted the samba branch November 14, 2019 07:56
@flokli
Copy link
Contributor

flokli commented Nov 15, 2019

@Izorkin no need to rewite entirely ;-)

I'd suggest to extend the daemonService function to conditionally add "samba-nmbd.service" to after, if appName is smbd and nmbd is configured to be enabled. Can you file a PR?

@Izorkin Izorkin mentioned this pull request Nov 17, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Izorkin @flokli