Repository navigation
quark: Fix race condition between the two container write paths - #438
Open
christos68k wants to merge 1 commit into
Open
christos68k wants to merge 1 commit into
christos68k wants to merge 1 commit into
Conversation
2 tasks done
christos68k
force-pushed
the
ck/container-link
branch
from
September 28, 2026 16:03
835a9f2 to
741c971
Compare
A container created without a Pod through the API would later fail an update and Pod link by kube-talker as it'd appear as a duplicate. Note that the reverse order (kube-talker first then API) would result in different (but correct) state as the container would exist linked to Pod metainformation. With this fix, both write path orderings should result in exactly the same state provided both writers report the same metadata, which they should as they receive them from the same container runtime. A failed image demux no longer deletes the container as it is no longer solely owned by kube-talker. The partial object is now left in place, to avoid destroying shared state, and hopefully completed by the next pod update.
christos68k
force-pushed
the
ck/container-link
branch
from
September 28, 2026 16:04
741c971 to
985f008
Compare
christos68k
commented
Sep 28, 2026
| * the full image demux instead of leaving it half done. | ||
| */ | ||
| if (demux_image(container) == -1) { | ||
| container_delete(qq, container); |
Member
Author
There was a problem hiding this comment.
With a second write path, kube-talker is not the sole owner so we shouldn't destroy associated state on demux_image failures (memory allocation failure which, if triggered, should be transient). We can clear the metadata and let a future pod update retry.
| return (n); | ||
| } | ||
|
|
||
| static struct quark_container * |
Member
Author
There was a problem hiding this comment.
This isn't needed any more.
| qwarnx("unknown container state, ignoring"); | ||
| return (-1); | ||
| } | ||
| container = pod_lookup_container(pod, containerID->valuestring); |
Member
Author
There was a problem hiding this comment.
This would return NULL for a container previously inserted without a pod.
| container = quark_container_get(qq, containerID->valuestring, | ||
| pod != NULL ? pod->uid : NULL); | ||
| if (container == NULL) { | ||
| container = quark_container_create(qq, |
Member
Author
There was a problem hiding this comment.
This would fail with EEXIST for containers without a pod.
nicholasberlin
approved these changes
Sep 28, 2026
This was referenced Sep 28, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A container created without a Pod through the API would later fail an update and Pod link by kube-talker as it'd appear as a duplicate. Note that the reverse order (kube-talker first then API) would result in different (but correct) state as the container would exist and be linked to Pod metainformation.
With this fix, both write path orderings should result in exactly the same state provided both writers report the same metadata, which they should as they receive them from the same container runtime.
A failed image demux no longer deletes the container as it is no longer solely owned by kube-talker. The partial object is now left in place, to avoid destroying shared state, and hopefully completed by the next pod update.
Also see: