Repository navigation
feat(operator): add per-package upgradeCRDs policy for HelmRelease - #2427
Conversation
dosubot
Bot
added
the
size/S
Summary of ChangesHello, 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 introduces a mechanism to manage CRD upgrades for Helm releases within the Cozystack operator. By adding an opt-in Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
dosubot
Bot
added
the
kind/feature
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User as PackageSource spec
participant Reconciler as Operator Reconciler
participant K8s as Kubernetes API
participant HelmCtrl as helm-controller (helmv2)
User->>Reconciler: ComponentInstall with install.upgradeCRDs
Reconciler->>Reconciler: parseCRDPolicy(UpgradeCRDs)
Reconciler->>K8s: Apply/Update HelmRelease (Spec.Upgrade.CRDs set)
HelmCtrl->>K8s: On HelmRelease upgrade, apply CRDs per Spec.Upgrade.CRDs
K8s-->>HelmCtrl: CRD create/replace/skip responses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the UpgradeCRDs field to the PackageSource API, allowing users to specify how CRDs are handled during Helm upgrades with options for Skip, Create, or CreateReplace. The implementation includes updates to the API types, documentation, generated manifests, and the operator's reconciliation logic. Review feedback identifies a violation of the repository's conventional commit scope requirements and suggests a minor code simplification in the reconciler to remove a redundant nil check.
| // loss if upstream drops fields from a CRD with live objects. | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=Skip;Create;CreateReplace | ||
| UpgradeCRDs string `json:"upgradeCRDs,omitempty"` |
There was a problem hiding this comment.
The commit message uses the scope operator, which is not listed as a valid scope in the repository style guide (lines 42-45). Please consider using a valid meta scope like api or a system scope like platform, or update the style guide if operator is intended to be a permanent scope.
References
- The repository style guide defines a specific list of valid scopes for conventional commits. (link)
There was a problem hiding this comment.
The Scopes section in docs/agents/contributing.md was intended as a list of examples, not an exhaustive enumeration. Reworded the section to make that explicit and added operator as an example scope (e52cf38). Contributors should pick the most specific scope and extend the list when a genuinely new area appears.
There was a problem hiding this comment.
Also mirrored the same wording in .github/PULL_REQUEST_TEMPLATE.md for consistency with human contributors (57e4859).
| // Empty / nil preserves the helm-controller default (Skip on upgrade); | ||
| // the CRD enum marker restricts the string to Skip/Create/CreateReplace. | ||
| func parseCRDPolicy(install *cozyv1alpha1.ComponentInstall) helmv2.CRDsPolicy { | ||
| if install == nil || install.UpgradeCRDs == "" { |
There was a problem hiding this comment.
The install == nil check is redundant here because the only caller of parseCRDPolicy (at line 234) is inside a loop where component.Install has already been verified as non-nil (at line 162). While safe, it can be simplified for clarity.
| if install == nil || install.UpgradeCRDs == "" { | |
| if install.UpgradeCRDs == "" { |
There was a problem hiding this comment.
Keeping the install == nil branch deliberately. While the sole current caller in Reconcile guards the nil via if component.Install == nil { continue }, treating parseCRDPolicy as safe for nil input keeps the helper independently correct. A future caller that skips the outer guard would otherwise panic on a deref with no hint that the helper relied on an external invariant — two lines of defensive code are a cheap price for that decoupling.
dosubot
Bot
added
size/M
Add an opt-in UpgradeCRDs field to ComponentInstall that maps to HelmRelease.Spec.Upgrade.CRDs, allowing a PackageSource component to declare how Flux should handle CRDs from the chart's crds/ directory on upgrade. The helm-controller default on upgrade is Skip, which means new CRDs added between chart versions never reach existing clusters and must be applied manually. Setting upgradeCRDs: CreateReplace makes Flux apply new CRDs declaratively with the chart. Allowed values are restricted to Skip, Create, CreateReplace via a kubebuilder enum marker. Empty / unset preserves the existing Flux default, so all existing PackageSource resources keep working. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Describe when to set upgradeCRDs: CreateReplace (operators that evolve their CRD set additively between versions) and the data-loss risk of enabling it on operators that drop fields. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The Scopes section was read as an exhaustive enumeration, which led to review feedback flagging any scope outside the list as invalid. The intent has always been that contributors pick the most specific scope for the change and extend the list when a genuinely new area appears. Reword the section accordingly and add operator as an example scope. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Match the wording adopted in docs/agents/contributing.md so that human contributors and AI agents see the same guidance in both places. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
57e4859 to
32ae993
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/operator/package_reconciler_test.go (1)
78-138: Solid guard against silent CRD-schema drift.Walking the OpenAPI schema down to
spec.variants.items.components.items.install.upgradeCRDsand asserting all three enum entries effectively catches an accidental removal of the kubebuilder marker followed by a CRD regeneration. Thet.Fatalon missing field plus per-valuet.Errorfgive clear failure messages.One minor nit (optional): the loop over
crd.Spec.Versionscontinues past any version missing the field, so ifv1alpha1ever ships alongside another version that lacksupgradeCRDs, the test would still pass via whichever version has it. Not actionable today (onlyv1alpha1exists), just noting for future-proofing — pinning tov.Name == "v1alpha1"would make intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator/package_reconciler_test.go` around lines 78 - 138, TestPackageSourceCRDHasUpgradeCRDsEnum currently scans all crd.Spec.Versions and accepts the first version that contains the upgradeCRDs field, which could mask cases where other versions lack it; narrow the search to the intended version by only inspecting versions with v.Name == "v1alpha1" (or explicitly the desired version string) inside the loop over crd.Spec.Versions so the assertion targets that specific API version (update the loop/filter around crd.Spec.Versions and the logic that finds spec.Properties["spec"] ... install.Properties["upgradeCRDs"] accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/operator/package_reconciler_test.go`:
- Around line 78-138: TestPackageSourceCRDHasUpgradeCRDsEnum currently scans all
crd.Spec.Versions and accepts the first version that contains the upgradeCRDs
field, which could mask cases where other versions lack it; narrow the search to
the intended version by only inspecting versions with v.Name == "v1alpha1" (or
explicitly the desired version string) inside the loop over crd.Spec.Versions so
the assertion targets that specific API version (update the loop/filter around
crd.Spec.Versions and the logic that finds spec.Properties["spec"] ...
install.Properties["upgradeCRDs"] accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c46aaf4-4902-4180-95f9-2ed7f859e9b1
📒 Files selected for processing (7)
.github/PULL_REQUEST_TEMPLATE.mdapi/v1alpha1/packagesource_types.godocs/agents/contributing.mddocs/agents/overview.mdinternal/crdinstall/manifests/cozystack.io_packagesources.yamlinternal/operator/package_reconciler.gointernal/operator/package_reconciler_test.go
✅ Files skipped from review due to trivial changes (3)
- docs/agents/contributing.md
- .github/PULL_REQUEST_TEMPLATE.md
- docs/agents/overview.md
…2428) ## What this PR does Bumps the vendored `etcd-operator` chart from `v0.4.2` to `v0.4.3` and wires the new backup machinery through the cozystack `Etcd` app API. Upstream v0.4.3 ships two new CRDs — `EtcdBackup` (one-shot snapshot) and `EtcdBackupSchedule` (periodic snapshots) — along with the `EtcdCluster.spec.bootstrap` restore field, a new `OPERATOR_IMAGE` env on the operator deployment, and a replacement of the deprecated `gcr.io/kubebuilder/kube-rbac-proxy` image. The chart portion of this PR is a clean re-vendor via `make update` — no local patches. On top of the bump, `packages/extra/etcd` exposes a new optional `backup.*` values block. When `backup.enabled=true`, the chart renders an `EtcdBackupSchedule` referencing the platform `EtcdCluster/etcd` and a `Secret/etcd-s3-creds` with the S3 credentials expected by the backup-agent. The values field names (`destinationPath`, `endpointURL`, `s3AccessKey`, `s3SecretKey`, `schedule`) mirror the existing `postgres` backup pattern for consistency. The chart `fail`s at template render if any of the four required S3 fields are empty or if `destinationPath` is malformed (missing `s3://` prefix, `//` segments, no bucket). Test coverage: 15 `helm-unittest` cases across `tests/backup-secret_test.yaml` and `tests/etcd-backup-schedule_test.yaml` (wired into `make test`, picked up by `hack/helm-unit-tests.sh`), plus three bats cases in `hack/e2e-apps/etcd.bats` covering the baseline `Etcd` app, the `backup: {}` default-off path, and the full `backup.enabled=true` case that verifies the `EtcdBackupSchedule` materializes and the operator generates the downstream `CronJob`. `packages/core/platform/sources/etcd-operator.yaml` now sets `upgradeCRDs: CreateReplace` on the etcd-operator component, so Flux applies the new CRDs on upgrade of the chart instead of silently skipping them. Depends on #2427. ### Scope deferred to follow-ups - The one-shot `EtcdBackup` CRD is vendored but not exposed through this chart — users can still hand-apply the CRD directly. - `EtcdCluster.spec.bootstrap` (restore from an existing snapshot) is likewise vendored but not exposed. Both gaps are called out in `packages/extra/etcd/README.md`. ### Release note ```release-note feat(etcd): update etcd-operator to v0.4.3 and expose S3 backup schedules on the Etcd app. Set `backup.enabled=true` along with `backup.s3AccessKey`, `backup.s3SecretKey`, `backup.destinationPath`, and `backup.endpointURL` to schedule periodic snapshots. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Scheduled S3 backups for etcd clusters (configurable cron, generated S3 credentials) and restore/init from S3 or PVC via new backup resources. * **Tests** * New end-to-end tests for etcd lifecycle and backup scenarios; added Helm unit tests for backup templates and validations. * **Documentation** * Backup configuration guide and parameter reference added. * **Chores** * Operator chart version bump; RBAC, webhooks, and deployment wiring updated; added chart test target. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…interval, timeouts, max-history) (#2509) ## Why The `cozystack-operator` Package reconciler builds every `HelmRelease` it generates with a handful of hardcoded values. Three of those have caused recurring pain across the project: - **`Spec.Interval = 5m`** (line 220 before this PR) — too long for E2E install where dependent HRs reconcile out of order; previously addressed by a manual `flux reconcile --force` workaround in `hack/e2e-install-cozystack.bats`. - **`Install.Remediation{Retries: -1}` + `Upgrade.Remediation{Retries: -1}`** — semantically "retry forever, never remediate", but the retry-on-failure timing was tied to `Spec.Interval` (5m). Audit of the last 30 successful PR runs found that **every** run hit `kubectl wait hr/seaweedfs-system` timeouts at the 2-min mark, recovering only after the inline `flux reconcile --force` workaround. - **`Install.Timeout = 10m` / `Upgrade.Timeout = 10m`** — directly contradicts the per-Application timeout work in `pkg/config/config.go` + `pkg/registry/apps/application/rest.go` (commit `7b146cbe`). `packages/apps/tenant/templates/etcd.yaml` hand-rolls its HR YAML to bypass this for cert rotation; harbor has had multiple commits chasing the same problem. - **`Spec.MaxHistory`** unset → Helm default 5. Test clusters bounce HRs frequently and accumulate release-history Secrets. This PR exposes all four as operator flags + helm chart values, with defaults that preserve current behavior. ## What ### New operator flags | Flag | Default | HR field | |---|---|---| | `--helmrelease-interval` | `5m` | `Spec.Interval` | | `--helmrelease-retry-interval` | `30s` | `Spec.{Install,Upgrade}.Strategy.RetryInterval` | | `--helmrelease-install-timeout` | `10m` | `Spec.Install.Timeout` | | `--helmrelease-upgrade-timeout` | `10m` | `Spec.Upgrade.Timeout` | | `--helmrelease-max-history` | `5` | `Spec.MaxHistory` | ### Strategy switch `Install.Strategy.Name` and `Upgrade.Strategy.Name` are now set to `RetryOnFailure` (instead of leaving the default `RemediateOnFailure`), with `RetryInterval` set from the new flag. Functionally equivalent to the previous `Remediation{Retries:-1}` ("retry forever") since remediation never fired anyway, but **decouples retry-on-failure timing from `Spec.Interval`** so failed releases recover at 30s without polling healthy releases at the same fast cadence. ### Chart exposure `packages/core/installer/values.yaml` gets five new optional values (`cozystackOperator.helmRelease{Interval,RetryInterval,InstallTimeout,UpgradeTimeout,MaxHistory}`), each defaulting to empty so the operator's own default applies. The chart conditionally renders the corresponding flag only when the value is set. ## Production safety - All static defaults match the previous hardcoded values (5m / 10m / 10m / 5). No new code path runs until an admin explicitly sets a flag. - **One intentional behavior change: failed-release retry cadence** drops from ~5m (previously coupled to `Spec.Interval`) to 30s (`RetryInterval` default). This is the whole motivation for the strategy switch — the recurring `kubectl wait hr/seaweedfs-system` flakes documented above were caused by the previous coupling. Healthy-release reconcile cadence is unchanged at 5m in production; CI overrides this to 30s via `--set cozystackOperator.helmReleaseInterval=30s` in `hack/e2e-install-cozystack.bats` for faster convergence on E2E clusters. - `RetryOnFailure` is functionally equivalent to `RemediateOnFailure + Retries:-1` since neither ever triggers remediation. - Empty value in the chart → flag not rendered → operator falls back to its built-in default. (`helmReleaseMaxHistory: 0` — unlimited history per Helm semantics — is also honored; the chart guards on `ne (toString …) ""` rather than truthiness so int `0` is not silently dropped.) ## Verification - `go build ./...` clean - `go test ./internal/operator/...` passes (covers `Interval`, `MaxHistory`, `Install/Upgrade.Timeout`, `Strategy.Name`, `RetryInterval`, `ChartRef` propagation, plus a regression test for `MaxHistory=0`) - `helm template installer packages/core/installer` renders cleanly with default and overridden values; `--set cozystackOperator.helmReleaseMaxHistory=0` correctly emits `--helmrelease-max-history=0` ## Followups (deliberately not bundled) - **Per-component timeout overrides** on `ComponentInstall` (mirroring the `UpgradeCRDs` pattern from #2427). Charts like etcd that need a 30m timeout would set it on their `Component` rather than tuning a global flag. Touches the Package CR API + codegen — bigger lift, separate PR. - **`Remediation.Retries` as a flag.** No current pain; skip. - **`createOrUpdateHelmRelease` field preservation.** Currently preserves `Suspend` but not `Test`/`Rollback`/`Uninstall`. Would silently overwrite an SRE's manual edit. Minor cleanliness, separate PR. ## Test plan - [ ] CI passes E2E with `helmReleaseInterval=30s` set in install bats (existing T1 test path) - [ ] No regression on production install path (default values) - [ ] HR objects in a healthy cluster render with `Strategy.Name=RetryOnFailure` and `RetryInterval=30s` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Operator and Helm chart now accept CLI flags and Helm values to configure HelmRelease reconciliation intervals, retry behavior, install/upgrade timeouts, and history retention. * **Bug Fixes** * Operator now validates HelmRelease timing/history inputs on startup and will exit if values are invalid or non-positive. * **Tests** * Added unit and e2e coverage to verify HelmRelease configuration and a chart install path using a custom reconciliation interval. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
What this PR does
Add an opt-in
upgradeCRDsfield toComponentInstallinPackageSource. The field maps directly toHelmRelease.Spec.Upgrade.CRDsso a component author can declare how Flux should handle CRDs from the chart'scrds/directory when the release is upgraded.The helm-controller default on upgrade is
Skip, which means CRDs added by a chart bump never reach clusters that already have the release installed — they must be applied manually withkubectl apply --filename charts/.../crds/. This surfaces on every upgrade of an operator whose CRD set expands between versions (etcd-operator, cnpg, kubevirt, kamaji, etc.).Setting
upgradeCRDs: CreateReplacelets Flux apply new CRDs declaratively with the chart.Values are restricted to
Skip,Create,CreateReplacevia a kubebuilder enum marker. Empty / unset preserves the existing helm-controller default, so every currentPackageSourcekeeps working unchanged.Migration is out of scope here — follow-ups will opt individual packages in case-by-case.
Relation to existing CRD management approach
The project convention (per #377) is to extract CRDs into a dedicated Helm chart that reconciles ahead of the operator chart. This PR does not replace that pattern — it complements it for charts that keep CRDs inline under
charts/<name>/crds/where extraction isn't practical (vendored upstream charts with tightly coupled CRDs). Packages that already split CRDs out can leaveupgradeCRDsunset and keep using their existing separate chart.Release note
Summary by CodeRabbit
New Features
Documentation
Tests