feat(vpa): separate-crds - #781
Conversation
WalkthroughThe changes add a new release configuration for the Changes
Sequence Diagram(s)sequenceDiagram
participant D as Deployment Manager
participant C as Cilium
participant K as Kubeovn
participant CR as VPA-CRDs
participant MA as Monitoring Agents
participant V as VPA
D->>C: Ensure dependency is installed
D->>K: Ensure dependency is installed
D->>CR: Deploy CRDs (waiting on C & K)
D->>MA: Deploy Monitoring Agents
D->>V: Deploy VPA (requires MA and CR)
Poem
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:
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 (7)
packages/system/vertical-pod-autoscaler/values.yaml (1)
2-3: New CRDs Configuration Option AddedThe addition of the
crds.enabled: falseoption undervertical-pod-autoscalerprovides a clean and explicit way to control the deployment behavior of CRDs. Make sure that any component consuming this configuration properly handles the flag.packages/system/vertical-pod-autoscaler-crds/Makefile (1)
6-7: Enhance the CRD Update TargetThe Makefile target for updating the CRD template is well structured. However, to improve robustness, consider adding the
--failflag to thecurlcommand so that download errors are caught and reported.-curl -o ./templates/vpa-v1-crd-gen.yaml https://raw.githubusercontent.com/kubernetes/autoscaler/refs/heads/master/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yaml +curl --fail -o ./templates/vpa-v1-crd-gen.yaml https://raw.githubusercontent.com/kubernetes/autoscaler/refs/heads/master/vertical-pod-autoscaler/deploy/vpa-v1-crd-gen.yamlpackages/core/platform/bundles/paas-hosted.yaml (1)
258-263: New CRDs Release Configuration – Privileged Mode ReviewThe new
vertical-pod-autoscaler-crdsrelease is defined correctly with dependencies onciliumandkubeovn. However, it is marked asprivileged: true. Verify whether elevated privileges are truly necessary for deploying CRDs, as following the principle of least privilege is recommended.packages/core/platform/bundles/paas-full.yaml (1)
402-407: Review Privileged Flag on CRDs ReleaseSimilar to the hosted bundle, the
vertical-pod-autoscaler-crdsrelease in this file is also configured withprivileged: true. Consider evaluating if this elevated security context is necessary solely for the CRD definitions.packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml (3)
1-121: Comprehensive CRD for VerticalPodAutoscalerCheckpoint.
The definition for theVerticalPodAutoscalerCheckpointCRD is very detailed and follows Kubernetes API conventions. The schema properly declares themetadata,spec, andstatussections and includes helpful descriptions and types.Consider generating or validating these schemas via automated tooling (e.g., controller-gen) to ease long-term maintenance if the API evolves.
78-80: Typo in Property Description.
In the description for thefirstSampleStartfield, "fist sample" appears instead of "first sample." A quick fix here will improve clarity.
585-835: Properly Marked Deprecated v1beta2 Version.
The deprecatedv1beta2version of theVerticalPodAutoscalerCRD is correctly annotated with a deprecation warning and its served/storage flags set to false. This is a sound strategy to guide users toward the more stablev1version. Consider planning for its removal in a future release once migration is confirmed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)packages/system/vertical-pod-autoscaler-crds/Chart.yaml(1 hunks)packages/system/vertical-pod-autoscaler-crds/Makefile(1 hunks)packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml(1 hunks)packages/system/vertical-pod-autoscaler/values.yaml(1 hunks)
🔇 Additional comments (5)
packages/system/vertical-pod-autoscaler-crds/Chart.yaml (1)
1-3: Helm Chart Configuration Looks CorrectThe Chart file is concise and correctly defines a Helm v2 chart with a placeholder version. Ensure that your build process reliably replaces the placeholder version as intended.
packages/core/platform/bundles/paas-hosted.yaml (1)
251-257: Update to Vertical Pod Autoscaler DependencyThe
vertical-pod-autoscalerrelease has been updated to include a dependency onvertical-pod-autoscaler-crdsin itsdependsOnlist. This change should ensure that the CRDs are deployed before the autoscaler.packages/core/platform/bundles/paas-full.yaml (1)
395-401: Consistent Update for Vertical Pod Autoscaler DependencyThe
vertical-pod-autoscalerrelease now depends onvertical-pod-autoscaler-crdsalong withmonitoring-agents. This update aligns with the separation-of-concerns introduced in this PR. Please ensure that this new dependency order is intentional across all deployment bundles.packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml (2)
222-224: Effective Use of Document Separator.
The use of the YAML document separator (---on line 222) clearly divides the two CRD definitions, enhancing readability and logical structure.
223-584: Robust VerticalPodAutoscaler CRD (v1) Definition.
TheVerticalPodAutoscalerCRD for versionv1is defined with extensive details—including additional printer columns, explicit enumeration for update modes, and a well-structured schema for bothspecandstatussections. Everything appears compliant with Kubernetes API conventions.
d21bc81 to
4efd842
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Please add vertical-pod-autoscaler-crds dependency for monitoring-agents
Signed-off-by: kklinch0 <kklinch0@gmail.com>
4efd842 to
c31a771
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml (1)
1-222: New CRD: VerticalPodAutoscalerCheckpointThis new CustomResourceDefinition for
VerticalPodAutoscalerCheckpointis comprehensive and leverages OpenAPI v3 schema validation effectively. A couple of points for improvement:
- The v1 version is marked as served and storage while the v1beta2 version is not; if v1beta2 is not intended for active use, consider adding an explicit deprecation note.
- There is a typographical error in the description for the
firstSampleStartproperty ("fist sample" should be "first sample").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/platform/bundles/paas-full.yaml(2 hunks)packages/core/platform/bundles/paas-hosted.yaml(2 hunks)packages/system/vertical-pod-autoscaler-crds/Chart.yaml(1 hunks)packages/system/vertical-pod-autoscaler-crds/Makefile(1 hunks)packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml(1 hunks)packages/system/vertical-pod-autoscaler/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/system/vertical-pod-autoscaler/values.yaml
- packages/system/vertical-pod-autoscaler-crds/Makefile
- packages/system/vertical-pod-autoscaler-crds/Chart.yaml
- packages/core/platform/bundles/paas-hosted.yaml
🔇 Additional comments (3)
packages/core/platform/bundles/paas-full.yaml (2)
114-120: Updated Dependency for monitoring-agents ReleaseThe dependency list for the
monitoring-agentsrelease has been updated to includevertical-pod-autoscaler-crdsand removeciliumandkubeovn. This change appears to be intentional for ensuring that the new CRDs are deployed before monitoring components run.
402-408: New Release Configuration: vertical-pod-autoscaler-crdsA new release configuration for
vertical-pod-autoscaler-crdsis added with the expected fields:releaseName,chart,namespace,privileged, and a dependency on[cilium, kubeovn]. Please verify that these dependency choices and the chosen namespace align with the overall deployment order and environment consistency.packages/system/vertical-pod-autoscaler-crds/templates/vpa-v1-crd-gen.yaml (1)
223-835: New CRD: VerticalPodAutoscalerThe CRD defining
VerticalPodAutoscaleris detailed and follows Kubernetes API standards. Key points include:
- The inclusion of additional printer columns improves usability when inspecting resources.
- The v1 version is properly set as served and storage, with the v1beta2 version clearly marked as deprecated and accompanied by a deprecation warning.
- The schema makes good use of OpenAPI validations and flexible types (via
x-kubernetes-preserve-unknown-fields) where necessary.Overall, this definition aligns well with best practices.
Summary by CodeRabbit
VerticalPodAutoscalerCheckpointandVerticalPodAutoscaler.