Remove lockc-runc-wrapper by vadorovsky · Pull Request #93 · lockc-project/lockc

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 mrostecki@opensuse.org
Signed-off-by: William Findlay william@williamfindlay.com

willfindlay

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

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

nickgerace

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

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

+          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

+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

+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

+    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

+        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

+        }
+    }
+
+    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

+                }
+            }
+            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

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


Same as above

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 <mrostecki@opensuse.org> Signed-off-by: William Findlay <william@williamfindlay.com>

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 <mrostecki@opensuse.org> Signed-off-by: Michal Jura <mjura@suse.com>

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 <mrostecki@opensuse.org>

Signed-off-by: Michal Jura <mjura@suse.com> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Enable buildx and everything necessary for plugins to work. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

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 <mrostecki@opensuse.org>

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 <mrostecki@opensuse.org>

To make them easier to grep. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

That should fix problems with showing images in Github Pages. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Signed-off-by: Michal Jura <mjura@suse.com>

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

Comments should be addressed now. Thanks everyone for review!

willfindlay requested changes Nov 4, 2021

View reviewed changes

Comment thread lockc/src/bpf/lockc.bpf.c

Comment thread lockc/src/uprobe_ext.rs

Comment thread lockc/src/uprobe_ext.rs

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 flavio and mjura 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

Comment thread lockc/src/runc.rs Outdated

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 <mrostecki@opensuse.org>

Add lockc helm chart

73183fb

Signed-off-by: Michal Jura <mjura@suse.com> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

github: Add missing environment variables for Docker

a1dbbb2

Enable buildx and everything necessary for plugins to work. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

examples: Add success suffix to deployment names

82093b1

To make them easier to grep. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

docs: Set site-url

50312e0

That should fix problems with showing images in Github Pages. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

docs: Update

c28e925

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Build container image for lockc on ghcr.io

045ae87

Signed-off-by: Michal Jura <mjura@suse.com>

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

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

Conversation

vadorovsky commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willfindlay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willfindlay commented Nov 4, 2021

Uh oh!

vadorovsky commented Nov 5, 2021

Uh oh!

willfindlay commented Nov 6, 2021

Uh oh!

vadorovsky commented Nov 17, 2021

Uh oh!

mjura left a comment

Choose a reason for hiding this comment

Uh oh!

flavio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickgerace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadorovsky Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vadorovsky commented Nov 4, 2021 •

edited

Loading

vadorovsky Nov 18, 2021 •

edited

Loading