refactor(helm): Replace dependency waiting logic with waitFor helper leveraging new job-watcher service account privileges.#1772
Conversation
…r leveraging new `job-watcher` service account privileges.
WalkthroughBumps Helm chart version, adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
| "-m", "clp_py_utils.initialize-results-cache", | ||
| "--uri", "mongodb://{{ include "clp.fullname" . }}-results-cache:27017/{{ .Values.clpConfig.results_cache.db_name }}", | ||
| "--stream-collection", {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} | ||
| "--uri", "mongodb://{{ include "clp.fullname" . }}-results-cache:27017/{{ |
There was a problem hiding this comment.
these lines are unrelated to the main purpose of the PR. they're only updated to avoid code style violations (exceeding max 100 chars per line)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml (1)
16-42: Fix MongoDB URI formatting to prevent whitespace injection in connection stringThe multi-line double-quoted URI at lines 39–40 will have its newline folded into a space by YAML's parsing rules, producing an invalid MongoDB connection string like
mongodb://...:27017 /db_nameinstead of the intendedmongodb://...:27017/db_name.Use a Helm template variable with
printfto construct the URI as a single string:containers: - name: "results-cache-indices-creator" image: "{{ include "clp.image.ref" . }}" imagePullPolicy: "{{ .Values.image.clpPackage.pullPolicy }}" + {{- $resultsCacheUri := printf "mongodb://%s-results-cache:27017/%s" (include "clp.fullname" .) .Values.clpConfig.results_cache.db_name }} env: - name: "PYTHONPATH" value: "/opt/clp/lib/python3/site-packages" command: [ "python3", "-u", "-m", "clp_py_utils.initialize-results-cache", "--uri", - "mongodb://{{ include "clp.fullname" . }}-results-cache:27017 - /{{.Values.clpConfig.results_cache.db_name }}", + {{ $resultsCacheUri | quote }}, "--stream-collection", {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} ]This approach preserves the URI semantics while respecting line-length constraints and is consistent with patterns already used elsewhere in this Helm chart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
tools/deployment/package-helm/Chart.yaml(1 hunks)tools/deployment/package-helm/templates/_helpers.tpl(1 hunks)tools/deployment/package-helm/templates/db-table-creator-job.yaml(1 hunks)tools/deployment/package-helm/templates/job-watcher-role.yaml(1 hunks)tools/deployment/package-helm/templates/job-watcher-rolebinding.yaml(1 hunks)tools/deployment/package-helm/templates/job-watcher-serviceaccount.yaml(1 hunks)tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-08T17:20:02.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2024-10-24T14:25:17.978Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2025-09-22T15:46:34.873Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1311
File: integration-tests/tests/test_identity_transformation.py:93-97
Timestamp: 2025-09-22T15:46:34.873Z
Learning: In the CLP project, multi-line formatting is preferred over single-line ternary operators when the single line would exceed the project's line length limits, prioritizing code readability and adherence to coding standards.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2025-03-18T07:27:54.738Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/job-watcher-role.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
tools/deployment/package-helm/templates/job-watcher-rolebinding.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 9-9: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
[error] 10-10: too many spaces inside braces
(braces)
[error] 10-10: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
[error] 16-16: too many spaces inside braces
(braces)
[error] 16-16: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/job-watcher-serviceaccount.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
tools/deployment/package-helm/templates/db-table-creator-job.yaml
[error] 16-16: too many spaces inside braces
(braces)
[error] 16-16: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
🔇 Additional comments (5)
tools/deployment/package-helm/Chart.yaml (1)
3-3: Chart version bump looks appropriateThe version increment to
0.1.2-dev.5is consistent with the added helpers/RBAC changes; just ensure it still matches your intended merge order with adjacent PRs before merging.tools/deployment/package-helm/templates/job-watcher-serviceaccount.yaml (1)
1-6: ServiceAccount wiring and labelling look consistentThe job-watcher ServiceAccount name and labels align with
clp.fullname/clp.labelsusage and match the Role/RoleBinding subjects; no functional issues spotted.tools/deployment/package-helm/templates/job-watcher-role.yaml (1)
1-13: RBAC scope for job-watcher is appropriately minimalGranting only
get,list, andwatchonbatch/jobsandpodsis a good fit for thekubectl waituse case, without over-privileging the ServiceAccount.tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
16-27: Job now correctly leverages job-watcher SA and clp.waitForBinding the Job to
{{ include "clp.fullname" . }}-job-watcherand switching the initContainer toclp.waitForfor thedatabaseservice is consistent with the new RBAC model; this should make readiness waiting more robust than the prior nc-based approach, assuming the database pods carry theapp.kubernetes.io/component: "database"label.tools/deployment/package-helm/templates/job-watcher-rolebinding.yaml (1)
1-14: RoleBinding correctly connects job-watcher SA and RoleThe RoleBinding cleanly ties
{{ include "clp.fullname" . }}-job-watcherto the corresponding Role in.Release.Namespace, with labels consistent with the rest of the chart.
| {{/* | ||
| Creates an initContainer that waits for a Kubernetes resource to be ready. | ||
|
|
||
| @param {object} root Root template context | ||
| @param {string} type "service" (waits for pod readiness) or "job" (waits for completion) | ||
| @param {string} name For "service": component name | ||
| For "job": job name suffix | ||
| @return {string} YAML-formatted initContainer definition | ||
| */}} | ||
| {{- define "clp.waitFor" -}} | ||
| name: "wait-for-{{ .name }}" | ||
| image: "bitnami/kubectl:latest" | ||
| command: [ | ||
| "kubectl", "wait", | ||
| {{- if eq .type "service" }} | ||
| "--for=condition=ready", | ||
| "pod", "--selector", "app.kubernetes.io/component={{ .name }}", | ||
| {{- else if eq .type "job" }} | ||
| "--for=condition=complete", | ||
| "job/{{ include "clp.fullname" .root }}-{{ .name }}", | ||
| {{- end }} | ||
| "--timeout=300s" | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten clp.waitFor helper: avoid :latest image and fail fast on invalid types
The overall helper logic looks good, but two robustness tweaks would help:
- Avoid
bitnami/kubectl:latestto reduce surprise breaks from upstream image changes; consider pinning a specific tag or making the image configurable viavalues.yaml. - If
.typeis neither"service"nor"job", the template currently emits an incompletekubectl waitcommand; it would be safer to surface this as a template error (e.g., viafail) rather than at runtime.
Example for the type handling:
- {{- if eq .type "service" }}
+ {{- if eq .type "service" }}
"--for=condition=ready",
"pod", "--selector", "app.kubernetes.io/component={{ .name }}",
- {{- else if eq .type "job" }}
+ {{- else if eq .type "job" }}
"--for=condition=complete",
"job/{{ include "clp.fullname" .root }}-{{ .name }}",
- {{- end }}
+ {{- else }}
+ {{- fail (printf "clp.waitFor: unsupported type %q" .type) }}
+ {{- end }}🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/_helpers.tpl around lines 138 to 160,
tighten the clp.waitFor helper by making the kubectl image configurable/pinned
instead of hardcoding "bitnami/kubectl:latest" (add a values.yaml key like
global.kubectlImage or default to a specific tag and reference it here) and add
an explicit template guard that fails for invalid .type (use sprig’s fail or
required to emit a template error when .type is not "service" or "job"),
ensuring the rendered command never becomes incomplete at runtime.
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-08T17:20:02.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2024-10-24T14:25:17.978Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2025-09-22T15:46:34.873Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1311
File: integration-tests/tests/test_identity_transformation.py:93-97
Timestamp: 2025-09-22T15:46:34.873Z
Learning: In the CLP project, multi-line formatting is preferred over single-line ternary operators when the single line would exceed the project's line length limits, prioritizing code readability and adherence to coding standards.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
📚 Learning: 2025-03-18T07:27:54.738Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-03-18T07:27:54.738Z
Learning: Double parentheses in the codebase are intentional and required for clang-tidy to pass. These should not be flagged as style issues in code reviews.
Applied to files:
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
[error] 16-16: too many spaces inside braces
(braces)
[error] 16-16: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 27-27: too many spaces inside braces
(braces)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: package-image
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (1)
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml (1)
16-27: ServiceAccount andclp.waitForwiring looks good; double-check RBAC for servicesThe
serviceAccountNamepointing at{{ include "clp.fullname" . }}-job-watcherand theclp.waitForinitContainer invocation for theresults-cacheservice both look consistent with the new helper and job-watcher concept.One thing to verify: since
clp.waitForis used withtype: "service", ensure the job-watcher Role (or another bound Role/ClusterRole) grants the necessary verbs onservices(and any other resourcekubectl waittouches for service readiness). The PR description mentions jobs and pods explicitly, so it’s worth confirming services are also covered to avoidkubectl waitfailing withForbidden.Based on learnings, I’m not suggesting additional refactors here beyond this RBAC check.
| "--uri", | ||
| "mongodb://{{ include "clp.fullname" . }}-results-cache:27017 | ||
| /{{ .Values.clpConfig.results_cache.db_name }}", | ||
| "--stream-collection", | ||
| {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} |
There was a problem hiding this comment.
Multi-line MongoDB URI introduces whitespace and will break the connection
The way the --uri value is currently split across lines will introduce whitespace between :27017 and /{{ .Values.clpConfig.results_cache.db_name }} at render time (either a space or a newline, depending on YAML parsing), yielding an invalid MongoDB URI and likely causing the job to fail to connect.
You need the URI to be rendered as a single, uninterrupted string.
A minimal fix is to put the URI back on a single line:
- "--uri",
- "mongodb://{{ include "clp.fullname" . }}-results-cache:27017
- /{{ .Values.clpConfig.results_cache.db_name }}",
+ "--uri",
+ "mongodb://{{ include "clp.fullname" . }}-results-cache:27017/{{ .Values.clpConfig.results_cache.db_name }}",
"--stream-collection",
{{ .Values.clpConfig.results_cache.stream_collection_name | quote }}This restores a valid MongoDB connection string; you can revisit line-wrapping separately if needed to satisfy the 100‑character limit without changing the rendered URI.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "--uri", | |
| "mongodb://{{ include "clp.fullname" . }}-results-cache:27017 | |
| /{{ .Values.clpConfig.results_cache.db_name }}", | |
| "--stream-collection", | |
| {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} | |
| "--uri", | |
| "mongodb://{{ include "clp.fullname" . }}-results-cache:27017/{{ .Values.clpConfig.results_cache.db_name }}", | |
| "--stream-collection", | |
| {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 39-39: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
[error] 42-42: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
In
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
around lines 38 to 42, the MongoDB URI argument is split across lines which
inserts whitespace/newline into the rendered string and breaks the connection;
fix by joining the URI into a single uninterrupted string (ensure "mongodb://{{
include "clp.fullname" . }}-results-cache:27017/{{
.Values.clpConfig.results_cache.db_name }}" is rendered on one line) so the
--uri value is a valid MongoDB connection string.
There was a problem hiding this comment.
@junhaoliao Merci! Feel free to ping me once you've applied the fix. 👍
# Conflicts: # tools/deployment/package-helm/Chart.yaml
junhaoliao
requested review from
kirkrodrigues
and removed request for
kirkrodrigues
- Fix directory name from `results-cache` to `results_cache` to match PV hostPath - Add `wait_for_pods()` function to wait for jobs and pods to be ready - Replace `helm uninstall || true` with `helm uninstall --ignore-not-found`
There was a problem hiding this comment.
changes in this file should be identical to the ones done in #1784
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tools/deployment/package-helm/Chart.yaml(1 hunks)tools/deployment/package-helm/test.sh(2 hunks)
🔇 Additional comments (4)
tools/deployment/package-helm/Chart.yaml (1)
3-3: Version bump looks good.The version increment to
0.1.2-dev.6aligns with the PR objectives and merge ordering requirements mentioned in the description.tools/deployment/package-helm/test.sh (3)
56-56: Good fix for naming consistency.The path change from
results-cachetoresults_cacheimproves consistency with the directory naming convention.
75-75: Excellent improvement to error handling.Using
--ignore-not-foundis more precise than|| true, as it specifically handles the "not found" case rather than suppressing all errors.
79-79: LGTM! The wait parameters are reasonable.The 300-second overall timeout with 5-second polling and wait intervals provides a good balance between responsiveness and avoiding excessive API calls.
| wait_for_pods() { | ||
| local timeout_seconds=$1 | ||
| local poll_interval_seconds=$2 | ||
| local wait_timeout_seconds=$3 | ||
|
|
||
| echo "Waiting for all pods to be ready" \ | ||
| "(timeout=${timeout_seconds}s, poll=${poll_interval_seconds}s," \ | ||
| "wait=${wait_timeout_seconds}s)..." | ||
|
|
||
| # Reset bash built-in SECONDS counter | ||
| SECONDS=0 | ||
|
|
||
| while true; do | ||
| sleep "${poll_interval_seconds}" | ||
| kubectl get pods | ||
|
|
||
| if kubectl wait job \ | ||
| --all \ | ||
| --for=condition=Complete \ | ||
| --timeout="${wait_timeout_seconds}s" 2>/dev/null \ | ||
| && kubectl wait pods \ | ||
| --all \ | ||
| --selector='!job-name' \ | ||
| --for=condition=Ready \ | ||
| --timeout="${wait_timeout_seconds}s" 2>/dev/null | ||
| then | ||
| echo "All jobs completed and services are ready." | ||
| return 0 | ||
| fi | ||
|
|
||
| if [[ ${SECONDS} -ge ${timeout_seconds} ]]; then | ||
| echo "ERROR: Timed out waiting for pods to be ready" | ||
| return 1 | ||
| fi | ||
|
|
||
| echo "---" | ||
| done | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider eliminating the initial sleep delay.
The function sleeps at the start of each iteration (line 28), which introduces a 5-second delay before the first readiness check. Consider restructuring to check immediately on the first iteration, then sleep only between subsequent checks.
Apply this diff to check immediately on first iteration:
# Reset bash built-in SECONDS counter
SECONDS=0
+ local first_iteration=true
while true; do
- sleep "${poll_interval_seconds}"
+ if [[ "${first_iteration}" == "true" ]]; then
+ first_iteration=false
+ else
+ sleep "${poll_interval_seconds}"
+ fi
kubectl get podsOverall, the wait logic is well-implemented.
The function correctly uses kubectl wait with appropriate timeouts for both jobs and pods, includes proper error handling, and has clear timeout logic using the SECONDS built-in.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wait_for_pods() { | |
| local timeout_seconds=$1 | |
| local poll_interval_seconds=$2 | |
| local wait_timeout_seconds=$3 | |
| echo "Waiting for all pods to be ready" \ | |
| "(timeout=${timeout_seconds}s, poll=${poll_interval_seconds}s," \ | |
| "wait=${wait_timeout_seconds}s)..." | |
| # Reset bash built-in SECONDS counter | |
| SECONDS=0 | |
| while true; do | |
| sleep "${poll_interval_seconds}" | |
| kubectl get pods | |
| if kubectl wait job \ | |
| --all \ | |
| --for=condition=Complete \ | |
| --timeout="${wait_timeout_seconds}s" 2>/dev/null \ | |
| && kubectl wait pods \ | |
| --all \ | |
| --selector='!job-name' \ | |
| --for=condition=Ready \ | |
| --timeout="${wait_timeout_seconds}s" 2>/dev/null | |
| then | |
| echo "All jobs completed and services are ready." | |
| return 0 | |
| fi | |
| if [[ ${SECONDS} -ge ${timeout_seconds} ]]; then | |
| echo "ERROR: Timed out waiting for pods to be ready" | |
| return 1 | |
| fi | |
| echo "---" | |
| done | |
| } | |
| wait_for_pods() { | |
| local timeout_seconds=$1 | |
| local poll_interval_seconds=$2 | |
| local wait_timeout_seconds=$3 | |
| echo "Waiting for all pods to be ready" \ | |
| "(timeout=${timeout_seconds}s, poll=${poll_interval_seconds}s," \ | |
| "wait=${wait_timeout_seconds}s)..." | |
| # Reset bash built-in SECONDS counter | |
| SECONDS=0 | |
| local first_iteration=true | |
| while true; do | |
| if [[ "${first_iteration}" == "true" ]]; then | |
| first_iteration=false | |
| else | |
| sleep "${poll_interval_seconds}" | |
| fi | |
| kubectl get pods | |
| if kubectl wait job \ | |
| --all \ | |
| --for=condition=Complete \ | |
| --timeout="${wait_timeout_seconds}s" 2>/dev/null \ | |
| && kubectl wait pods \ | |
| --all \ | |
| --selector='!job-name' \ | |
| --for=condition=Ready \ | |
| --timeout="${wait_timeout_seconds}s" 2>/dev/null | |
| then | |
| echo "All jobs completed and services are ready." | |
| return 0 | |
| fi | |
| if [[ ${SECONDS} -ge ${timeout_seconds} ]]; then | |
| echo "ERROR: Timed out waiting for pods to be ready" | |
| return 1 | |
| fi | |
| echo "---" | |
| done | |
| } |
🤖 Prompt for AI Agents
In tools/deployment/package-helm/test.sh around lines 15 to 52, the loop always
sleeps at the top causing an unnecessary initial delay before the first
readiness check; change the loop so it performs the kubectl checks immediately
on the first iteration and only sleeps between subsequent iterations (e.g., move
the sleep to the end of the loop or guard it with a first-iteration flag),
keeping all existing kubectl wait calls, timeout/return logic, and SECONDS-based
timeout unchanged.
# Conflicts: # tools/deployment/package-helm/Chart.yaml
| "--uri", "mongodb://{{ include "clp.fullname" . }}-results-cache:27017/{{ .Values.clpConfig.results_cache.db_name }}", | ||
| "--stream-collection", {{ .Values.clpConfig.results_cache.stream_collection_name | quote }} | ||
| "--uri", | ||
| "mongodb://{{ include "clp.fullname" . }}-results-cache:27017\ |
There was a problem hiding this comment.
note in 635090b , a \ was added at the end of this line to properly escape the multiline string; otherwise, a space would show up in the URL.
…r leveraging new `job-watcher` service account privileges. (y-scope#1772) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
3 participants
Important
This PR is created address handling / styling issues found in reviewing #1749 . It should be merged:
createLocalPv: #1771 so that the Helm chart development version numbers will be correctly incremented.createLocalPv: #1771. See Update test.sh to use results_cache directory naming #1782 for details of the bug.createLocalPv: #1771 & refactor(helm): Replace dependency waiting logic withwaitForhelper leveraging newjob-watcherservice account privileges. #1772 are incremental to reach the state in feat(helm): Add compression & query job orchestration and the webui to the chart. #1749Description
Note
This PR is part of the ongoing work for #1309. More PRs will be submitted until the Helm chart is complete and fully functional.
Add clp.waitFor helper template that creates an initContainer
to wait for Kubernetes resources (services or jobs) to be ready.
Uses kubectl wait instead of nc-based polling.
Add RBAC resources for job-watcher:
- job-watcher-serviceaccount.yaml: ServiceAccount for pods
- job-watcher-role.yaml: Role with get/list/watch on jobs and pods
- job-watcher-rolebinding.yaml: RoleBinding to connect the two
Update all jobs to use:
- serviceAccountName for RBAC permissions
- waitFor helper for init containers
Checklist
breaking change.
Validation performed
same validation steps as done in #1698
Summary by CodeRabbit
Chores
New Features
Security
Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.