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

Rewrite syschdemd #126

Merged
merged 1 commit into from
Aug 29, 2022
Merged

Rewrite syschdemd #126

merged 1 commit into from
Aug 29, 2022

Conversation

K900
Copy link
Contributor

@K900 K900 commented Aug 22, 2022

Mostly just a big pile of cleanups.

Important changes:

  • refactor and clean up the code, fix all the shellcheck complaints
  • stop trying to guess paths in scripts and wrap them correctly instead
  • use nsfs instead of trying to figure out the right PID to copy namespaces from
  • clean up $WSLPATH to remove extra impure entries
  • don't try to restart systemd if it died
  • make sure the store is read-only before we do anything, so systemd can't mess with it

@nzbr nzbr added the enhancement New feature or request label Aug 22, 2022
, util-linux
, defaultUser
, automountPath
,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

,
}:
let
writeShellScript =
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to mkWrappedScript.

/bin/sh -c "cd \"$PWD\"; $exportCmd; source /etc/set-environment; exec $command"
}

main "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Missing final new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reformatted the whole thing with shfmt.

Copy link
Member

Choose a reason for hiding this comment

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

Just for my interest: What settings are you using? Maybe we should document that somewhere to not reformat files multiple times. I am always using shfmt -ci -i 2 -s -w $FILE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add it as a check in CI.

#!/usr/bin/env bash
set -euo pipefail

[ "${NIXOS_WSL_DEBUG:-}" == "1" ] && set -x
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[ "${NIXOS_WSL_DEBUG:-}" == "1" ] && set -x
[[ -v NIXOS_WSL_DEBUG ]] && set -x

testing if it is set to anything maybe makes it easier if people accidentally use true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, applied.

@K900 K900 force-pushed the refactor branch 2 times, most recently from 596e8ba to e3b87ea Compare August 22, 2022 09:09
@nzbr
Copy link
Member

nzbr commented Aug 22, 2022

Docker Desktop is broken, because the mount --make-rshared ${automountPath} line is missing from the systemd wrapper script. It's needed so that the bind mount that docker creates is present outside of the systemd container

@nzbr
Copy link
Member

nzbr commented Aug 22, 2022

Other than that, everything seems to be working

@K900
Copy link
Contributor Author

K900 commented Aug 23, 2022

I tried adding it to the outside script, can you see if that works?

@K900 K900 force-pushed the refactor branch 2 times, most recently from b4a504c to 819b815 Compare August 24, 2022 07:05
@nzbr
Copy link
Member

nzbr commented Aug 25, 2022

I tried adding it to the outside script, can you see if that works?

That does not seem to work. The volume shows up as an empty directory inside the container

@K900
Copy link
Contributor Author

K900 commented Aug 25, 2022

I am very confused by how the mounts work then. Can you run findmnt -o+PROPAGATION outside the container?

@nzbr
Copy link
Member

nzbr commented Aug 25, 2022

mounts.txt

/mnt/wsl/docker-desktop-bind-mounts/20fd66ba2c571f0ae3898ed7b5fd93125b557fd3925bd0dc7410daef649e26e1 is the relevant docker mount. It has contents inside the systemd namespace, but outside of it, the mount isn't present and it therefore shows up as an empty directory

@K900
Copy link
Contributor Author

K900 commented Aug 25, 2022

Is this from inside the namespace or outside?

@nzbr
Copy link
Member

nzbr commented Aug 25, 2022

The command output is from inside the systemd namespace, but not the docker container. Outside the systemd namespace it looks like this:

mounts-outside.txt

@nzbr
Copy link
Member

nzbr commented Aug 25, 2022

This is also a problem for other mounts created inside the namespace: I have a tmpfs on /tmp in my NixOS installation (with the current syschdemd). Because of that any files in /tmp can't be accessed from outside it (e.g. through SMB with the explorer or interop) and trying to use interop in there results in a not at all helpful error:
image

When I made the docker-desktop fix, I also tried removing mount namespacing completely, but that means that the namespace and the ouside have the same /proc filesystem which breaks even more things

@K900
Copy link
Contributor Author

K900 commented Aug 25, 2022

Can you try the latest commit? I think unshare was just doing a weird.

@nzbr
Copy link
Member

nzbr commented Aug 25, 2022

The latest commit seems to be broken
image

@K900
Copy link
Contributor Author

K900 commented Aug 26, 2022

Forgot to remove the other thing. Can you try it now?

@nzbr
Copy link
Member

nzbr commented Aug 29, 2022

image

Seems to be working now

Mostly just a big pile of cleanups.

Important changes:
- refactor and clean up the code, fix all the shellcheck complaints
- stop trying to guess paths in scripts and wrap them correctly instead
- use nsfs instead of trying to figure out the right PID to copy namespaces from
- clean up $WSLPATH to remove extra impure entries
- don't try to restart systemd if it died
- make sure the store is read-only before we do anything, so systemd can't mess with it
- reformat shell scripts with shfmt
@K900
Copy link
Contributor Author

K900 commented Aug 29, 2022

Squashed, should be good to go now.

@nzbr nzbr merged commit c1b0259 into nix-community:main Aug 29, 2022
@nzbr nzbr changed the title feat: rewrite syschdemd Rewrite syschdemd Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants