Upd Velero v1.17.0 - #1484
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughHelm Velero chart bumped to appVersion 1.17.0 / chart 11.0.0. CRDs updated (new fields: cancel, progress, acceptedTimestamp, node, volumeGroupSnapshotLabelKey, csiSnapshot.driver; adjusted required fields). Helm templates/values/schema/docs add repo maintenance ConfigMap wiring, resizePolicy, dual-stack service options, and template/variable tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Helm as Helm (install/upgrade)
participant Chart as Velero Chart
participant K8s as Kubernetes API
User->>Helm: helm upgrade --install
Helm->>Chart: render templates with values
alt repositoryConfigData present
Chart->>K8s: create ConfigMap repo-maintenance-config
Chart->>K8s: Deployment velero w/ --repo-maintenance-job-configmap=<name>
else
Note right of Chart: omit ConfigMap & flag
end
opt metrics.ipFamily settings
Chart->>K8s: Service with ipFamilyPolicy/ipFamilies
end
opt resizePolicy provided
Chart->>K8s: Deployment/DaemonSet include resizePolicy
end
Chart->>K8s: apply CRDs (updated schemas)
sequenceDiagram
autonumber
participant Controller as Velero Controller
participant PVB as PodVolumeBackup CR
participant Worker as Node Agent
participant Store as Backup Storage
Controller->>PVB: create/update (phase -> Accepted)
PVB-->>Controller: status.acceptedTimestamp set
Controller->>Worker: assign backup (status.node)
Worker->>PVB: phase -> Prepared -> InProgress
Worker->>PVB: update status.progress(bytesDone/totalBytes)
alt user requests cancel
User->>PVB: set spec.cancel = true
Worker->>PVB: phase -> Canceling -> Canceled
else complete
Worker->>Store: upload data
Worker->>PVB: phase -> Completed
end
sequenceDiagram
autonumber
participant Controller as Velero Controller
participant PVR as PodVolumeRestore CR
participant Worker as Node Agent
participant Store as Backup Storage
Controller->>PVR: create/update (phase -> Accepted)
PVR-->>Controller: status.acceptedTimestamp set
Controller->>Worker: assign restore (status.node)
Worker->>PVR: phase -> Prepared -> InProgress
Worker->>Store: read data
alt user requests cancel
User->>PVR: set spec.cancel = true
Worker->>PVR: phase -> Canceling -> Canceled
else complete
Worker->>PVR: phase -> Completed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🪛 YAMLlint (1.37.1)packages/system/velero/charts/velero/templates/repo-maintenance-configmap.yaml[error] 1-1: syntax error: expected the node content, but found '-' (syntax) ⏰ 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). (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. Comment |
Summary of ChangesHello Andrei Kvapil (@kvaps), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a significant upgrade of the Velero application and its Helm chart to version 1.17.0. The primary goal is to incorporate the latest features and improvements from Velero, including a more robust and configurable system for managing repository maintenance jobs. It also updates several Custom Resource Definitions to support new functionalities and provides various Helm chart enhancements for better deployment and operational flexibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates Velero to v1.17.0 and its Helm chart to v11.0.0, introducing several new features and breaking changes. My review focuses on the correctness and maintainability of the Helm chart updates. I've identified a critical issue where a resource name is hardcoded, which prevents multiple chart installations in the same namespace. Additionally, there are several undocumented breaking changes in the values.yaml structure that will negatively impact users upgrading their releases. I've provided suggestions to add validation checks to NOTES.txt to mitigate this. I also found a minor opportunity to improve code readability in one of the templates.
| kind: Deployment | ||
| metadata: | ||
| name: {{ include "velero.fullname" . }} | ||
| name: velero |
There was a problem hiding this comment.
The deployment name has been hardcoded to velero. This is a critical issue as it prevents installing multiple Velero releases in the same namespace. Each new installation will overwrite the velero deployment of the previous one, breaking existing releases. The name should be unique per release, for example by using {{ include "velero.fullname" . }}. While the pod labels and service selectors also use a hardcoded name: velero label, which is a pre-existing issue, hardcoding the deployment name makes the problem worse by causing direct resource conflicts.
name: {{ include "velero.fullname" . }}|
|
||
| {{- if hasKey .Values.configuration.repositoryMaintenanceJob "latestJobsCount" }} | ||
| {{- $breaking = print $breaking "\n\nREMOVED: configuration.repositoryMaintenanceJob.latestJobsCount has been removed, please use the configmap" }} | ||
| {{- end }} |
There was a problem hiding this comment.
Several breaking changes have been introduced to the values.yaml structure but are not validated here. This can lead to silent failures or unexpected behavior for users upgrading their release. Please add checks for the following breaking changes:
.Values.namespace.labelschanged from a list of objects to a map of strings..Values.hostAliasesfor the node-agent is now under.Values.nodeAgent.hostAliases..Values.runtimeClassNamefor the node-agent is now under.Values.nodeAgent.runtimeClassName.
I suggest adding checks for these to fail the deployment with a clear error message.
{{- end }}
{{- if .Values.namespace.labels }}
{{- if not (kindIs "map" .Values.namespace.labels) }}
{{- $breaking = print $breaking "\n\nERROR: .Values.namespace.labels must be a map of key-value pairs. The list format is no longer supported." }}
{{- end }}
{{- end }}
{{- if and .Values.deployNodeAgent .Values.hostAliases }}
{{- $breaking = print $breaking "\n\nREMOVED: .Values.hostAliases for node-agent has been moved to .Values.nodeAgent.hostAliases" }}
{{- end }}
{{- if and .Values.deployNodeAgent .Values.runtimeClassName }}
{{- $breaking = print $breaking "\n\nREMOVED: .Values.runtimeClassName for node-agent has been moved to .Values.nodeAgent.runtimeClassName" }}
{{- end }}
| {{- if and .repositoryConfigData (or .repositoryConfigData.global .repositoryConfigData.repositories) }} | ||
| - --repo-maintenance-job-configmap={{ default "velero-repo-maintenance" .repositoryConfigData.name }} | ||
| {{- else }} | ||
| {{- end }} |
There was a problem hiding this comment.
The {{- else }} block is empty. For better readability and conciseness, you can remove the empty else block.
{{- if and .repositoryConfigData (or .repositoryConfigData.global .repositoryConfigData.repositories) }}
- --repo-maintenance-job-configmap={{ default "velero-repo-maintenance" .repositoryConfigData.name }}
{{- end }}There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/system/velero/charts/velero/Chart.yaml(2 hunks)packages/system/velero/charts/velero/README.md(1 hunks)packages/system/velero/charts/velero/crds/backuprepositories.yaml(1 hunks)packages/system/velero/charts/velero/crds/backups.yaml(1 hunks)packages/system/velero/charts/velero/crds/datauploads.yaml(1 hunks)packages/system/velero/charts/velero/crds/podvolumebackups.yaml(4 hunks)packages/system/velero/charts/velero/crds/podvolumerestores.yaml(4 hunks)packages/system/velero/charts/velero/crds/schedules.yaml(1 hunks)packages/system/velero/charts/velero/templates/NOTES.txt(1 hunks)packages/system/velero/charts/velero/templates/deployment.yaml(5 hunks)packages/system/velero/charts/velero/templates/label-namespace/labelnamespace.yaml(1 hunks)packages/system/velero/charts/velero/templates/node-agent-daemonset.yaml(3 hunks)packages/system/velero/charts/velero/templates/repo-maintenance-configmap.yaml(1 hunks)packages/system/velero/charts/velero/templates/service.yaml(1 hunks)packages/system/velero/charts/velero/values.schema.json(7 hunks)packages/system/velero/charts/velero/values.yaml(11 hunks)packages/system/velero/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/system/velero/charts/velero/templates/repo-maintenance-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/system/velero/charts/velero/templates/label-namespace/labelnamespace.yaml (1)
34-35: Key/value binding fix looks goodThe explicit
$key/$valuebinding keeps the template robust across Helm versions; no issues spotted.
| kind: Deployment | ||
| metadata: | ||
| name: {{ include "velero.fullname" . }} | ||
| name: velero |
There was a problem hiding this comment.
Revert to templated Deployment name
Hard-coding metadata.name to velero breaks existing releases that rely on fullnameOverride/nameOverride and prevents installing two releases in the same namespace. Helm will treat this as a resource rename during upgrade, delete the old Deployment, and create a new one, causing downtime and leaving other resources (e.g., RBAC) under the old name. Please restore the templated name (e.g., {{ include "velero.fullname" . }}) to preserve backward compatibility.
🤖 Prompt for AI Agents
In packages/system/velero/charts/velero/templates/deployment.yaml around line 6,
metadata.name is hard-coded to "velero" which breaks
fullnameOverride/nameOverride and prevents multiple releases; change it back to
use the chart fullname template (e.g., replace the hard-coded name with the
template include that returns the chart's full name) so Helm honors
fullnameOverride/nameOverride and avoids resource renames during upgrades.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
ed9fdd2 to
4afda63
Compare
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
velerodoes not start #1442Release note
Summary by CodeRabbit
New Features
Changes
Documentation