feat: update cardano-node to use socat as PID1 - #342
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChart version in charts/cardano-node/Chart.yaml is bumped from 0.8.1 to 0.8.2. In charts/cardano-node/templates/statefulset.yaml, checksum annotations for blockProducer and topology are reformatted (conditionals remain gated by .Values.blockProducer.enabled and .Values.topology.enabled). The cardano-node container and socat-ntc sidecar commands were changed from shell-wrapped 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/cardano-node/templates/statefulset.yaml (1)
217-224: Good change for PID1, butPORTenv var is now unused.The refactor to direct exec form correctly makes socat PID1 for proper signal handling. However, the
PORTenvironment variable (lines 222-224) is no longer used since the port is now templated directly in the args. Consider removing this dead code.🧹 Proposed cleanup
args: - "TCP-LISTEN:{{ .Values.service.ports.socatNtc.targetPort }},fork" - "UNIX-CLIENT:/ipc/node.socket,ignoreeof" - env: - - name: PORT - value: "{{ .Values.service.ports.socatNtc.targetPort }}" ports:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/cardano-node/templates/statefulset.yaml` around lines 217 - 224, Remove the now-unused environment variable by deleting the env block that sets PORT (env name: PORT) in the socat container spec, since the port is directly templated in the args for the socat command; also scan for any remaining references to PORT in the same template (e.g., container env usage or liveness/readiness probes) and remove or update them if present to avoid unused/dead config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/cardano-node/templates/statefulset.yaml`:
- Around line 217-224: Remove the now-unused environment variable by deleting
the env block that sets PORT (env name: PORT) in the socat container spec, since
the port is directly templated in the args for the socat command; also scan for
any remaining references to PORT in the same template (e.g., container env usage
or liveness/readiness probes) and remove or update them if present to avoid
unused/dead config.
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
91d95c9 to
e71372e
Compare
Summary by cubic
Run socat as PID 1 in the cardano-node sidecar to improve signal handling and clean shutdowns. Bump chart to 0.8.2.
Written for commit e71372e. Summary will update on new commits.
Summary by CodeRabbit
Chores
Refactor