Use systemd-nspawn instead of chroot #74

Merged
lpoujol merged 36 commits from server-nspawn into master 2024-08-28 17:16:40 +02:00
Owner
No description provided.
jlecour added 3 commits 2024-06-27 18:19:55 +02:00
Big change :
-> systemd-nspawn usage instead of chroot
-> Folder directory structure changes

Before : /backup/jails/JAIL_NAME/ <- RootFS of chroot jail
Now :
-  /backup/jails/JAIL_NAME/rootfs <- Jail rootfs (ro)
-  /backup/jails/JAIL_NAME/var <- Jail /var (rw)
-  /backup/jails/JAIL_NAME/data <- Jail /data (rw)

New dependencies :
- systemd-nspawn (package systemd-container on Debian)
- check_ssh (package monitoring-plugins-basic on Debian

Still a Work In Progress.
As of now, it won't work on existing servers as the jail folders needs
to be converted to the new folder structure
server: new command log, to quickly show last logs from journalctl
All checks were successful
gitea/evobackup/pipeline/head This commit looks good
840a72d74c
jlecour added 1 commit 2024-06-27 18:23:11 +02:00
jlecour added 1 commit 2024-06-27 18:27:04 +02:00
When strings must be quoted in messages, it's common to use an escaped backtick at the beginning, then a single-quote at the end.
jlecour added 1 commit 2024-06-27 18:28:57 +02:00
jlecour added 1 commit 2024-06-27 18:32:39 +02:00
jlecour reviewed 2024-06-27 18:41:54 +02:00
@ -466,2 +492,4 @@
grep --extended-regexp --only-matching "${pattern}" "${file}" | tail -1 | cut -d= -f2
}
get_jail_sshd_pid(){
Author
Owner

I'm not sure this will work correctly all the time.

In a random test I got this process list :

root     1991710  0.0  0.0  13464  5788 ?        Ss   16:35   0:00 systemd-nspawn --quiet --keep-unit --boot --link-journal=try-guest --network-veth -U --settings=override --machine=demo-nspawn-gitea01
root     1991712  0.0  0.0  13776  4004 ?        Ss   16:35   0:00  \_ (sd-stubinit)
root     1991713  0.0  0.0   2480   224 pts/0    Ss+  16:35   0:00      \_ /bin/sh start.sh
root     1991716  0.0  0.0  13336  7884 pts/0    S+   16:35   0:00          \_ sshd: /usr/sbin/sshd -D -e -f /etc/ssh/sshd_config [listener] 0 of 10-100 startups

and the resulted PID was the one for /bin/sh start.sh and not sshd :(

I'm not sure this will work correctly all the time. In a random test I got this process list : ``` root 1991710 0.0 0.0 13464 5788 ? Ss 16:35 0:00 systemd-nspawn --quiet --keep-unit --boot --link-journal=try-guest --network-veth -U --settings=override --machine=demo-nspawn-gitea01 root 1991712 0.0 0.0 13776 4004 ? Ss 16:35 0:00 \_ (sd-stubinit) root 1991713 0.0 0.0 2480 224 pts/0 Ss+ 16:35 0:00 \_ /bin/sh start.sh root 1991716 0.0 0.0 13336 7884 pts/0 S+ 16:35 0:00 \_ sshd: /usr/sbin/sshd -D -e -f /etc/ssh/sshd_config [listener] 0 of 10-100 startups ``` and the resulted PID was the one for `/bin/sh start.sh` and not `sshd` :(
Owner

Maybe you missed one of the two pgrep?

I checked this morning, it was okay.
But... yeah. The code isn't that clear, thats not helping :v

I've changed it in f4319c252d
I've changed the sshd script to exec, so it get replaced by sshd, and removed one pgrep accordingly

Maybe you missed one of the two pgrep? I checked this morning, it was okay. But... yeah. The code isn't that clear, thats not helping :v I've changed it in f4319c252dbfaa66a1000613821387db1fcc2505 I've changed the sshd script to exec, so it get replaced by sshd, and removed one pgrep accordingly
lpoujol added 1 commit 2024-06-28 10:02:33 +02:00
So we don't keep a sh process around.
Updated get_jail_sshd_pid accordingly with some comment
lpoujol added 3 commits 2024-07-01 10:55:59 +02:00
Owner

Would it be a good time to finaly rename the bkctld commands that are confusing (looking at "rm" command) ?

\cc @jlecour

Would it be a good time to finaly rename the bkctld commands that are confusing (looking at "rm" command) ? \cc @jlecour
lpoujol added 2 commits 2024-07-01 17:43:57 +02:00
lpoujol added 2 commits 2024-07-02 11:04:40 +02:00
lpoujol added 1 commit 2024-07-02 11:09:23 +02:00
lpoujol added 4 commits 2024-07-02 12:15:00 +02:00
lpoujol added 3 commits 2024-07-10 10:07:37 +02:00
ie: /data instead of /var/backup
-> Use systemctl show to fetch the jail pid
-> Use journalctl to grep in jail log
-> Adapt paths to fit the new directory structures
lpoujol added 7 commits 2024-07-15 18:11:27 +02:00
lpoujol added 6 commits 2024-08-02 10:31:11 +02:00
Owner

Okay.... this is it I guess.
While I have others idea in mind, I'll keep those for a next version as this one already introduce lots of changes.

I'm looking forward for a review/comments.

Okay.... this is it I guess. While I have others idea in mind, I'll keep those for a next version as this one already introduce lots of changes. I'm looking forward for a review/comments.
bwaegeneire requested changes 2024-08-09 12:05:29 +02:00
@ -58,0 +54,4 @@
echo -n "${output}" | grep -E "^UNKNOWN"
echo -n "${output}" | grep -E "^CRITICAL"
echo -n "${output}" | grep -E "^WARNING"
echo -n "${output}" | grep -E "^OK"
Owner

I'm not a fan of replacing printf with echo in general.

I'm not a fan of replacing printf with echo in general.
Owner

shellcheck wasn't happy with it (the intial form).
An alternative could be something like
printf "%s" "${output}" | grep -E "^UNKNOWN" that wouldn't trigger it

shellcheck wasn't happy with it (the intial form). An alternative could be something like `printf "%s" "${output}" | grep -E "^UNKNOWN"` that wouldn't trigger it
@ -77,0 +73,4 @@
echo -n "${output}" | grep -E "^UNKNOWN"
echo -n "${output}" | grep -E "^CRITICAL"
echo -n "${output}" | grep -E "^WARNING"
echo -n "${output}" | grep -E "^OK"
Owner

Same here about using echo instead of printf.

Same here about using echo instead of printf.
@ -92,0 +86,4 @@
echo -n "${output}" | grep -E "^UNKNOWN"
echo -n "${output}" | grep -E "^CRITICAL"
echo -n "${output}" | grep -E "^WARNING"
echo -n "${output}" | grep -E "^OK"
Owner

printf to echo :(

printf to echo :(
@ -0,0 +10,4 @@
jail_name="${1:?}"
if [ -z "${jail_name}" ]; then
show_help && exit 1
fi
Owner

Useless test, if ${1:?} is unset or null shell will fail by itself and won't execute the following test on ${jail_name}.

Useless test, if `${1:?}` is unset or null shell will fail by itself and won't execute the following test on `${jail_name}`.
Owner

It's the same pattern in other (unchanged) parts of the code.
Shall we replace it with only jail_name="${1}" , ensuring that the test would then be reached to display the error message & help ?

It's the same pattern in other (unchanged) parts of the code. Shall we replace it with only `jail_name="${1}" `, ensuring that the test would then be reached to display the error message & help ?
@ -0,0 +13,4 @@
fi
jail_path=$(jail_path "${jail_name}")
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
Owner

Redefining a function by a variable isn't a good idea. Since it's a single used variable, I suggest to just remove it and modify the test to be:

test -d "$(jail_path "${jail_name}")" || error "${jail_name}: jail not found" 2
Redefining a function by a variable isn't a good idea. Since it's a single used variable, I suggest to just remove it and modify the test to be: ``` sh test -d "$(jail_path "${jail_name}")" || error "${jail_name}: jail not found" 2 ```
Owner

It's used more than once.
But indeed we could replace all occurrences of the variable with the function call. Would it be better?

It's used more than once. But indeed we could replace all occurrences of the variable with the function call. Would it be better?
@ -19,3 +19,3 @@
port="${2}"
ip="${3}"
debug "Accept \`${ip}:${port}' for jail \`${jail_name}'"
debug "Accept '${ip}:${port}' for jail '${jail_name}'"
Owner

I would, I have like a separate PR for such stylistic changes peppered in this PR.

I would, I have like a separate PR for such stylistic changes peppered in this PR.
@ -17,2 +16,3 @@
jail_rootfs_path=$(jail_rootfs_path "${jail_name}")
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
test -d "${jail_rootfs_path}" || error "${jail_name}: jail rootfs not found" 2
Owner

It would be nice to have the error message printing the path that was tested, it simplifies debugging.

It would be nice to have the error message printing the path that was tested, it simplifies debugging.
@ -19,2 +19,3 @@
jail_sshd_config="${jail_path}/${SSHD_CONFIG}"
# Ensure we manipulate jails in the current/compatible version format
test "$(get_jail_version "${jail_name}")" -ne "${CURRENT_JAIL_VERSION}" && error "${jail_name}: jail needs to be uptated to version ${CURRENT_JAIL_VERSION} (currently $(get_jail_version "${jail_name}")) - Use bkctld convert-v2 <jail>" 3
Owner

This line is too long, it probably should be split.

This line is too long, it probably should be split.
@ -0,0 +11,4 @@
if [ -z "${jail_name}" ]; then
show_help && exit 1
fi
Owner

Dead test, just as the previous one.

Dead test, just as the previous one.
@ -0,0 +12,4 @@
if [ -z "${jail_name}" ]; then
show_help && exit 1
fi
jail_path=$(jail_path "${jail_name}")
Owner

Redefining the jail_path function by a variable :/

Redefining the `jail_path` function by a variable :/
Owner

Oh! Nice one :D

Oh! Nice one :D
@ -0,0 +14,4 @@
fi
jail_path=$(jail_path "${jail_name}")
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
Owner

Would be nice to have the path on the failing test.

Would be nice to have the path on the failing test.
@ -0,0 +16,4 @@
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
get_jail_version "${jail_name}"
Owner

Overall, this script in context with get_jail_version seems to be overly complicated, just to cat a version file in get_jail_version.

Overall, this script in context with `get_jail_version` seems to be overly complicated, just to cat a `version` file in `get_jail_version`.
@ -19,0 +18,4 @@
test -d "${jail_rootfs_path}" || error "${jail_name}: jail rootfs not found" 2
# Ensure we manipulate jails in the current/compatible version format
test "$(get_jail_version "${jail_name}")" -ne "${CURRENT_JAIL_VERSION}" && error "${jail_name}: jail needs to be uptated to version ${CURRENT_JAIL_VERSION} (currently $(get_jail_version "${jail_name}")) - Use bkctld convert-v2 <jail>" 3
Owner

Line too long.

Line too long.
@ -0,0 +10,4 @@
LIBDIR="$(dirname $0)" && . "${LIBDIR}/includes"
jail_name="${1:?}"
if [ -z "${jail_name}" ]; then
Owner

Dead test, like the others.

Dead test, like the others.
@ -0,0 +17,4 @@
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
journalctl --reverse --unit systemd-nspawn@"${jail_name}"
Owner

TIL --reverse the option I didn't knew I needed.

TIL `--reverse` the option I didn't knew I needed.
lpoujol marked this conversation as resolved
@ -16,6 +16,7 @@ if [ -z "${jail_name}" ] || [ -z "${new_jail_name}" ]; then
fi
jail_path=$(jail_path "${jail_name}")
#jail_rootfs_path=$(jail_rootfs_path "${jail_name}")
Owner

This variable assignation is commented, but it's used in the next lines.

This variable assignation is commented, but it's used in the next lines.
Owner

No. It's not. It's unused (that's why I commented it)

No. It's not. It's unused (that's why I commented it)
lpoujol marked this conversation as resolved
@ -20,2 +20,2 @@
# Prepare the chroot
mount_jail_fs "${jail_name}"
# Ensure we only start jails in the current/compatible version format
test "$(get_jail_version "${jail_name}")" -ne "${CURRENT_JAIL_VERSION}" && error "${jail_name}: jail needs to be updated to version ${CURRENT_JAIL_VERSION} (currently $(get_jail_version "${jail_name}")) - Use bkctld convert-v2 <jail>" 3
Owner

Line too long.

Line too long.
@ -14,3 +14,3 @@
[ -d "${JAILDIR}/${jail_name}" ] || error "${jail_name} : jail is missing.\nUse '$0 status [all]' to get the status of all jails."
incs_policy_file=$(current_jail_incs_policy_file ${jail_name})
test "$(get_jail_version "${jail_name}")" -ne "${CURRENT_JAIL_VERSION}" && error "${jail_name}: jail needs to be updated to version ${CURRENT_JAIL_VERSION} (currently $(get_jail_version "${jail_name}")) - Use bkctld convert-v2 <jail>" 3
Owner

Line too long.

Line too long.
@ -16,2 +16,4 @@
test -d "${jail_path}" || error "${jail_name}: jail not found" 2
# Ensure we manipulate jails in the current/compatible version format
test "$(get_jail_version "${jail_name}")" -ne "${CURRENT_JAIL_VERSION}" && error "${jail_name}: jail needs to be updated to version ${CURRENT_JAIL_VERSION} (currently $(get_jail_version "${jail_name}")) - Use bkctld convert-v2 <jail>" 3
Owner

Line too long.

Line too long.
@ -20,0 +19,4 @@
[ -n "${NODE}" ] || error "Sync need config for \$NODE variable in /etc/default/bkctld !"
# Ensure the same version of bkctld runs on the other node
node_version=$(ssh "${NODE}" grep "^VERSION=" "${LIBDIR}/includes" | cut -d'=' -f2 | sed 's/"//g')
Owner

sed 's/"//g' would not work if someone would replace " with '.

`sed 's/"//g'` would not work if someone would replace `"` with `'`.
Owner

Will replace it with | tr -d "'\""

Will replace it with `| tr -d "'\""`
@ -8,2 +8,3 @@
VERSION="22.11"
VERSION="24.08"
CURRENT_JAIL_VERSION="2"
Owner

Can't we just use VERSION for that?

Can't we just use `VERSION` for that?
Owner

We could. It felt easier/simpler to have a separate numbering scheme.

Tomorrow, 24.09 without any changes in the jail structure, it would stay in jail v2 mode.
So no need to handle the jail upgrade

We could. It felt easier/simpler to have a separate numbering scheme. Tomorrow, 24.09 without any changes in the jail structure, it would stay in jail v2 mode. So no need to handle the jail upgrade
lpoujol changed title from WIP: Use systemd-nspawn instead of chroot to Use systemd-nspawn instead of chroot 2024-08-28 16:38:16 +02:00
lpoujol force-pushed server-nspawn from 3dcc0bc01a to 7f5fe8effa 2024-08-28 17:15:54 +02:00 Compare
lpoujol merged commit d44be2f567 into master 2024-08-28 17:16:40 +02:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/evobackup#74
No description provided.