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

Revert "nixos-container: use systemd-run instead of nsenter" #83392

Merged
merged 1 commit into from Mar 26, 2020
Merged

Revert "nixos-container: use systemd-run instead of nsenter" #83392

merged 1 commit into from Mar 26, 2020

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 26, 2020 •
edited

Motivation for this change

⚠️ Please note that I'll only revert this on release-20.03 for now to get rid of the regression within the container-backend of nixops. After merging this, an issue should be opened to track further changes (to use the original commit without further regressions on 20.09).

This reverts commit 7cb100b.

This appears to break at least the container-backend of nixops: when
running switch-to-configuration within nixos-container run, the
running systemd-instance gets reloaded which appears to kill the
systemd-run command and causes nixos-container run to hang.

The full issue is reported in the original PR[1].

[1] #67332 (comment)

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 nixpkgs-review --run "nixpkgs-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.

@Ma27
Revert "nixos-container: use systemd-run instead of nsenter"
7f1ba60 
This reverts commit 7cb100b.

This appears to break at least the `container`-backend of `nixops`: when
running `switch-to-configuration` within `nixos-container run`, the
running `systemd`-instance gets reloaded which appears to kill the
`systemd-run` command and causes `nixos-container run` to hang.

The full issue is reported in the original PR[1].

[1] NixOS#67332 (comment)
@Ma27 Ma27 added 0.kind: regression Something that worked before working no longer 6.topic: nixos-container Imperative and declarative systemd-nspawn containers labels Mar 26, 2020
@Ma27 Ma27 added this to the 20.03 milestone Mar 26, 2020
@Ma27
Copy link
Member Author

Ma27 commented Mar 26, 2020 •
edited

Please note that I didn't test it sufficiently as I need to build a fresh kernel on latest release-20.03 to run the tests 😅

Tests are passing locally as well 🎉

@GrahamcOfBorg test containers-imperative

@Ma27 Ma27 mentioned this pull request Mar 26, 2020
Merged
10 tasks
uvNikita
uvNikita reviewed Mar 26, 2020
View reviewed changes
Copy link
Contributor

@uvNikita uvNikita left a comment

Choose a reason for hiding this comment

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

This should be safe to revert since we don't have support for unprivileged containers yet anyway. Thanks for looking into it!

@arianvp
Copy link
Member

arianvp commented Mar 26, 2020

lgtm

@worldofpeace worldofpeace merged commit a3a02d6 into NixOS:release-20.03 Mar 26, 2020
@worldofpeace
Copy link
Contributor

worldofpeace commented Mar 26, 2020 •
edited

@Ma27 Is there a PR for master with this as well? If we don't do that it's less likely it will be fixed. (I did read your header)

@Ma27 Ma27 deleted the nixos-container-nsenter-20.03 branch March 26, 2020 15:50
@Ma27
Copy link
Member Author

Ma27 commented Mar 26, 2020

Is there a PR for master with this as well? If we don't do that it's less likely it will be fixed. (I did read your header)

I would've opened a tracking issue to fix it properly. Or do you prefer a revert for master as well?

@worldofpeace
Copy link
Contributor

worldofpeace commented Mar 26, 2020 •
edited

Is there a PR for master with this as well? If we don't do that it's less likely it will be fixed. (I did read your header)

I would've opened a tracking issue to fix it properly. Or do you prefer a revert for master as well?

I'd actually say both since we should be using systemd-run eventually.

@Ma27 Ma27 mentioned this pull request Mar 26, 2020
Open
@Ma27
Copy link
Member Author

Ma27 commented Mar 26, 2020

I forward-ported it to master: ebb6e38
Also opened #83432

@Ma27 Ma27 mentioned this pull request Oct 2, 2023
Merged
12 tasks
@thiagokokada thiagokokada mentioned this pull request Oct 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uvNikita uvNikita uvNikita left review comments

@fpletz fpletz Awaiting requested review from fpletz

@veprbl veprbl Awaiting requested review from veprbl

@arianvp arianvp Awaiting requested review from arianvp

@disassembler disassembler Awaiting requested review from disassembler

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

Assignees
No one assigned
Labels
0.kind: regression Something that worked before working no longer 6.topic: nixos 6.topic: nixos-container Imperative and declarative systemd-nspawn containers 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ma27 @arianvp @worldofpeace @uvNikita