Add Watson daemon set to HCP cluster to meet KPI - #6820
Add Watson daemon set to HCP cluster to meet KPI#6820Jonathan CHang (jonathan34c) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The forwarder DaemonSet template introduces a functional regression risk for Shoebox logs (missing /shoebox-config mount in Fluent Bit) and includes dead/unwired Watson values that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds the Azure Watson agent to the arobit-forwarder DaemonSet (svc and mgmt clusters) via new Helm values, wires Watson image/resources/securityContext into config/config.yaml + schema, and updates the rendered configs and Helm fixtures accordingly.
Changes:
- Add
watsonconfiguration blocks to arobit svc/mgmt values and inject awatson-agentcontainer + volumes into theforwarder-daemonsetHelm template. - Extend
config/config.yamldefaults andconfig/config.schema.jsonto includearobit.watsonsettings, then re-render dev configs. - Update Helm template fixtures / dev-infrastructure rendered fixtures to reflect the new container and volumes.
File summaries
| File | Description |
|---|---|
| observability/arobit/values-svc.yaml | Adds Watson values for svc clusters. |
| observability/arobit/values-mgmt.yaml | Adds Watson values for mgmt clusters. |
| observability/arobit/deploy/templates/forwarder-daemonset.yaml | Adds watson-agent container and watson-crashdump volume to the forwarder DaemonSet. |
| observability/arobit/testdata/zz_fixture_TestHelmTemplate_helmtest_mdsd_and_kusto_enabled_svc.yaml | Updates Helm fixture output for svc (mdsd+kusto enabled). |
| observability/arobit/testdata/zz_fixture_TestHelmTemplate_helmtest_mdsd_and_kusto_enabled_mgmt.yaml | Updates Helm fixture output for mgmt (mdsd+kusto enabled). |
| observability/arobit/testdata/zz_fixture_TestHelmTemplate_helmtest_kusto_unlimited_memory_svc.yaml | Updates Helm fixture output for svc (kusto unlimited memory). |
| observability/arobit/testdata/zz_fixture_TestHelmTemplate_helmtest_kusto_disabled_svc.yaml | Updates Helm fixture output for svc (kusto disabled). |
| observability/arobit/testdata/zz_fixture_TestHelmTemplate_helmtest_buffering_disabled_svc.yaml | Updates Helm fixture output for svc (buffering disabled). |
| dev-infrastructure/zz_fixture_TestHelmTemplate_dev_westus3_svc_1_arobit.yaml | Updates rendered dev-infra fixture for svc. |
| dev-infrastructure/zz_fixture_TestHelmTemplate_dev_westus3_mgmt_1_arobit.yaml | Updates rendered dev-infra fixture for mgmt. |
| config/rendered/dev/pers/westus3.yaml | Materialized config includes new arobit.watson block. |
| config/rendered/dev/perf/westus3.yaml | Materialized config includes new arobit.watson block. |
| config/rendered/dev/dev/westus3.yaml | Materialized config includes new arobit.watson block. |
| config/rendered/dev/cspr/westus3.yaml | Materialized config includes new arobit.watson block. |
| config/rendered/dev/ci01/centralus.yaml | Materialized config includes new arobit.watson block. |
| config/rendered/dev/ci00/centralus.yaml | Materialized config includes new arobit.watson block. |
| config/config.yaml | Adds default arobit.watson configuration (image/resources/securityContext/volumes). |
| config/config.schema.json | Adds schema for arobit.watson. |
Review details
Suppressed comments (2)
observability/arobit/values-svc.yaml:121
watson.volumes.mdsdSocketHostPathis defined here but not referenced by the Helm templates, so it currently has no effect. Either wire it into the DaemonSet volume definition or remove it to avoid dead configuration.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
observability/arobit/values-mgmt.yaml:121
watson.volumes.mdsdSocketHostPathis defined here but not referenced by the Helm templates, so it currently has no effect. Either wire it into the DaemonSet volume definition or remove it to avoid dead configuration.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
- Files reviewed: 18/18 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: flb-config | ||
| mountPath: /forwarder/etc | ||
| readOnly: true | ||
| - name: watson-agent | ||
| image: {{ .Values.watson.image.registry }}/{{ .Values.watson.image.repository }}@{{ .Values.watson.image.digest }} |
0bf5369 to
346ddb4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces dead/unused configuration (watson.volumes.mdsdSocketHostPath) and the PR metadata is not compliant with CONTRIBUTING.md (missing filled-in Why + ticket link).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
| watson: | ||
| image: | ||
| registry: mcr.microsoft.com | ||
| repository: azure-watson/agent/azlinux3 | ||
| digest: sha256:8e5ef3e76be33794cef7460712eb9ff0c73bce8b8e5ffc33c5c6bfe0762f5db0 # 1.24.128.0 |
346ddb4 to
62dd480
Compare
62dd480 to
ee6740c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a privileged, hostPath-mounting daemonset across clusters, which is security-sensitive and warrants final human review despite only minor fixups being needed.
Review details
Suppressed comments (2)
observability/arobit/values-svc.yaml:117
watson.volumes.mdsdSocketHostPathis defined here but isn’t referenced anywhere in the arobit Helm templates (onlywatson.volumes.crashdumpHostPathis used). Leaving unused values around is confusing and suggests there’s a missing chart wiring; either wire it into the DaemonSet template or remove it until needed.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
observability/arobit/values-mgmt.yaml:117
watson.volumes.mdsdSocketHostPathis defined here but isn’t referenced anywhere in the arobit Helm templates (onlywatson.volumes.crashdumpHostPathis used). Leaving unused values around is confusing and suggests there’s a missing chart wiring; either wire it into the DaemonSet template or remove it until needed.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/retest-required |
|
/test e2e-parallel |
|
/retest-required |
ee6740c to
af0975c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The new watson.volumes.mdsdSocketHostPath value is introduced in Helm values but is unused by the chart, which is misleading and suggests incomplete wiring.
Review details
Suppressed comments (2)
observability/arobit/values-svc.yaml:117
watson.volumes.mdsdSocketHostPathis defined but not referenced anywhere in the arobit Helm templates (the watson-agent mountsmdsd-run-clusterlogsemptyDir instead). This unused value is misleading and makes it unclear whether the intent was to mount the host’s/var/run/mdsdinto the pod.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
observability/arobit/values-mgmt.yaml:117
watson.volumes.mdsdSocketHostPathis defined but not referenced anywhere in the arobit Helm templates (the watson-agent mountsmdsd-run-clusterlogsemptyDir instead). This unused value is misleading and makes it unclear whether the intent was to mount the host’s/var/run/mdsdinto the pod.
volumes:
crashdumpHostPath: "{{ .arobit.watson.volumes.crashdumpHostPath }}"
mdsdSocketHostPath: "/var/run/mdsd"
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
af0975c to
95f9130
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There’s a mismatch between the PR’s stated “Watson daemon set” intent and the actual implementation (sidecar in an existing DaemonSet), and a small Helm-template indentation issue should be corrected for maintainability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite
| {{- if and .Values.forwarder.mdsd.enabled (eq .Values.forwarder.clusterType "mgmt") }} | ||
| - name: shoebox-mappings | ||
| mountPath: /shoebox-config | ||
| readOnly: true | ||
| {{- end }} |
| - name: watson-agent | ||
| image: {{ .Values.watson.image.registry }}/{{ .Values.watson.image.repository }}@{{ .Values.watson.image.digest }} | ||
| imagePullPolicy: '{{ .Values.watson.image.pullPolicy }}' | ||
| env: |
|
/retest |
Steve Kuznetsov (stevekuznetsov)
left a comment
Steve Kuznetsov (stevekuznetsov)
left a comment
There was a problem hiding this comment.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonathan34c, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
95f9130 to
742d879
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved merge conflict markers remain in the Helm template and a generated fixture, which will break rendering/tests until resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite
742d879 to
004f3d0
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Fix service-cluster MDSD volume integration and rename the PR with a Conventional Commits prefix.
Review details
Suppressed comments (2)
config/config.yaml:187
- The PR title
Add Watson daemon set to HCP cluster to meet KPIdoes not use the required Conventional Commits prefix fromCONTRIBUTING.md:79-83; please rename it, for example, tofeat: add Watson daemon set to HCP cluster to meet KPI.
watson:
observability/arobit/deploy/templates/forwarder-daemonset.yaml:358
- On the
svcchart, thisemptyDiris the only backing volume for Watson's/var/run/mdsd, but themdsd-clusterlogscontainer is gated toclusterType == "mgmt"above, so no process populates it. Service-cluster nodes expose Geneva through the host's/var/run/mdsd(docs/mise.md:51-56); with this volume,MDSD_ROLE_PREFIX=/var/run/mdsd/asa/cannot reach the socket and Watson reports will not be sent. Select a/var/run/mdsdhostPath forsvcwhile retainingemptyDirformgmt.
- name: mdsd-run-clusterlogs
emptyDir: {}
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
004f3d0 to
c99d95a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The PR title lacks the required Conventional Commits prefix, and Watson's image digest is not tracked by the image-update workflow.
Review details
Suppressed comments (2)
config/config.yaml:187
- The PR title does not use the repository's required Conventional Commits prefix. Please rename it to something like
feat: add Watson DaemonSet to HCP clustersbefore merging.
watson:
config/config.yaml:191
- This introduces a third Arobit image, but
tooling/image-updater/config.yaml:108-126only tracks the forwarder and mdsd images. Consequently future Watson digest updates will not be picked up by the repository's image-update workflow, leaving this privileged agent pinned indefinitely unless updated manually. Add an updater component targetingdefaults.arobit.watson.image.digest(or document why this image is intentionally excluded).
digest: sha256:8e5ef3e76be33794cef7460712eb9ff0c73bce8b8e5ffc33c5c6bfe0762f5db0 # 1.24.128.0
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Jonathan CHang (@jonathan34c): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
https://redhat.atlassian.net/jira/software/c/projects/ARO/boards/3274?assignee=64066bea4d5e4b44e233a4c8&selectedIssue=[ARO-29045](https://redhat.atlassian.net/browse/ARO-29045)
What
The Azure Watson container accesses coredumps from the service container once the generated coredumps is saved to a directory (also called incoming queue) on the host node. Azure Watson container relies on host node mounts to persist the dump files on the host node if the container stops unexpectedly and also to store larger dump files.
Why
We need to add watson daemon set to every cluster required by aks to meet KPI.
Testing
MGMT:
SVC:
Testing is required for feature completion and tests should be part of the pull
request along with the feature changes.
Describe the testing provided. If you did not add tests, provide a clear
justification.
Special notes for your reviewer
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.