Sync kubex charts from automation-controller main @ f1e348f#114
Conversation
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
Overall Assessment
This is a well-scoped automated sync PR introducing the new PodAffinityPolicy CRD, its associated RBAC rules, a self-protection annotation on the Deployment, and documentation improvements. The CRD schema itself is structurally sound and follows existing patterns. Two actionable issues were found across the docs and templates.
Risk level: Low–Medium
Critical issues
- None
Major issues
-
Hardcoded
args[3]index in Troubleshooting.md (Troubleshooting.md:18): Thekubectl patchsnippet hardcodes/spec/template/spec/containers/0/args/3as the path for--zap-log-level. Sinceargsare rendered from a Helm template helper, any future reordering silently patches the wrong argument. See inline comment for a safer approach. -
Deployment-level vs Pod-template-level annotation (
deployment.yaml:13): Therightsizing.kubex.ai/pause-until: infiniteannotation is added to the Deployment's ownmetadata.annotations, but an identical annotation already exists on.spec.template.metadata.annotations. Confirm which level the controller evaluates; if only the Pod template annotation is needed, the Deployment-level copy is redundant and may mislead operators.
Minor issues
- Copy-paste artifact in CRD description (
podaffinitypolicies.yaml:165):status.conditionssays "StaticPolicy resource" instead of "PodAffinityPolicy resource". - Inconsistent field name in docs (
Policy-Configuration.md:75): Section heading usesnamespaces.valuesbut the CRD and all other docs usenamespaceSelector.values. workloadTypesdefault vs enum ordering mismatch (podaffinitypolicies.yaml:131): Default list order and enum list order differ, inconsistent with other CRDs in the chart.
DRY improvement opportunities
- The
rightsizing.kubex.ai/pause-until: infiniteannotation now appears in two places in the same template (Deployment metadata + Pod template metadata). If the Deployment-level copy is intentional, a shared comment block or a single YAML anchor would reduce the risk of them drifting.
Suggested next steps
- Verify the correct annotation level (Deployment vs Pod template) and remove the redundant one if applicable.
- Fix the
args/3hardcoded index in the troubleshooting docs before this ships to avoid operator incidents. - Correct the copy-paste description in the CRD (
StaticPolicy→PodAffinityPolicy) and align the field name inPolicy-Configuration.md.
| ``` | ||
|
|
||
| Revert back to info: | ||
|
|
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Major] [Confidence: High]
Location: charts/kubex-automation-engine/docs/Troubleshooting.md:18
Issue: The kubectl patch command hardcodes /spec/template/spec/containers/0/args/3 as the JSON Patch path for --zap-log-level, which will silently break if the args list order ever changes.
Why it matters: args in the Deployment are rendered via {{- include "kubex-automation-engine.managerArgs" . | nindent 8 }}, whose output order is determined by the Helm template. If a future Helm release inserts or removes an arg before index 3, the patch will overwrite the wrong argument—potentially disabling a critical flag—without producing any visible error. This is an operational hazard for users who follow the documentation.
Suggested fix: Replace the index-based approach with a value filter to make the command index-agnostic. For example:
# Find the current index of the --zap-log-level arg
INDEX=$(kubectl -n kubex get deploy/$(kubectl -n kubex get deploy -l app.kubernetes.io/name=kubex-automation-engine -o jsonpath='{.items[0].metadata.name}') \
-o jsonpath='{range .spec.template.spec.containers[0].args[*]}{@}{"\n"}{end}' \
| grep -n 'zap-log-level' | cut -d: -f1)
INDEX=$((INDEX - 1)) # zero-indexed
kubectl -n kubex patch deploy/... --type='json' \
-p="[{\"op\":\"replace\",\"path\":\"/spec/template/spec/containers/0/args/${INDEX}\",\"value\":\"--zap-log-level=debug\"}]"Alternatively, document the use of --zap-log-level exclusively via helm upgrade --set controllerManager.extraArgs[0]=... (the persistent path already documented below) to avoid the fragile index reference altogether.
| metadata: | ||
| name: {{ include "kubex-automation-engine.fullname" . }} | ||
| namespace: {{ include "kubex-automation-engine.namespace" . }} | ||
| annotations: |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Major] [Confidence: Medium]
Location: charts/kubex-automation-engine/templates/deployment.yaml:13
Issue: The rightsizing.kubex.ai/pause-until: infinite annotation is added to the Deployment metadata but the same annotation already existed on the Pod template metadata (.spec.template.metadata.annotations). The Deployment-level annotation is not the mechanism the controller uses to pause rightsizing; the controller reads the annotation on the Pod template spec, not on the enclosing Deployment object itself.
Why it matters: If the controller evaluates this annotation on the Deployment object it would work, but most admission-time/recommendation-time controllers inspect the Pod or Pod template annotations, not the Deployment-level metadata. A mismatch could result in the engine still attempting to automate its own Deployment despite the intent expressed by the comment, or it may be dead/redundant metadata that misleads operators. Given the Pod template already carries the annotation, the Deployment-level copy may be harmless—but if it is relied upon for correctness it should be validated.
Suggested fix: Confirm whether the rightsizing.kubex.ai/pause-until annotation is evaluated at Deployment-level or Pod-level by the controller. If Pod-level is sufficient (as is typical for admission controllers), remove the newly added Deployment-level annotation to avoid confusion. If both levels are intentionally checked, add a comment explaining why both are needed.
| minimum: 0 | ||
| type: integer | ||
| required: | ||
| - affinity |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Location: charts/kubex-crds/templates/rightsizing.kubex.ai_podaffinitypolicies.yaml:165
Issue: The status.conditions description copy-pastes "represent the current state of the StaticPolicy resource" instead of "PodAffinityPolicy resource".
Why it matters: Stale copy-paste in the CRD schema description is surfaced in kubectl describe output and any OpenAPI-aware tooling (e.g., kubectl explain podaffinitypolicies.status.conditions). It will mislead users debugging PodAffinityPolicy resources and indicates the schema block was templated from StaticPolicy without a full find-replace pass.
Suggested fix: Change the description to reference PodAffinityPolicy:
description: |-
conditions represent the current state of the PodAffinityPolicy resource.
...| | `policy.policies.<name>.safetyChecks.maxAnalysisAgeDays` | `ClusterProactivePolicy.spec.safetyChecks.maxAnalysisAgeDays` | Per-policy value wins over top-level `policy.safetyChecks.maxAnalysisAgeDays`. | | ||
| | `policy.safetyChecks.maxAnalysisAgeDays` | `ClusterProactivePolicy.spec.safetyChecks.maxAnalysisAgeDays` | Backward-compatible fallback when not set per policy. | | ||
|
|
||
| ### Namespace wildcards in `scope[].namespaces.values` |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Location: charts/kubex-automation-engine/docs/Policy-Configuration.md:75
Issue: The new section heading references scope[].namespaces.values but all other documentation and the ClusterProactivePolicy CRD field path use scope.namespaceSelector.values. The Cluster-Proactive-Policies.md doc updated in this same PR correctly refers to spec.scope.namespaceSelector.values.
Why it matters: Users searching the docs for the correct field path will find two different names for the same concept (namespaces.values vs namespaceSelector.values), leading to confusion and likely misconfiguration.
Suggested fix: Align the heading and prose to match the canonical field name used in the CRD and other docs:
### Namespace wildcards in `scope[].namespaceSelector.values`
`scope[].namespaceSelector.values` supports shell-style `*` wildcards ...| type: object | ||
| workloadTypes: | ||
| default: | ||
| - Deployment |
There was a problem hiding this comment.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: High]
Location: charts/kubex-crds/templates/rightsizing.kubex.ai_podaffinitypolicies.yaml:131
Issue: The workloadTypes default list order differs between the new PodAffinityPolicy CRD and other policies in the chart. The CRD lists [Deployment, StatefulSet, CronJob, Rollout, Job, AnalysisRun, DaemonSet] but the workloadTypes items enum lists [Deployment, StatefulSet, DaemonSet, CronJob, Rollout, Job, AnalysisRun].
Why it matters: While functionally equivalent (defaults and enums are sets), inconsistency across CRDs in the same chart makes maintenance harder and can confuse users comparing schemas. It also signals a copy-paste from two different sources without reconciliation.
Suggested fix: Align the default list order with the items enum order, or align all CRDs in this chart to use the same canonical ordering (e.g., [Deployment, StatefulSet, DaemonSet, CronJob, Rollout, Job, AnalysisRun]).
This PR was created automatically to sync managed Kubex chart content.
Source commit:
f1e348fManaged chart scope:
charts/kubex-automation-enginecharts/kubex-crds