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

No chdir - allowing relative paths in command line #210

Closed
wants to merge 3 commits into from

Conversation

Lockszmith-GH
Copy link
Contributor

@Lockszmith-GH Lockszmith-GH commented Jun 25, 2024

Addresses #209

@Lockszmith-GH Lockszmith-GH changed the base branch from main to develop June 25, 2024 13:16
@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 25, 2024

will remove draft status when I get a chance to test everything

🔳 jlmkr create
✅ jlmkr edit
🔳 jlmkr exec
🔳 jlmkr images
✅ jlmkr list
🔳 jlmkr log
🔳 jlmkr remove
🔳 jlmkr restart
✅ jlmkr shell
❌ jlmkr start
🔳 jlmkr status
✅ jlmkr stop

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jun 25, 2024

Looking forward to the results of testing :)

By the way the chdir inside class Chroot should be left as is. Otherwise chroot will break and the absolute symlinks inside nixos rootfs will cause trouble. E.g. when parsing os-release.

Maybe the solution is not to drop the chdir but instead remember the original working dir somewhere so the relative config path can still be constructed. Not sure what the best approach is.

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 25, 2024

I started by dropping everything so I can test it - but you're right, that location probably should remain.
As for chdir - in a a way that is 'bad form' to globally chdir.

If you want to preserve 'relative to a location' operation, I think a better pattern would be to use the pushd/popd paradigm from bash:

  1. pushd <location of interest>
  2. perform operation accepting relative to <location of interest> path
  3. popd location

or alternatively, calculate absolute paths all the way - which when debugging makes things easier to catch.

@Lockszmith-GH
Copy link
Contributor Author

And now I actually see what you meant, and agree, still some stuff needs to be resolved.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jun 26, 2024

Yeah my initial design was to keep everything relative to (and included in) the jailmaker directory. From the perspective of jlmkr there'd be only this directory to care about. But with the introduction of config templates I forgot those could be located outside this directory...

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 28, 2024

In the next comment I'll document the code changes.

This comment documents descibes the automated test bash-script I used to test as many edge cases as I could think off)
moved comment to #215 ~

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 28, 2024

  • Removed os.getcwd() in create_jail
  • Modified remove_zfs_dataset and create_zfs_dataset to work with full dataset paths. Added a safety to only allow actions within the Jailmaker dataset.
  • Modified get_zfs_dataset to produce path for existing dataset as well as 'future' datasets, where only the parent dataset exists.
  • Removed os.chdir from start of script.

@Lockszmith-GH Lockszmith-GH marked this pull request as ready for review June 28, 2024 02:25
@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 28, 2024

All tests passed for nixos.
A complete full test run with nixos completed successfully, as well as a single interactive run.

Complete run output - click to expand
> sudo STOP=i SCALE_POOL_ROOT=$SCALE_POOL_ROOT $SCALE_POOL_ROOT/jailmaker/test/test-jlmkr nixos
🔳 jlmkr list
NAME   RUNNING STARTUP GPU_INTEL GPU_NVIDIA OS     VERSION ADDRESSES
docker False   False   False     False      debian 12      -
🔳 jlmkr images
Downloading the image index

---
DIST             RELEASE     ARCH   VARIANT  BUILD
---
almalinux        8           amd64  default  20240627_23:08
almalinux        9           amd64  default  20240627_23:08
alt              Sisyphus    amd64  default  20240628_01:17
alt              p10         amd64  default  20240628_01:17
alt              p9          amd64  default  20240628_01:17
archlinux        current     amd64  default  20240627_04:18
centos           7           amd64  default  20240627_07:08
centos           9-Stream    amd64  default  20240627_07:08
debian           bookworm    amd64  default  20240627_05:24
debian           bullseye    amd64  default  20240627_05:24
debian           buster      amd64  default  20240627_05:24
debian           trixie      amd64  default  20240627_05:24
fedora           39          amd64  default  20240627_20:33
fedora           40          amd64  default  20240627_20:33
kali             current     amd64  default  20240627_17:14
mint             ulyana      amd64  default  20240626_08:51
mint             ulyssa      amd64  default  20240626_08:51
mint             uma         amd64  default  20240626_08:51
mint             una         amd64  default  20240626_08:51
mint             vanessa     amd64  default  20240626_08:51
mint             vera        amd64  default  20240626_08:51
mint             victoria    amd64  default  20240626_08:51
nixos            23.11       amd64  default  20240628_01:00
nixos            24.05       amd64  default  20240628_01:00
nixos            unstable    amd64  default  20240628_01:01
openeuler        20.03       amd64  default  20240627_15:48
openeuler        22.03       amd64  default  20240627_15:48
openeuler        23.09       amd64  default  20240627_15:48
opensuse         15.4        amd64  default  20240624_04:38
opensuse         15.5        amd64  default  20240624_04:20
opensuse         tumbleweed  amd64  default  20240627_05:13
oracle           7           amd64  default  20240627_07:46
oracle           8           amd64  default  20240627_07:46
oracle           9           amd64  default  20240627_07:46
rockylinux       8           amd64  default  20240627_02:06
rockylinux       9           amd64  default  20240627_02:06
slackware        15.0        amd64  default  20240627_23:08
slackware        current     amd64  default  20240627_23:17
springdalelinux  7           amd64  default  20240627_06:38
springdalelinux  8           amd64  default  20240627_06:38
springdalelinux  9           amd64  default  20240627_06:38
ubuntu           bionic      amd64  default  20240626_07:42
ubuntu           focal       amd64  default  20240626_07:42
ubuntu           jammy       amd64  default  20240626_07:42
ubuntu           lunar       amd64  default  20240626_07:42
ubuntu           mantic      amd64  default  20240626_07:42
ubuntu           noble       amd64  default  20240626_07:42
ubuntu           xenial      amd64  default  20240626_07:42
---
🔳 jlmkr create --config /mnt/tank/jailmaker/templates/nixos/config nixos-test
USE THIS SCRIPT AT YOUR OWN RISK!
IT COMES WITHOUT WARRANTY AND IS NOT SUPPORTED BY IXSYSTEMS.
Creating jail nixos-test from config template /mnt/tank/jailmaker/templates/nixos/config.

Hint: run `jlmkr.py create` without any arguments for interactive config.
Or use CLI args to override the default options.
For more info, run: `jlmkr.py create --help`

/mnt/tank/jailmaker/jails None
Using image from local cache
Unpacking the rootfs

---
You just created an Nixos 24.05 x86_64 (20240627_01:02) container.
🔳 jlmkr start nixos-test

Starting jail nixos-test with the following command:

systemd-run --property=KillMode=mixed --property=Type=notify --property=RestartForceExitStatus=133 --property=SuccessExitStatus=133 --property=Delegate=yes --property=TasksMax=infinity --collect --setenv=SYSTEMD_NSPAWN_LOCK=0 --unit=jlmkr-nixos-test --working-directory=/mnt/tank/jailmaker/jails/nixos-test '--description=My nspawn jail nixos-test [created with jailmaker]' --property=ExecStartPre=/mnt/tank/jailmaker/jails/nixos-test/.ExecStartPre -- systemd-nspawn --keep-unit --quiet --boot --bind-ro=/sys/module --inaccessible=/sys/module/apparmor --machine=nixos-test --directory=rootfs --network-bridge=br1 --bind-ro=./lxd.nix:/etc/nixos/lxd.nix

Running as unit: jlmkr-nixos-test.service
🔳 jlmkr restart nixos-test
Waiting 4s seconds before test...
Wait for nixos-test to stop.
Starting jail nixos-test with the following command:

systemd-run --property=KillMode=mixed --property=Type=notify --property=RestartForceExitStatus=133 --property=SuccessExitStatus=133 --property=Delegate=yes --property=TasksMax=infinity --collect --setenv=SYSTEMD_NSPAWN_LOCK=0 --unit=jlmkr-nixos-test --working-directory=/mnt/tank/jailmaker/jails/nixos-test '--description=My nspawn jail nixos-test [created with jailmaker]' --property=ExecStartPre=/mnt/tank/jailmaker/jails/nixos-test/.ExecStartPre -- systemd-nspawn --keep-unit --quiet --boot --bind-ro=/sys/module --inaccessible=/sys/module/apparmor --machine=nixos-test --directory=rootfs --network-bridge=br1 --bind-ro=./lxd.nix:/etc/nixos/lxd.nix

Running as unit: jlmkr-nixos-test.service
🔳 jlmkr edit nixos-test
Waiting 4s seconds before test...

Restart the jail for edits to apply (if you made any).
🔳 jlmkr shell nixos-test
Waiting 4s seconds before test...
Connected to machine nixos-test. Press ^] three times within 1s to exit session.

[root@nixos:~]# echo one
one

[root@nixos:~]#
logout
Connection to machine nixos-test terminated.
🔳 jlmkr exec nixos-test /bin/sh -c echo exec successful
Waiting 4s seconds before test...
exec successful
🔳 jlmkr status nixos-test --no-pager
● jlmkr-nixos-test.service - My nspawn jail nixos-test [created with jailmaker]
     Loaded: loaded (/run/systemd/transient/jlmkr-nixos-test.service; transient)
  Transient: yes
     Active: active (running) since Thu 2024-06-27 22:31:05 EDT; 21s ago
    Process: 3445762 ExecStartPre=/mnt/tank/jailmaker/jails/nixos-test/.ExecStartPre (code=exited, status=0/SUCCESS)
   Main PID: 3445763 (systemd-nspawn)
     Status: "Container running: Ready."
      Tasks: 21
     Memory: 30.0M
        CPU: 2.537s
     CGroup: /system.slice/jlmkr-nixos-test.service
             ├─payload
             │ ├─3445765 /run/current-system/systemd/lib/systemd/systemd
             │ ├─3445864 /nix/store/d9ff8aqv537mlhpinncx6dwc7a5ky6gk-systemd-255.6/lib/systemd/systemd-journald
             │ ├─3446016 /nix/store/d9ff8aqv537mlhpinncx6dwc7a5ky6gk-systemd-255.6/lib/systemd/systemd-resolved
             │ ├─3446042 /nix/store/d9ff8aqv537mlhpinncx6dwc7a5ky6gk-systemd-255.6/lib/systemd/systemd-networkd
             │ ├─3446072 /nix/store/w44ykc1hjhg6z1m06x1h38p6zk86h91b-nsncd-unstable-2024-01-16/bin/nsncd
             │ ├─3446073 /nix/store/nn54yahdasinv9hyk1hjwpakw8sqqyq0-dbus-1.14.10/bin/dbus-daemon --system --address=systemd: --nofork --nopidfile …
             │ ├─3446086 /nix/store/d9ff8aqv537mlhpinncx6dwc7a5ky6gk-systemd-255.6/lib/systemd/systemd-logind
             │ ├─3446089 agetty --login-program /nix/store/3kgyqixfqf6vhq3cw41vgxv2i0x2fayz-shadow-4.14.6/bin/login --noclear --keep-baud console 1…
             │ ├─3446391 /nix/store/d9ff8aqv537mlhpinncx6dwc7a5ky6gk-systemd-255.6/lib/systemd/systemd --user
             │ └─3446394 "(sd-pam)"
             └─supervisor
               └─3445763 /usr/bin/systemd-nspawn --keep-unit --quiet --boot --bind-ro=/sys/module --inaccessible=/sys/module/apparmor --machine=nix…

Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Finished Wait for Network to be Configured.
Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Reached target Network is Online.
Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Reached target Multi-User System.
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]: <<< Welcome to NixOS 24.05.2150.89c49874fb15 (x86_64) - console >>>
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]: Log in as "root" with an empty password.
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
🔳 jlmkr log nixos-test -n 10
Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Finished Wait for Network to be Configured.
Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Reached target Network is Online.
Jun 27 22:31:07 kateryna systemd-nspawn[3445763]: [  OK  ] Reached target Multi-User System.
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]: <<< Welcome to NixOS 24.05.2150.89c49874fb15 (x86_64) - console >>>
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]: Log in as "root" with an empty password.
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
Jun 27 22:31:08 kateryna systemd-nspawn[3445763]:
🔳 jlmkr stop nixos-test
Wait for nixos-test to stop.🔳 jlmkr remove nixos-test <<<nixos-test

CAUTION: Type "nixos-test" to confirm jail deletion!



Cleaning up: /mnt/tank/jailmaker/jails/nixos-test.
Report Summary
Report for:
        CWD: /mnt/tank/jailmaker/templates/docker            JAIL_CONFIG: /mnt/tank/jailmaker/templates/nixos/config

✅ jlmkr create --config /mnt/tank/jailmaker/templates/nixos/config nixos-test
✅ jlmkr edit nixos-test
✅ jlmkr exec nixos-test /bin/sh -c 'echo exec successful'
✅ jlmkr images
✅ jlmkr list
✅ jlmkr log nixos-test -n 10
✅ jlmkr remove nixos-test <<<'nixos-test'
✅ jlmkr restart nixos-test
✅ jlmkr shell nixos-test
✅ jlmkr start nixos-test
✅ jlmkr status nixos-test --no-pager
✅ jlmkr stop nixos-test
Stopped: Single Test
[ble: elapsed 60.793s (CPU 58.6%)]

@Jip-Hop Jip-Hop force-pushed the develop branch 2 times, most recently from 31f6c6b to 1bd58c9 Compare June 28, 2024 11:56
@Jip-Hop
Copy link
Owner

Jip-Hop commented Jun 28, 2024

Thanks again for all this effort @Lockszmith-GH! For me it's easier to review and accept changes when they're offered in smaller bits. For example this PR was about fixing chdir bug, but in order to fix it I now also need to accept the test script and other changes (for which I'm not immediately sure if they're required for the chdir fix or required for the test script, or perhaps they are not required by either and actually independent improvements).

For example I'd easily accept passing args to log and status if that was in it's own PR. I've cherry picked that commit into the develop branch for now.

You think you could split these changes into individual PRs? Automated testing would be a great addition but I'd like to discuss it a bit more (and understand the approach better) and I don't think fixing the chdir bug should depend on it.

P.S. if you check "Allow edits by maintainer" in the PR I can also make changes to your branch. E.g. I tried fixing some spelling and shellcheck warnings inside test/test-jlmkr but couldn't push my changes.

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jun 28, 2024

Because I chose to organize repos using Organizations, I don't have the "Allow edits by maintainer" checkbox. Sorry about that. I did however invite you to join the repo.

image

I'll split the commit into a separate PR. I'll also pass the shell script through shellcheck and a spellchecker (I did all the work on my TrueNAS SCALE machine, where I didn't install all of the plugins)

Switching to draft until all of that is done.

@Lockszmith-GH Lockszmith-GH marked this pull request as draft June 28, 2024 12:33
@Jip-Hop Jip-Hop mentioned this pull request Jun 29, 2024
@Jip-Hop
Copy link
Owner

Jip-Hop commented Jun 29, 2024

@Lockszmith-GH please see #86 regarding automated testing.

@Lockszmith-GH
Copy link
Contributor Author

I will, this weekend I'm away, will continue throughout the week.

@Lockszmith-GH Lockszmith-GH marked this pull request as ready for review July 1, 2024 22:50
@Lockszmith-GH
Copy link
Contributor Author

Removed the automated testing code from this branch.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 3, 2024

I did however invite you to join the repo.

Thanks but I don't want to join a GitHub organization for this purpose.

When you accept an invitation to join an organization, the organization owners may be able to see:

Your public profile information
Your email address
If you have two-factor authorization enabled
Repositories you have access to within the organization, and your access level
Certain activity within the organization
Country of request origin
Your IP address

https://docs.github.com/en/enterprise-cloud@latest/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-membership-in-organizations/about-organization-membership

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jul 3, 2024

sorry about that, I'm currently stuck with the organization - just because of past choices.
Didn't think I need to invite you into the organization - just the repo

Let me know if there are any changes you want me to make - or just grab the code that you like

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 3, 2024

Screen Shot 2024-07-03 at 17 08 18

Rather not share IP address unless I really have to. 😉

or just grab the code that you like

Thanks! I took the code I like while trying to keep changes as minimal as possible. You think you could have a look and test this branch? https://github.com/Jip-Hop/jailmaker/tree/issue-209 I hope I didn't overlook anything...

@Lockszmith-GH
Copy link
Contributor Author

Lockszmith-GH commented Jul 3, 2024

I see... the IP - probably in logs.
All good. I'll check the branch

@Lockszmith-GH
Copy link
Contributor Author

The develop branch looks good to me. Closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants