Capture all resources by WorkloadMonitors - #1024
Conversation
|
""" WalkthroughThis change updates Helm chart versions for multiple applications, standardizes Kubernetes resource labels to use Changes
Sequence Diagram(s)sequenceDiagram
participant Helm
participant Kubernetes
participant WorkloadMonitor
Helm->>Kubernetes: Deploys app resources with app.kubernetes.io/instance label
Helm->>Kubernetes: Deploys WorkloadMonitor CR with selector for instance label
Kubernetes->>WorkloadMonitor: WorkloadMonitor selects pods/resources using instance label
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/installer/values.yaml (1)
2-2: Avoid using ‘latest’ tag with digest for image pinning.Using the
latesttag undermines reproducibility—even with a digest—since the tag can float. Consider switching to a versioned tag (e.g.v0.31.2) or dropping the@sha256digest if you truly want to followlatest.packages/apps/tcp-balancer/templates/deployment.yaml (1)
18-18: Include instance label inspec.selector.matchLabelsfor clarity.The pod template adds both
appandapp.kubernetes.io/instance, but the Deployment’s selector only matches onapp. To avoid selector drift, add the instance label too:spec: selector: matchLabels: app: {{ .Release.Name }}-haproxy + app.kubernetes.io/instance: {{ .Release.Name }}packages/apps/vm-disk/templates/workloadmonitor.yaml (2)
3-5: Use Helm naming helper and add metadata labels for consistency.
- Replace raw release name with the chart’s fullname helper to match other resources.
- Add
metadata.labelsto facilitate resource tracking by instance.metadata: - name: {{ $.Release.Name }} + name: {{ include "vm-disk.fullname" . }} + labels: + app.kubernetes.io/instance: "{{ $.Release.Name }}"
10-12: Quote templated values and unify release context.
To prevent YAML parsing issues and ensure consistency, wrap templated outputs in quotes and use the global context ($) for release and chart references:selector: - app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/instance: "{{ $.Release.Name }}" -version: {{ $.Chart.Version }} +version: "{{ $.Chart.Version }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(3 hunks)packages/apps/clickhouse/templates/workloadmonitor.yaml(1 hunks)packages/apps/ferretdb/Chart.yaml(1 hunks)packages/apps/ferretdb/templates/external-svc.yaml(1 hunks)packages/apps/ferretdb/templates/postgres.yaml(1 hunks)packages/apps/http-cache/Chart.yaml(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/redis/Chart.yaml(1 hunks)packages/apps/tcp-balancer/Chart.yaml(1 hunks)packages/apps/tcp-balancer/templates/deployment.yaml(1 hunks)packages/apps/tcp-balancer/templates/workloadmonitor.yaml(1 hunks)packages/apps/versions_map(7 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(1 hunks)packages/apps/vm-disk/Chart.yaml(1 hunks)packages/apps/vm-disk/templates/dv.yaml(1 hunks)packages/apps/vm-disk/templates/workloadmonitor.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/vpn/Chart.yaml(1 hunks)packages/apps/vpn/charts/cozy-lib(1 hunks)packages/apps/vpn/templates/deployment.yaml(2 hunks)packages/apps/vpn/templates/service.yaml(1 hunks)packages/apps/vpn/templates/workloadmonitor.yaml(1 hunks)packages/core/installer/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (42)
packages/apps/ferretdb/Chart.yaml (1)
19-19: Correct patch version bump: The chartversionhas been incremented from0.6.0to0.6.1, which follows semantic versioning for a patch release and stays consistent with the coordinated chart updates in this PR.packages/apps/redis/Chart.yaml (1)
19-19: Correct patch version bump: The chartversionhas been incremented from0.7.0to0.7.1, following semantic versioning for a patch release and keeping in sync with the other Helm charts updated in this PR.packages/apps/clickhouse/Chart.yaml (1)
19-19: Correct patch version bump: The chartversionhas been updated from0.9.0to0.9.1, which is a proper patch-level increment and aligns with the batch of chart version updates in this PR.packages/apps/mysql/Chart.yaml (1)
19-19: Correct patch version bump: The chartversionhas been incremented from0.7.0to0.7.1, matching the coordinated version bumps across application charts in this PR.packages/apps/http-cache/Chart.yaml (1)
19-19: Correct patch version bump: The chartversionhas been updated from0.5.0to0.5.1, adhering to semantic versioning for patch releases and consistent with the other charts updated in this PR.packages/apps/tcp-balancer/Chart.yaml (1)
19-19: Chart version bump is appropriate
Updating the chart version to 0.4.1 correctly reflects the added WorkloadMonitor resources and label changes. Ensure any umbrella charts or deployment manifests that pin this chart are updated to reference the new version.packages/apps/vpn/charts/cozy-lib (1)
1-1:Details
✅ Verification successful
Include
cozy-libas a subchart via symlink
This symlink correctly pulls in the sharedcozy-libHelm library from../../../library/cozy-lib.To ensure the symlink resolves properly during chart packaging and templating, you can run:
🏁 Script executed:
#!/bin/bash # Description: Verify that the 'charts/cozy-lib' symlink points to the shared library directory. symlink="packages/apps/vpn/charts/cozy-lib" if [ -L "$symlink" ]; then target=$(readlink "$symlink") if [ "$target" = "../../../library/cozy-lib" ]; then echo "✔ Symlink target is correct: $target" else echo "✖ Unexpected symlink target: $target" exit 1 fi else echo "✖ '$symlink' is not a symbolic link" exit 1 fiLength of output: 354
Subchart symlink verified
The symlink atpackages/apps/vpn/charts/cozy-libcorrectly points to../../../library/cozy-lib. ✔️packages/apps/vpn/Chart.yaml (1)
19-19: Approve version bump and verifycozy-libdependency
The chart version bump from0.5.0to0.5.1is correct and aligns with the coordinated patch release. However, the AI summary mentions adding acozy-liblibrary dependency, but I don’t see adependencies:block here. Please ensure thecozy-libchart is declared underdependencies:or update the summary accordingly.Likely an incorrect or invalid review comment.
packages/apps/postgres/Chart.yaml (1)
19-19: Approve chart version bump
Updating the chart version from0.12.0to0.12.1is a valid patch increment that remains consistent with the other application charts.packages/apps/vm-instance/Chart.yaml (1)
20-20: Approve chart version bump
Bumping the chart version to0.7.1correctly follows semantic versioning and aligns with the coordinated updates across charts in this PR.packages/apps/virtual-machine/Chart.yaml (1)
20-20: Approve chart version bump
Updating the chart version from0.10.0to0.10.1is appropriate for a patch release and matches the pattern used elsewhere.packages/apps/vm-disk/Chart.yaml (1)
19-19: Approve chart version bump
The patch bump from0.1.1to0.1.2is correct and keeps the release in sync with the other application charts.packages/apps/ferretdb/templates/external-svc.yaml (1)
5-6: Standardize onapp.kubernetes.io/instancelabel key.Most charts in this PR adopt the
app.kubernetes.io/instancelabel for Helm‐scoped identification. Using a plainapplabel here breaks consistency. Confirm whether this should be:labels: app.kubernetes.io/instance: {{ .Release.Name }}packages/apps/ferretdb/templates/postgres.yaml (1)
38-38: Label addition looks good.The new
app: {{ .Release.Name }}underinheritedMetadata.labelsmatches the chart’s existing pattern and will propagate correctly to PostgreSQL resources.packages/apps/vm-instance/templates/dashboard-resourcemap.yaml (1)
25-25: WorkloadMonitor selector update is correct.Switching to the explicit
app.kubernetes.io/instance: {{ .Release.Name }}selector ensures the monitor captures all VM resources by Helm release.packages/apps/virtual-machine/templates/vm.yaml (1)
29-30: Standardize DataVolume labeling
Great addition of theapp.kubernetes.io/instance: {{ .Release.Name }}label underdataVolumeTemplates.metadata.labels. This aligns the DataVolume resources with the rest of the chart’s naming convention and ensures they can be selected by the WorkloadMonitor.packages/apps/vm-disk/templates/dv.yaml (1)
10-11: Add instance label to DataVolume metadata
Includingapp.kubernetes.io/instance: {{ .Release.Name }}in the DataVolume’smetadata.labelsfollows the chart-wide labeling standard, which is essential for consistent resource tracking and selection.packages/apps/vpn/templates/service.yaml (1)
8-8: Include Helm release instance label on Service
Addingapp.kubernetes.io/instance: {{ .Release.Name }}to the Service metadata standardizes identification of the VPN service alongside its peers in the cluster.packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
12-12: Update WorkloadMonitor selector to instance label
Switching the selector key toapp.kubernetes.io/instance: {{ $.Release.Name }}ensures the WorkloadMonitor targets the correct ClickHouse pods labeled under the new convention.packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml (1)
24-24: Align VirtualMachine WorkloadMonitor selector
Changing the selector toapp.kubernetes.io/instance: {{ .Release.Name }}matches the updated labels on the VirtualMachine resources, enabling accurate monitoring.packages/apps/clickhouse/templates/clickhouse.yaml (4)
95-98: Approve addingapp.kubernetes.io/instancelabel to data-volume-template. Standardizes PVC labeling by Helm release instance.
105-108: Approve addingapp.kubernetes.io/instancelabel to log-volume-template. Consistent PVC labeling aids resource organization and monitoring.
116-119: Approve addingapp.kubernetes.io/instancelabel to pod template. Ensure that the pod anti-affinity rule still matches your pods—either preserve theclickhouse.altinity.com/chilabel or update thematchExpressionsin the affinity to use this new label.
145-148: Approve addingapp.kubernetes.io/instancelabel to service template. This will ensure services carry the standard release-instance label.packages/apps/versions_map (11)
12-13: Approve version mapping update for clickhouse. Matches the Chart.yaml bump from 0.9.0 to 0.9.1.
22-23: Approve version mapping update for ferretdb. Updated to 0.6.0 → 0.6.1 HEAD consistent with chart version.
29-30: Approve version mapping update for http-cache. Aligned to new 0.5.0 commit and HEAD.
82-83: Approve version mapping update for mysql. Reflects 0.7.0 → HEAD.
108-109: Approve version mapping update for postgres. Correctly moves 0.12.0 → HEAD.
127-128: Approve version mapping update for redis. Bumped to 0.7.1 HEAD.
132-133: Approve version mapping update for tcp-balancer. 0.4.1 HEAD mapping is correct.
171-172: Approve version mapping update for virtual-machine. 0.10.1 HEAD entry matches Chart.yaml.
174-175: Approve version mapping update for vm-disk. 0.1.2 HEAD is properly appended.
184-185: Approve version mapping update for vm-instance. Updated to 0.7.1 HEAD correctly.
191-192: Approve version mapping update for vpn. 0.5.1 HEAD entry aligns with Chart bump.packages/apps/vpn/templates/deployment.yaml (3)
17-17: Approve addingapp.kubernetes.io/instancelabel to pod template metadata. This will improve resource identification across the cluster.
47-47: Approve use ofcozy-lib.resources.sanitizefor resource definitions. Leveraging the shared library ensures consistent resource formatting.
49-49: Approve use ofcozy-lib.resources.presetfor preset resource configurations. Good abstraction of resource presets.packages/apps/vpn/templates/workloadmonitor.yaml (1)
1-12: Approve addition ofWorkloadMonitorresource for VPN workloads. This CRD will enable automated scaling and monitoring using the standard release-instance selector.packages/apps/tcp-balancer/templates/workloadmonitor.yaml (1)
1-14: Approve addition ofWorkloadMonitorresource for TCP Balancer workloads. Consistent with other apps, using theapp.kubernetes.io/instanceselector and chart version.packages/apps/vm-disk/templates/workloadmonitor.yaml (2)
1-2: Verify API version and resource kind consistency.
TheapiVersion: cozystack.io/v1alpha1andkind: WorkloadMonitoralign with existing CRDs for workload monitoring. No issues detected.
6-9: Validate default replica settings and workload identifiers.
Settingreplicas: 0andminReplicas: 0correctly initializes the monitor in an idle state. Thespec.kindandspec.typefields accurately reflect thevm-diskworkload. No changes needed.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
c065c1c to
b140f1b
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores