Repository navigation
Conversation
24c8711 to
499635f
Compare
PR Review: #380 — Split pods/containers into separate trees in quark_queueAuthor: Tacklebox SummaryThis change moves the pod index onto FindingsSignificant
Minor
VerdictREQUEST_CHANGES — fix the existing-container parent-link path before exposing Verification
|
56cf25f to
b028731
Compare
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
left a comment
There was a problem hiding this comment.
@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?
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.
e4c4b6f to
de5cbf6
Compare
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.
7814c75 to
a8e55b7
Compare
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.
e3a3ab3 to
f448b14
Compare
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.
f448b14 to
579ebab
Compare
PR Review: #380 — Split pods/containers into separate trees in quark_queueAuthor: Tacklebox SummaryThe PR moves pod ownership into FindingsSignificant
VerdictREQUEST_CHANGES — the prior findings are resolved, but Kubernetes ingestion needs to accept the orphan state now supported by the public API. |
--- 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);
|
--- 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), |
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. |
The merge-base changed after approval.
Pods and containers need to be accessible independently of Kubernetes state. This change moves the pod index from
quark_kubetoquark_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 withEEXIST. 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 dockerpassed using the repository Dockerfile.t_pod_getandt_container_getpassed in the Linux builder container.git diff --checkpassed.The Kubernetes integration test was not run.