Remove lockc-runc-wrapper #93

vadorovsky · 2021-11-04T19:04:12Z

This change consists of several changes which overall allow to remove
lockc-runc-wrapper and any necessity of configuring containerd for
integration with lockc:

Using fanotify to monitor runc processes and register containers.
Using uprobes for registering containers and processes in BPF maps.
Doing BPF operations from the wrapper had the weird issue - by
registering the wrapper process, we were in fact preventing the same
lockc-runc-wrapper process from doing any more BPF map modifications.
Using uprobes has also an another advantage - it will allow rootless
containers to work.
Allowing access to /run directory inside container filesystem. It's
used both by popular applications (like nginx) and by kubelet to store
the network namespace (/run/netns).
Adding containerd v2 CRI-related cgroup mount directories as allowed.
Proper cleanup of old containers and processes.

Majority of uprobes code on the userspace side comes from
bpfcontain-rs.

Fixes: #94
Fixes: #52
Fixes: #92
Signed-off-by: Michal Rostecki [email protected]
Signed-off-by: William Findlay [email protected]

willfindlay

LGTM overall. Just a few general requests from me, but nothing prohibitive.

lockc/src/bpf/lockc.bpf.c

lockc/src/uprobe_ext.rs

willfindlay · 2021-11-04T20:27:40Z

Hmm, CI appears to be failing? Have you bumped the libbpf-rs version lately?

vadorovsky · 2021-11-05T08:08:44Z

I can't reproduce the CI failure locally with the same dapper cargo build command, weird. :/ But indeed it looks as some internal problem with libbpf-rs and libbpf-sys.

willfindlay · 2021-11-06T05:03:41Z

I can't reproduce the CI failure locally with the same dapper cargo build command, weird. :/ But indeed it looks as some internal problem with libbpf-rs and libbpf-sys.

I've also had similar problems where libbpf-rs fails to build that were basically about a mismatch between libbpf-sys and the system libbpf. Are you using the vendored libbpf or system libbpf? I believe it's gated with a feature flag in libbpf-rs.

vadorovsky · 2021-11-17T15:41:04Z

@willfindlay @mjura @flavio It's ready for review. Sorry that it took me so long and that this pull request is so huge., but I wanted to make sure that everything works and is documented.

Reviewing it per commit should be easier than looking at the full diff.

mjura

LGTM, let's merge this change and then add more features

flavio

LGTM, but I don't have enough knowledge to understand the low level parts.

I left some minor comments

contrib/terraform/libvirt/cloud-init/common.tpl

docs/src/architecture.md

docs/src/build/README.md

docs/src/install/docker.md

docs/src/install/kubernetes.md

docs/src/terraform/libvirt.md

lockc/Cargo.toml

lockc/src/bin/lockcd.rs

nickgerace

Great PR! This is one the biggest ones I've seen in awhile lol.

nickgerace · 2021-11-18T17:06:04Z

.github/workflows/container-image.yml

+          password: ${{ secrets.GITHUB_TOKEN }}
+      -
+        name: Build and push development container image
+        if: ${{ startsWith(github.ref, 'refs/heads/') }}


Is this conditional necessary? I think it's for tags only, which would make it necessary, but I am trying to discern what this does 😅 . Probably my own misunderstanding.

I will clarify with him later.

nickgerace · 2021-11-18T17:07:55Z

contrib/terraform/libvirt/deploy-kubernetes.sh

+if [ -z "${TR_MASTER_IPS}" ] || [ -z "${TR_USERNAME}" ]; then
+    error '$TR_MASTER_IPS $TR_USERNAME must be specified'
+    exit 1
+fi
+
+sleep 5
+
+CILIUM_VERSION=$(curl -s https://api.github.com/repos/cilium/cilium/releases/latest | jq -r '.tag_name' | sed -e 's/^v//')
+
+info "### Run following commands to bootstrap Kubernetes cluster:\\n"
+
+i=0
+for MASTER in $TR_MASTER_IPS; do
+    if [ $i -eq "0" ]; then
+        ssh -o 'StrictHostKeyChecking no' -l ${TR_USERNAME} ${MASTER} /bin/bash <<-EOF
+          set -eux
+          sudo kubeadm init --cri-socket /run/containerd/containerd.sock --control-plane-endpoint ${MASTER}:6443 --upload-certs | tee kubeadm-init.log
+          mkdir -p /home/${TR_USERNAME}/.kube
+          sudo cp /etc/kubernetes/admin.conf /home/${TR_USERNAME}/.kube/config
+          sudo chown ${TR_USERNAME}:users /home/${TR_USERNAME}/.kube/config
+          helm repo add cilium https://helm.cilium.io/
+          helm install cilium cilium/cilium --version ${CILIUM_VERSION} --namespace kube-system
+EOF
+
+        export KUBEADM_MASTER_JOIN=`ssh -o 'StrictHostKeyChecking no' -l ${TR_USERNAME} ${MASTER} tail -n12 kubeadm-init.log | head -n3`
+        export KUBEADM_WORKER_JOIN=`ssh -o 'StrictHostKeyChecking no' -l ${TR_USERNAME} ${MASTER} tail -n2 kubeadm-init.log`
+    else
+        ssh -o 'StrictHostKeyChecking no' -l ${TR_USERNAME} ${MASTER} /bin/bash <<-EOF
+          set -eux
+          sudo ${KUBEADM_MASTER_JOIN}
+          mkdir -p /home/${TR_USERNAME}/.kube
+          sudo cp /etc/kubernetes/admin.conf /home/${TR_USERNAME}/.kube/config
+          sudo chown ${TR_USERNAME}:users /home/${TR_USERNAME}/.kube/config
+EOF
+    fi
+    ((++i))
+done


Does this get executed in cloud-init for terraform? Curious what happens when it exits with a non-zero status code.

As there is set -eux everywhere, any non-zero status code in any line should fail the script and then the terraform provisioner should fail as well. Deployment should stop with an error.

Anyway, I'm not proud of that script, I would be happy if some more devops-oriented person could help with having some proper salt formulas or anything which would hurt eyes less. Or I might eventually do that in future.

This is for development-only right? If so, I think this is more than fine enough for now. Looks good to me!

Yes, that's only for development. On all the other clusters, lockc is going to be installed with helm.

nickgerace · 2021-11-18T17:09:33Z

docs/src/install/docker.md

+with Docker. In order to do that, we need to install `lockcd` binary and a
+systemd unit for it.


Side note: is there a lockcd rpm?

Not yet, but I'm thinking about it. I would wait for #49 though - removing libbpf and LLVM dependency will make packaging much easier. I already started working on aya rewrite, I'm hoping to have some PR next week.

nickgerace · 2021-11-18T17:10:48Z

lockc/src/bin/lockcd.rs

+    let log_level = match env::var("LOCKC_DEBUG") {
+        Ok(_) => LevelFilter::Debug,
+        Err(_) => LevelFilter::Info,
+    };


I assume this is a boolean setter, not a level?

I was assuming it's a level. Was my assumption wrong?

I ask because I think you always set the enum to either Debug or Info based on whether or not that ENV's value is non-empty or exists. Thus, it seems like it behaves like a bool (true/false for debug mode)

Yes, exposing it as a bool env variable was my intention.

nickgerace · 2021-11-18T17:14:55Z

lockc/src/runc.rs

+        if annotations.contains_key(ANNOTATION_CONTAINERD_LOG_DIRECTORY) {
+            // containerd doesn't expose k8s namespaces directly. They have
+            // to be parsed from the log directory path, where the first
+            // part of the filename is the namespace.
+            let log_directory = &annotations[ANNOTATION_CONTAINERD_LOG_DIRECTORY];
+            debug!(
+                "detected k8s+containerd container with log directory {}",
+                log_directory
+            );
+            let log_path = std::path::PathBuf::from(log_directory);
+            let file_name = log_path.file_name().unwrap().to_str().unwrap();
+            let mut splitter = file_name.split('_');
+            let namespace = splitter.next().unwrap().to_string();
+
+            return Ok((ContainerType::KubernetesContainerd, namespace));
+        // containerd
+        } else if annotations.contains_key(ANNOTATION_CONTAINERD_SANDBOX_ID) {


For readability, it could be a good idea to split the contains key checks into another function and then return an enum (or other primitive) to match off of. This is purely something to think about and I'd be fine with keeping it as is.

Good idea, I will try

nickgerace · 2021-11-18T17:15:37Z

lockc/src/runc.rs

+        }
+    }
+
+    Ok((ContainerType::Unknown, String::from("")))


Containing an Option<String> as value None instead of empty String could be better depending on your preference

nickgerace · 2021-11-18T17:18:04Z

lockc/src/runc.rs

+                }
+            }
+            ContainerAction::Create => {
+                let container_id = container_id_o.unwrap();


Are we ok with the risk of unwrapping here?

Every runc create and runc delete command has to come with container ID. I never got any panic here. But on the other hand, maybe returning some descriptive error could be better here, just in case...

Whatever you think is best! I usually like to wrap an error or return it since panic traces can be a bit harder to deal with

nickgerace · 2021-11-18T17:18:15Z

lockc/src/runc.rs

+                check_uprobe_ret(ret)?;
+            }
+            ContainerAction::Delete => {
+                let container_id = container_id_o.unwrap();


Same as above

lockc/src/runc.rs

This change consists of several changes which overall allow to remove lockc-runc-wrapper and any necessity of configuring containerd for integration with lockc: * Using fanotify to monitor runc processes and register containers. * Using uprobes for registering containers and processes in BPF maps. Doing BPF operations from the wrapper had the weird issue - by registering the wrapper process, we were in fact preventing the same lockc-runc-wrapper process from doing any more BPF map modifications. Using uprobes has also an another advantage - it will allow rootless containers to work. * Allowing access to /run directory inside container filesystem. It's used both by popular applications (like nginx) and by kubelet to store the network namespace (/run/netns). * Adding containerd v2 CRI-related cgroup mount directories as allowed. * Proper cleanup of old containers and processes. Majority of uprobes code on the userspace side comes from bpfcontain-rs. Fixes: lockc-project#94 Fixes: lockc-project#52 Fixes: lockc-project#92 Signed-off-by: Michal Rostecki <[email protected]> Signed-off-by: William Findlay <[email protected]>

We've observed issues and long timeouts while building the VM image with guestfs. Using just terraform for provisioning seems to be faster. Signed-off-by: Michal Rostecki <[email protected]> Signed-off-by: Michal Jura <[email protected]>

This change adds a multi-stage Dockerfile which is used both for building the project and for the final VM image. Signed-off-by: Michal Rostecki <[email protected]>

Signed-off-by: Michal Jura <[email protected]> Signed-off-by: Michal Rostecki <[email protected]>

Enable buildx and everything necessary for plugins to work. Signed-off-by: Michal Rostecki <[email protected]>

To make templating of unit files working with DESTDIR, we need to differentiate between * full bindir, unitdir, sysconfdir - which contains DESTDIR prefix for installation of target files; DESTDIR is something using by RPM packers or producing tarballs to move targets somewhere else in the filesystem before making a package * relative bindir, unitdir, sysconfdir - which doesn't contain DESTDIR prefix and is used to template values like `{{ bindir }}` into real values; those should be relative towards DESTDIR Signed-off-by: Michal Rostecki <[email protected]>

Add a new subcommand `bintar` which produces a .tar.gz archive with lockcd binary and systemd unit. It's going to be used as a release artifact and as a way of distributing lockc to virtual machines when deploying without Kubernetes. Signed-off-by: Michal Rostecki <[email protected]>

To make them easier to grep. Signed-off-by: Michal Rostecki <[email protected]>

That should fix problems with showing images in Github Pages. Signed-off-by: Michal Rostecki <[email protected]>

Signed-off-by: Michal Rostecki <[email protected]>

Signed-off-by: Michal Jura <[email protected]>

vadorovsky · 2021-11-19T01:11:23Z

Comments should be addressed now. Thanks everyone for review!

willfindlay requested changes Nov 4, 2021

View reviewed changes

lockc/src/bpf/lockc.bpf.c Show resolved Hide resolved

lockc/src/uprobe_ext.rs Outdated Show resolved Hide resolved

lockc/src/uprobe_ext.rs Show resolved Hide resolved

vadorovsky force-pushed the fix-dir branch 2 times, most recently from f811483 to 1aba170 Compare November 5, 2021 09:18

vadorovsky force-pushed the fix-dir branch 2 times, most recently from dc2110b to b18968b Compare November 8, 2021 15:53

vadorovsky changed the title ~~bpf: Fix problems with container starts~~ Remove lockc-runc-wrapper Nov 8, 2021

vadorovsky force-pushed the fix-dir branch 12 times, most recently from 8e8f873 to 2c6b259 Compare November 11, 2021 08:28

vadorovsky force-pushed the fix-dir branch 2 times, most recently from bad9717 to a92d23f Compare November 15, 2021 18:35

vadorovsky mentioned this pull request Nov 15, 2021

Deploy lockc as a DeamonSet #91

Closed

vadorovsky force-pushed the fix-dir branch 6 times, most recently from 28725cd to 2da88b2 Compare November 16, 2021 18:40

vadorovsky force-pushed the fix-dir branch from 2da88b2 to 6a39119 Compare November 17, 2021 15:39

vadorovsky marked this pull request as ready for review November 17, 2021 15:39

vadorovsky requested review from mjura and flavio November 17, 2021 15:41

vadorovsky force-pushed the fix-dir branch from 6a39119 to b08628b Compare November 18, 2021 07:21

mjura approved these changes Nov 18, 2021

View reviewed changes

vadorovsky force-pushed the fix-dir branch 2 times, most recently from 7dce1b9 to 65f427f Compare November 18, 2021 07:41

vadorovsky requested a review from nickgerace November 18, 2021 13:22

flavio approved these changes Nov 18, 2021

View reviewed changes

nickgerace reviewed Nov 18, 2021

View reviewed changes

vadorovsky force-pushed the fix-dir branch from 65f427f to 8b3a286 Compare November 18, 2021 17:43

willfindlay requested changes Nov 18, 2021

View reviewed changes

lockc/src/runc.rs Outdated Show resolved Hide resolved

vadorovsky and others added 11 commits November 19, 2021 01:42

docker: Add Dockerfile, base dapper on it

f509c12

This change adds a multi-stage Dockerfile which is used both for building the project and for the final VM image. Signed-off-by: Michal Rostecki <[email protected]>

Add lockc helm chart

73183fb

Signed-off-by: Michal Jura <[email protected]> Signed-off-by: Michal Rostecki <[email protected]>

github: Add missing environment variables for Docker

a1dbbb2

Enable buildx and everything necessary for plugins to work. Signed-off-by: Michal Rostecki <[email protected]>

examples: Add success suffix to deployment names

82093b1

To make them easier to grep. Signed-off-by: Michal Rostecki <[email protected]>

docs: Set site-url

50312e0

That should fix problems with showing images in Github Pages. Signed-off-by: Michal Rostecki <[email protected]>

docs: Update

c28e925

Signed-off-by: Michal Rostecki <[email protected]>

Build container image for lockc on ghcr.io

045ae87

Signed-off-by: Michal Jura <[email protected]>

vadorovsky force-pushed the fix-dir branch from 8b3a286 to 045ae87 Compare November 19, 2021 00:42

vadorovsky merged commit 77bf7e6 into lockc-project:main Nov 19, 2021

vadorovsky deleted the fix-dir branch November 19, 2021 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove lockc-runc-wrapper #93

Remove lockc-runc-wrapper #93

vadorovsky commented Nov 4, 2021 •

edited

Loading

willfindlay left a comment

willfindlay commented Nov 4, 2021

vadorovsky commented Nov 5, 2021

willfindlay commented Nov 6, 2021

vadorovsky commented Nov 17, 2021

mjura left a comment

flavio left a comment

nickgerace left a comment

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

vadorovsky Nov 19, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

vadorovsky Nov 18, 2021 •

edited

Loading

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

nickgerace Nov 18, 2021

nickgerace Nov 18, 2021

vadorovsky Nov 18, 2021

vadorovsky commented Nov 19, 2021

		with Docker. In order to do that, we need to install `lockcd` binary and a
		systemd unit for it.

Remove lockc-runc-wrapper #93

Remove lockc-runc-wrapper #93

Conversation

vadorovsky commented Nov 4, 2021 • edited Loading

willfindlay left a comment

Choose a reason for hiding this comment

willfindlay commented Nov 4, 2021

vadorovsky commented Nov 5, 2021

willfindlay commented Nov 6, 2021

vadorovsky commented Nov 17, 2021

mjura left a comment

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

nickgerace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadorovsky Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadorovsky commented Nov 19, 2021

vadorovsky commented Nov 4, 2021 •

edited

Loading

vadorovsky Nov 18, 2021 •

edited

Loading