Skip to content

Split pods/containers into separate trees in quark_queue - #380

Open
Tacklebox wants to merge 2 commits into
mainfrom
oci-trees-structure
Open

Tacklebox wants to merge 2 commits into
mainfrom
oci-trees-structure

Conversation

@Tacklebox

@Tacklebox Tacklebox commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

Pods and containers need to be accessible independently of Kubernetes state. This change moves the pod index from quark_kube to quark_queue, alongside the existing container index, and adds public pod/container get and lookup helpers.

quark_container_get() supports creating a container without a pod and attaching it later. Repeated calls preserve the existing object; requesting a different parent for an attached container fails with EEXIST. Queue shutdown frees all pods, their attached containers, and any remaining orphan containers.

Independent container garbage collection and public pod/container removal APIs are implemented in follow-up PR #383. That PR adds GC_CONTAINER, container GC queue linkage and cleanup, and C/Go removal helpers that schedule collection after the queue's cache grace time. In this PR, containers attached to a pod are freed with that pod; orphan containers are freed when the queue closes.

Validation on the local checkout, including the review fixes:

  • make ARCH=arm64 docker passed using the repository Dockerfile.
  • t_pod_get and t_container_get passed in the Linux builder container.
  • git diff --check passed.

The Kubernetes integration test was not run.

@Tacklebox
Tacklebox requested a review from a team as a code owner August 12, 2026 09:54
@Tacklebox
Tacklebox force-pushed the oci-trees-structure branch from 24c8711 to 499635f Compare August 12, 2026 09:56
@nicholasberlin

Copy link
Copy Markdown
Contributor

PR Review: #380 — Split pods/containers into separate trees in quark_queue

Author: Tacklebox
Branch: oci-trees-structure -> main
Commits: 1 | Files changed: 2 | +116 -11
URL: #380
Reviewed commit: 499635f
Reviewed by: OpenAI GPT-5

Summary

This change moves the pod index onto quark_queue, makes pod/container lookup available without Kubernetes state, adds public get/lookup helpers, and cleans up queue-owned pods and orphan containers on close. The ownership move itself is coherent, but the new container getter does not honor its documented parent-link behavior for existing containers, and the reviewed commit does not contain the independent container-GC work claimed by the PR description.

Findings

Significant

  1. Existing containers are returned without applying or validating pod_uid (quark.c:2402).

    quark_container_get() resolves the requested pod, but then immediately returns when container_lookup() finds an existing container. A supported call sequence such as quark_container_get(qq, "id", NULL) followed later by quark_container_get(qq, "id", "pod-uid") therefore leaves the container orphaned forever, despite the function's contract saying that a non-NULL pod_uid links the container to that pod. The same path silently returns a container already linked to a different pod. Because there is no other public attachment operation, callers cannot repair either state. When an existing container has no parent, link it into the requested pod; when it already belongs to a different pod, return a defined error such as EEXIST or EINVAL.

  2. The commit does not implement the independent container garbage collection described as part of this PR (quark.h:571, quark.h:695, quark.c:473, quark.c:789).

    At the reviewed head, enum gc_type has no GC_CONTAINER, struct quark_container has no leading gc_link, gc_collect() has no container case, and container_delete() cannot unlink a queued container. Those are all explicitly listed in the PR summary as the structural foundation being delivered. They currently appear only in a later draft PR in the stack. Either include that foundation here or update this PR's scope and description so reviewers and merge sequencing reflect what this commit actually provides.

Minor

  1. The comments still describe the moved trees as belonging to quark_kube (quark.h:712, quark.h:736).

    After this change both global indexes live in quark_queue, so these comments now document the wrong owner and preserve Kubernetes-specific naming for a queue-wide data structure.

Verdict

REQUEST_CHANGES — fix the existing-container parent-link path before exposing quark_container_get(), and reconcile the implementation with the PR's stated container-GC scope. The ownership relocation and close-time cleanup otherwise look sound.

Verification

  • git diff --check origin/main...HEAD passed.
  • GitHub reports successful buildkite/quark and CLA statuses for the reviewed commit.
  • make libquark_big.a could not be validated locally on macOS because vendored libelf fails first on Clang signedness warnings promoted to errors; the identical failure occurs at the base commit d795343d147d0120b7c7a8e59b76a833f4b8b812, before compiling the PR's changed files.

Comment thread quark.c
Comment thread quark.c
Comment thread quark.c
@biscout42
biscout42 force-pushed the oci-trees-structure branch 2 times, most recently from 56cf25f to b028731 Compare September 16, 2026 11:38
biscout42 added a commit that referenced this pull request Sep 16, 2026
Permit a later pod link for a container without a pod.
Reject a different parent pod.
Add pod and container tests.
Correct the index ownership comments.

The Docker build passed.
The pod and container tests passed on Linux.
biscout42
biscout42 previously approved these changes Sep 16, 2026

@biscout42 biscout42 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nicholasberlin , I have addressed all the comments and changed PR description (because GC is implemented in the follow-up PR).

Could you double-check the changes?

biscout42 added a commit that referenced this pull request Sep 16, 2026
Permit a later pod link for a container without a pod.
Reject a different parent pod.
Add pod and container tests.
Correct the index ownership comments.

The Docker build passed.
The pod and container tests passed on Linux.
biscout42 added a commit that referenced this pull request Sep 21, 2026
Permit a later pod link for a container without a pod.
Reject a different parent pod.
Add pod and container tests.
Correct the index ownership comments.

The Docker build passed.
The pod and container tests passed on Linux.
@biscout42
biscout42 force-pushed the oci-trees-structure branch 3 times, most recently from 7814c75 to a8e55b7 Compare September 24, 2026 11:29
biscout42 added a commit that referenced this pull request Sep 24, 2026
Permit a later pod link for a container without a pod.
Reject a different parent pod.
Add pod and container tests.
Correct the index ownership comments.

The Docker build passed.
The pod and container tests passed on Linux.
@biscout42
biscout42 force-pushed the oci-trees-structure branch 2 times, most recently from e3a3ab3 to f448b14 Compare September 25, 2026 08:29
Maxwell Borden and others added 2 commits September 25, 2026 19:17
Permit a later pod link for a container without a pod.
Reject a different parent pod.
Add pod and container tests.
Correct the index ownership comments.

The Docker build passed.
The pod and container tests passed on Linux.
@nicholasberlin

Copy link
Copy Markdown
Contributor

PR Review: #380 — Split pods/containers into separate trees in quark_queue

Author: Tacklebox
Branch: oci-trees-structure -> main
Commits: 2 | Files changed: 3 | +236 -16
URL: #380
Reviewed commit: 579ebab
Reviewed by: GPT-6

Summary

The PR moves pod ownership into quark_queue, exposes independent pod/container get and lookup helpers, and frees both attached and orphan containers on queue shutdown. The previous review's late-attachment and conflicting-parent concerns are fixed, ownership comments are corrected, and the description now accurately defers independent container GC to PR #383. One integration issue remains when the new API and the Kubernetes watcher populate the same queue.

Findings

Significant

  1. [P2] Reconcile API-created orphan containers when Kubernetes discovers their pod — quark.c:2525–2532 (new global insertion and optional parent), interacting with kube_handle_container() at quark.c:1053–1093.

    With Kubernetes enabled, call quark_container_get(qq, "containerd://id", NULL) before the watcher reports that container's pod. This is a valid orphan creation even if the caller fully populates its metadata. When the Kubernetes update arrives, kube_handle_container() searches only pod->containers, allocates a second container, then fails its global insertion with unexpected container collision 1. It never attaches the existing orphan. Every subsequent update repeats the collision, so processes linked to the existing container continue to lack pod metadata, and pod deletion does not collect that container.

    Reconcile the watcher path with the queue-wide index: reuse an existing orphan and attach it to the discovered pod, preserving its identity and process links, while rejecting genuinely conflicting parents. Add coverage for API creation followed by Kubernetes ingestion. Simply calling the new getter from the watcher also requires handling the fact that kube_handle_pod() currently inserts a newly discovered pod only after processing its containers.

Verdict

REQUEST_CHANGES — the prior findings are resolved, but Kubernetes ingestion needs to accept the orphan state now supported by the public API.

@nicholasberlin

Copy link
Copy Markdown
Contributor
--- a/quark.c
+++ b/quark.c
@@ -874,7 +874,23 @@
 	return (k);
 }
 
+/* The pod need not be indexed yet while handling a Kubernetes update. */
 static int
+container_attach_pod(struct quark_container *container, struct quark_pod *pod)
+{
+	if (pod == NULL || container->pod == pod)
+		return (0);
+	if (container->pod != NULL)
+		return (errno = EEXIST, -1);
+	if (pod_containers_RB_INSERT(&pod->containers, container) != NULL)
+		return (errno = EEXIST, -1);
+	container->pod = pod;
+	container->linked_by_pod = 1;
+
+	return (0);
+}
+
+static int
 pod_by_uid_cmp(struct quark_pod *a, struct quark_pod *b)
 {
 	return (strcmp(a->uid, b->uid));
@@ -947,19 +963,6 @@
 	free(pod);
 }
 
-static struct quark_container *
-pod_lookup_container(struct quark_pod *pod, char *container_id)
-{
-	struct quark_container key, *k;
-
-	key.container_id = container_id;
-	k = pod_containers_RB_FIND(&pod->containers, &key);
-	if (k == NULL)
-		errno = ESRCH;
-
-	return (k);
-}
-
 static void
 debug_json(cJSON *json)
 {
@@ -1050,7 +1053,7 @@
 		qwarnx("unknown container state, ignoring");
 		return (-1);
 	}
-	container = pod_lookup_container(pod, containerID->valuestring);
+	container = container_lookup(qq, containerID->valuestring);
 	if (container == NULL) {
 		container = calloc(1, sizeof(*container));
 		if (container == NULL)
@@ -1093,18 +1096,12 @@
 			return (-1);
 		}
 		container->linked_by_id = 1;
-		if (pod != NULL) {
-			container->pod = pod;
-			col = pod_containers_RB_INSERT(&pod->containers,
-			    container);
-			if (col != NULL) {
-				qwarnx("unexpected container collision 2");
-				container_delete(qq, container);
-				return (-1);
-			}
-			container->linked_by_pod = 1;
+		if (container_attach_pod(container, pod) == -1) {
+			container_delete(qq, container);
+			return (-1);
 		}
-	}
+	} else if (container_attach_pod(container, pod) == -1)
+		return (-1);
 
 	return (0);
 #undef GET
@@ -2500,15 +2497,8 @@
 
 	container = container_lookup(qq, (char *)container_id);
 	if (container != NULL) {
-		if (pod != NULL && container->pod != pod) {
-			if (container->pod != NULL)
-				return (errno = EEXIST, NULL);
-			col = pod_containers_RB_INSERT(&pod->containers, container);
-			if (unlikely(col != NULL))
-				return (errno = EEXIST, NULL);
-			container->pod = pod;
-			container->linked_by_pod = 1;
-		}
+		if (container_attach_pod(container, pod) == -1)
+			return (NULL);
 		return (container);
 	}
 
@@ -2529,14 +2519,9 @@
 	}
 	container->linked_by_id = 1;
 
-	if (pod != NULL) {
-		container->pod = pod;
-		col = pod_containers_RB_INSERT(&pod->containers, container);
-		if (unlikely(col != NULL)) {
-			container_delete(qq, container);
-			return (errno = EEXIST, NULL);
-		}
-		container->linked_by_pod = 1;
+	if (container_attach_pod(container, pod) == -1) {
+		container_delete(qq, container);
+		return (NULL);
 	}
 
 	return (container);

@nicholasberlin

Copy link
Copy Markdown
Contributor
--- a/quark.c
+++ b/quark.c
@@ -1502,7 +1502,7 @@
 /*
  * Returns 0 if there's nothing else to parse, 1 otherwise
  */
-static int
+int
 kube_parse_events(struct quark_queue *qq)
 {
 #define GET cJSON_GetObjectItemCaseSensitive
--- a/quark.h
+++ b/quark.h
@@ -117,6 +117,7 @@
 			     struct raw_event *, struct raw_event *);
 
 /* quark.c: These are exported for testing only */
+int	 kube_parse_events(struct quark_queue *);
 int	 parse_container_cgroup(const char *, char *, size_t);
 
 /* btf.c */
--- a/quark-test.c
+++ b/quark-test.c
@@ -3540,6 +3540,110 @@
 	quark_queue_close(&qq);
 	assert(RB_EMPTY(&qq.pod_by_uid));
 	assert(RB_EMPTY(&qq.container_by_id));
+
+	return (0);
+}
+
+static void
+kube_feed_pod(struct quark_queue *qq, const char *uid, const char *container_id)
+{
+	struct quark_kube	*qkube = qq->qkube;
+	char			 json[1024];
+	u32			 len;
+	int			 n;
+
+	n = snprintf(json, sizeof(json),
+	    "{\"kind\":\"Pod\","
+	    "\"metadata\":{\"name\":\"test\",\"namespace\":\"default\",\"uid\":\"%s\"},"
+	    "\"spec\":{\"containers\":[]},"
+	    "\"status\":{\"phase\":\"Running\",\"containerStatuses\":[{"
+	    "\"name\":\"test\",\"image\":\"registry/image:tag\","
+	    "\"imageID\":\"registry/image@sha256:123\","
+	    "\"containerID\":\"%s\",\"state\":{\"running\":{}}}]}}",
+	    uid, container_id);
+	assert(n > 0 && (size_t)n < sizeof(json));
+	len = n;
+	assert(qkube->buf_r == 0 && qkube->buf_w == 0);
+	assert(sizeof(len) + len <= qkube->buf_len);
+	memcpy(qkube->buf, &len, sizeof(len));
+	memcpy(qkube->buf + sizeof(len), json, len);
+	qkube->buf_w = sizeof(len) + len;
+	assert(kube_parse_events(qq) == 1);
+	assert(kube_parse_events(qq) == 0);
+}
+
+static int
+t_kube_container_attach(const struct test *t, struct quark_queue_attr *qa)
+{
+	struct quark_queue	 qq;
+	struct quark_process	 process;
+	struct quark_container	*container;
+	const struct quark_container *fresh;
+	const struct quark_pod	*pod, *other;
+	int			 existing_pod;
+
+	/* Test both a newly discovered pod and one already in the index. */
+	for (existing_pod = 0; existing_pod <= 1; existing_pod++) {
+		bzero(&qq, sizeof(qq));
+		qq.epollfd = -1;
+		TAILQ_INIT(&qq.event_gc);
+		qq.qkube = calloc(1, sizeof(*qq.qkube));
+		assert(qq.qkube != NULL);
+		qq.qkube->fd = -1;
+		qq.qkube->buf_len = 2048;
+		qq.qkube->buf = malloc(qq.qkube->buf_len);
+		assert(qq.qkube->buf != NULL);
+		if (existing_pod)
+			assert(quark_pod_get(&qq, "pod") != NULL);
+
+		container = quark_container_get(&qq, "containerd://id", NULL);
+		assert(container != NULL);
+		container->name = strdup("external-name");
+		container->image = strdup("registry/image:tag");
+		container->image_id = strdup("registry/image@sha256:123");
+		assert(container->name != NULL && container->image != NULL &&
+		    container->image_id != NULL);
+		bzero(&process, sizeof(process));
+		process.container = container;
+		TAILQ_INSERT_TAIL(&container->processes, &process, entry_container);
+
+		/* Watcher updates must reuse and attach the API-created orphan. */
+		kube_feed_pod(&qq, "pod", "containerd://id");
+		pod = quark_pod_lookup(&qq, "pod");
+		assert(pod != NULL);
+		assert(container->pod == pod && container->linked_by_pod);
+		assert(RB_ROOT(&pod->containers) == container);
+		assert(quark_container_lookup(&qq, "containerd://id") == container);
+		assert(strcmp(container->name, "external-name") == 0);
+		assert(process.container == container);
+		assert(TAILQ_FIRST(&container->processes) == &process);
+
+		/* Repeated updates must not duplicate either linkage. */
+		kube_feed_pod(&qq, "pod", "containerd://id");
+		assert(quark_container_lookup(&qq, "containerd://id") == container);
+		assert(container->pod == pod && RB_ROOT(&pod->containers) == container);
+		assert(process.container == container);
+		assert(TAILQ_FIRST(&container->processes) == &process);
+		assert(TAILQ_NEXT(&process, entry_container) == NULL);
+
+		/* A conflicting watcher update must not reparent the container. */
+		kube_feed_pod(&qq, "other", "containerd://id");
+		other = quark_pod_lookup(&qq, "other");
+		assert(other != NULL && RB_EMPTY(&other->containers));
+		assert(container->pod == pod && RB_ROOT(&pod->containers) == container);
+		assert(quark_container_lookup(&qq, "containerd://id") == container);
+
+		/* The normal watcher allocation path still creates containers. */
+		kube_feed_pod(&qq, "other", "containerd://fresh");
+		fresh = quark_container_lookup(&qq, "containerd://fresh");
+		assert(fresh != NULL && fresh->pod == other && fresh->linked_by_pod);
+		assert(strcmp(fresh->name, "test") == 0);
+
+		quark_queue_close(&qq);
+		assert(process.container == NULL);
+		assert(RB_EMPTY(&qq.pod_by_uid));
+		assert(RB_EMPTY(&qq.container_by_id));
+	}
 
 	return (0);
 }
@@ -3596,6 +3700,7 @@
 	T(t_backend_flags),
 	T(t_pod_get),
 	T(t_container_get),
+	T(t_kube_container_attach),
 	T(t_hanson),
 	T(t_hanson_escape),
 	T_EBPF(t_rule_path),

@christos68k

christos68k commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Verdict

REQUEST_CHANGES — the prior findings are resolved, but Kubernetes ingestion needs to accept the orphan state now supported by the public API.

I think these are addressed in #382 and #438 (set to merge into #395 (that's one issue with multiple PRs in a stack, reviews in isolation miss context). Besides not ever using stacked PRs in the future 😅 , we could create an "integration branch" where we merge the stacked PRs in batches (by changing the merge target branch as needed).

That should leave us with a final PR containing all the changes from previously stacked PRs that we can review as a single unit and merge.

Alternatively, we can do that work locally in a new branch, push that branch, open a new PR based on that branch and close the stacked PRs.

EDIT: I opened #439 which essentially unstacks all PRs into a single PR/branch.

This branch has not been deployed

No deployments
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.

4 participants