-
Notifications
You must be signed in to change notification settings - Fork 51
fix(container-cache): always publish NodePort and report inactive registry config #1000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -290,19 +290,26 @@ persistentVolumeClaim: | |||||||||||||||||
|
|
||||||||||||||||||
| ### Service Configuration | ||||||||||||||||||
|
|
||||||||||||||||||
| The service type and port can be configured based on your access requirements: | ||||||||||||||||||
| The service port is configurable. The service type is not: Container Cache is | ||||||||||||||||||
| always exposed as a `NodePort`. | ||||||||||||||||||
|
|
||||||||||||||||||
| ```yaml | ||||||||||||||||||
| # values.yaml | ||||||||||||||||||
|
|
||||||||||||||||||
| service: | ||||||||||||||||||
| # Service type: ClusterIP, NodePort, or LoadBalancer | ||||||||||||||||||
| type: ClusterIP | ||||||||||||||||||
|
|
||||||||||||||||||
| # Port for the Container Cache service | ||||||||||||||||||
| port: 30345 | ||||||||||||||||||
| ``` | ||||||||||||||||||
|
|
||||||||||||||||||
| The container runtime on each node reaches the cache at `${NODE_IP}:${port}` | ||||||||||||||||||
| from the host network namespace, where cluster service DNS and ClusterIP | ||||||||||||||||||
| addresses are not dependable, so the port must be published on every node. | ||||||||||||||||||
|
|
||||||||||||||||||
| Setting `service.type` fails the install with an explicit error. A ClusterIP | ||||||||||||||||||
| service publishes no node port, so the registry mirror written to each node | ||||||||||||||||||
| would point at a port nothing listens on, and image pulls would fall back to | ||||||||||||||||||
| the upstream registry with no error and no cache involvement. | ||||||||||||||||||
|
Comment on lines
+308
to
+311
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Document the allowed override accurately. Line 308 says that every Proposed fix-Setting `service.type` fails the install with an explicit error. A ClusterIP
+Setting `service.type` to a value other than `NodePort` fails the install with an explicit error. A ClusterIP📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| ### Metrics Configuration | ||||||||||||||||||
|
|
||||||||||||||||||
| Container Cache includes Prometheus metrics for monitoring cache performance: | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Derive readiness from active runtime state.
The readiness state is container-local and evaluated only once. If this pod restarts before containerd restarts, Lines 101-106 reset
containerd_restart_pending; unchanged hashes then cause Line 260 to mark an inactive configuration ready. If containerd restarts later, the sleep loop never re-evaluates state.A failed CRI-O SIGHUP has the same result. Line 248 states that the drop-in applies on a later reload, but Lines 257-260 still mark the node ready.
Persist and reconcile pending runtime activation state. Keep the pod process successful for best-effort CRI-O signaling, but do not create the readiness marker until the relevant runtime has activated the written configuration. Add coverage for pod recreation before a containerd restart, a later containerd restart, and a failed CRI-O reload.
Also applies to: 163-173, 243-260, 263-267
🤖 Prompt for AI Agents