Repository navigation
Add vm-default-images package - #2258
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds vm-disk cloning via a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI / Dashboard"
participant Override as "dashboard/customformsoverride"
participant API as "Kubernetes API / Cozystack APIs"
participant DV as "DataVolume / vmdisk resources"
rect rgba(135,206,250,0.5)
UI->>Override: Request form schema for VMDisk
end
rect rgba(144,238,144,0.5)
Override->>API: Query storageClasses (listInput)
Override->>API: Query default images -> /apis/.../datavolumes?label=vm-default-images
Override->>API: Query existing vmdisks -> /apis/.../vmdisks
end
rect rgba(255,228,181,0.5)
API-->>Override: Return lists (storageClasses, images, vmdisks)
Override-->>UI: Return enriched form schema with listInputs for image.name and disk.name
end
UI->>API: Create VMDisk / DataVolume based on selected source
API->>DV: Create DataVolume (uses vm-default-images- or vm-disk- PVC refs)
DV-->>API: Provisioned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
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 new 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
|
![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 a new vm-default-images package to provide a set of default VM images that can be used across the cluster. The changes are well-integrated, updating the vm-disk application to utilize these images through a UI dropdown and adding support for cloning from PVCs. The implementation includes necessary code modifications, new tests, and documentation updates. My review has identified a couple of incorrect URLs in the default image list that need to be corrected to ensure the images can be downloaded successfully.
| architecture: amd64 | ||
| description: "Debian 13 (Trixie) generic cloud image" | ||
| - name: centos-stream-9 | ||
| url: https://cloud.debian.org/images/cloud/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-x86_64-9-latest.x86_64.qcow2 |
There was a problem hiding this comment.
The URL for the CentOS Stream 9 image appears to be incorrect. It points to cloud.debian.org, which is unlikely to host CentOS images. The domain should be cloud.centos.org, and the path needs adjustment as well.
url: https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-9-latest.x86_64.qcow2There was a problem hiding this comment.
This was already fixed in 551b4e6 — CentOS Stream 9 and 10 URLs now point to cloud.centos.org with correct paths.
| architecture: amd64 | ||
| description: "CentOS Stream 9 GenericCloud image" | ||
| - name: centos-stream-10 | ||
| url: https://cloud.debian.org/images/cloud/centos/10-stream/x86_64/images/CentOS-Stream-GenericCloud-x86_64-10-latest.x86_64.qcow2 |
There was a problem hiding this comment.
Similar to the CentOS Stream 9 image, the URL for CentOS Stream 10 is incorrect, pointing to cloud.debian.org. Even if CentOS Stream 10 images are not yet available, the URL should be configured to point to the correct future location on cloud.centos.org.
url: https://cloud.centos.org/centos/10-stream/x86_64/images/CentOS-Stream-GenericCloud-10-latest.x86_64.qcow2There was a problem hiding this comment.
Same as above — fixed in 551b4e6. Both CentOS Stream URLs now use cloud.centos.org.
3befd76 to
a3585b3
Compare
dosubot
Bot
added
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
hack/cdi_golden_image_create.sh (1)
27-30: Storage size differs from Helm chart defaults.This script hardcodes
storage: 5Giwhile the Helm chart (values.yaml) defaults to20Gifor all images. Consider aligning these values or making storage a script parameter to avoid confusion.💡 Proposed fix to add storage parameter
name="$1" url="$2" +storage="${3:-20Gi}" if [ -z "$name" ] || [ -z "$url" ]; then - echo "Usage: <name> <url>" - echo "Example: 'ubuntu' 'https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img'" + echo "Usage: <name> <url> [storage]" + echo "Example: 'ubuntu' 'https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img' '20Gi'" exit 1 fiAnd update the storage request:
resources: requests: - storage: 5Gi + storage: $storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/cdi_golden_image_create.sh` around lines 27 - 30, The script hardcodes "storage: 5Gi" which diverges from the Helm chart default of 20Gi; make storage configurable by introducing a STORAGE_SIZE variable (default "20Gi") in hack/cdi_golden_image_create.sh and replace the hardcoded "5Gi" in the YAML-generation section (the block that emits "storage:") with that variable so callers can override via env or CLI flag; ensure the default aligns with values.yaml and update any help/comments to reflect the new STORAGE_SIZE parameter.packages/system/vm-default-images/values.yaml (1)
42-68: Consider pinning image versions for reproducibility.Several image URLs use "latest" or "current" patterns (e.g.,
Rocky-8-GenericCloud-Base.latest.x86_64.qcow2), which means the actual image content can change without notice. This could lead to inconsistent cluster deployments and make debugging difficult.For production environments, consider documenting whether this is intentional behavior or if version-pinned URLs should be used. The Alpine entry (line 160) demonstrates the pinned approach with
nocloud_alpine-3.21.6-x86_64.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/vm-default-images/values.yaml` around lines 42 - 68, The image entries named rocky-8, rocky-9, and rocky-10 use "latest" URLs which can change unexpectedly; update these entries in values.yaml to use version-pinned image URLs or document intentional use of rolling URLs: replace the current Rocky-*-GenericCloud-Base.latest.x86_64.qcow2 URLs with explicit release filenames (or add a new pinned_url field) for each image name (rocky-8, rocky-9, rocky-10) and add a short comment/annotation indicating why pinning was chosen or why latest is acceptable for reproducibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/v1alpha1/vmdisk/types.go`:
- Around line 35-48: The DeepCopyInto implementation for Source in
zz_generated.deepcopy.go lacks copying logic for the new pointer field Disk
(type *SourceDisk), causing pointer aliasing; update Source.DeepCopyInto to
deep-copy Disk the same way as Http/Image/Upload (allocate new SourceDisk and
copy the value) or simply regenerate the autogenerated deepcopy file (run make
generate or the controller-gen invocation used in the project) so
zz_generated.deepcopy.go includes the Disk handling.
In `@internal/controller/dashboard/customformsoverride.go`:
- Around line 256-280: The nested type assertions for
specProps["source"].(map[string]any) -> sourceObj["properties"].(map[string]any)
-> imgName["properties"].(map[string]any) and similarly for diskName assume the
"properties" key is present and a map, which can panic; update the blocks that
set imgName["properties"] and diskName["properties"] to first fetch and check
the "properties" value (e.g., v, ok := imgProps["properties"]; props, ok :=
v.(map[string]any)) before indexing into it, and only assign the "name" subkey
when the type assertions succeed—apply the same defensive checks to both the
imgName and diskName branches (referencing specProps, sourceObj, imgProps,
imgName, diskName).
In `@packages/apps/vm-disk/templates/dv.yaml`:
- Around line 24-27: The template assigns a lookup result to $dv via lookup
"cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf
"vm-default-images-%s" .Values.source.image.name) but never uses it; either
remove that lookup line or add validation that checks $dv (e.g., test if $dv is
nil and fail with a clear required message) before rendering the pvc name;
locate the lookup usage in the template where $dv is declared and update it to
either delete the assignment or insert a conditional check referencing $dv and
.Values.source.image.name to enforce existence.
---
Nitpick comments:
In `@hack/cdi_golden_image_create.sh`:
- Around line 27-30: The script hardcodes "storage: 5Gi" which diverges from the
Helm chart default of 20Gi; make storage configurable by introducing a
STORAGE_SIZE variable (default "20Gi") in hack/cdi_golden_image_create.sh and
replace the hardcoded "5Gi" in the YAML-generation section (the block that emits
"storage:") with that variable so callers can override via env or CLI flag;
ensure the default aligns with values.yaml and update any help/comments to
reflect the new STORAGE_SIZE parameter.
In `@packages/system/vm-default-images/values.yaml`:
- Around line 42-68: The image entries named rocky-8, rocky-9, and rocky-10 use
"latest" URLs which can change unexpectedly; update these entries in values.yaml
to use version-pinned image URLs or document intentional use of rolling URLs:
replace the current Rocky-*-GenericCloud-Base.latest.x86_64.qcow2 URLs with
explicit release filenames (or add a new pinned_url field) for each image name
(rocky-8, rocky-9, rocky-10) and add a short comment/annotation indicating why
pinning was chosen or why latest is acceptable for reproducibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fba3cabb-41bf-4535-8cb2-45dc7664eb6f
📒 Files selected for processing (14)
api/apps/v1alpha1/vmdisk/types.gohack/cdi_golden_image_create.shinternal/controller/dashboard/customformsoverride.gointernal/controller/dashboard/customformsoverride_test.gopackages/apps/vm-disk/README.mdpackages/apps/vm-disk/templates/dv.yamlpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-disk/values.yamlpackages/core/platform/sources/vm-default-images.yamlpackages/system/vm-default-images/Chart.yamlpackages/system/vm-default-images/Makefilepackages/system/vm-default-images/templates/dv.yamlpackages/system/vm-default-images/values.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Two blockers and a few non-blocking follow-ups.
Blockers
1. Breaking rename of DataVolume prefix with no migration path
packages/apps/vm-disk/templates/dv.yaml renames the source DataVolume/PVC lookup from vm-image-<name> to vm-default-images-<name>. On clusters where golden images were previously provisioned via hack/cdi_golden_image_create.sh (which produces PVCs named vm-image-*), any existing vm-disk that references source.image.name will, after upgrade, attempt to clone from a PVC that does not exist. CDI will surface a missing-source error and running disks will fail to reconcile.
No migration script is included (the platform migrations in packages/core/platform/images/migrations/migrations/ top out at 37). Please either:
- add a migration that renames existing DataVolumes/PVCs in
cozy-publicfromvm-image-<name>tovm-default-images-<name>, or - keep backwards compatibility in
dv.yamlduring a transition (check new name first, fall back to old).
2. vm-default-images PackageSource is not referenced in any bundle
packages/core/platform/sources/vm-default-images.yaml defines PackageSource cozystack.vm-default-images, but none of the bundle templates (iaas.yaml, paas.yaml, system.yaml, naas.yaml) include it. As-is, the platform will never install the chart on fresh clusters, meaning the source.image dropdown in the dashboard will always be empty. The chart depends on kubevirt-cdi, so the natural place is iaas.yaml next to it:
{{ include "cozystack.platform.package.default" (list "cozystack.vm-default-images" $) }}Non-blocking
- Dead code:
{{- $dv := lookup ... }}inpackages/apps/vm-disk/templates/dv.yamlassigns a result that is never used. Pre-existing, but touched by this PR — worth removing. - No E2E test for
vm-default-images: no.batsfile added underhack/e2e-apps/for the new system package. - No E2E test for
source.diskclone path: dashboard unit tests cover the override logic, but there is no integration test verifying disk-to-disk cloning in a live cluster. hack/cdi_golden_image_create.sh: hardcodesstorageClassName: replicated. With the new Helm chart (which correctly defaults to the cluster default when empty), this script's role and divergent behavior should either be updated or the script deprecated.
Overall the dashboard override refactor (buildMultilineStringSchema for no-spec-wrapper schemas) is clean and well-tested; the main concerns are the upgrade path and bundle wiring.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous blockers — the migration to rename DataVolumes and wiring vm-default-images into the iaas bundle both look good. However, one remaining blocker with migration sequencing, plus a couple of non-blocking observations.
Observation (non-blocking): no e2e coverage for the new source types.
Existing e2e in hack/e2e-apps/vminstance.bats only exercises source.http. The new source.disk (same-namespace PVC clone) and source.image (cross-namespace clone from cozy-public) paths are not covered. Worth adding bats cases given the dashboard now exposes both as dropdowns.
| @@ -0,0 +1,52 @@ | |||
| #!/bin/sh | |||
| # Migration 38 --> 39 | |||
There was a problem hiding this comment.
Blocker: migration 38 will never execute on merged state.
main bumped migrations.targetVersion to 38 in commit af3cccd ("fix(platform): update migration targetVersion to 38") so that migration 37 runs. This PR adds migration file 38 but does not bump targetVersion in packages/core/platform/values.yaml. After squash-merge to current main:
values.yamlwill still havetargetVersion: 38- migration file
38will be present
The runner (run-migrations.sh) does seq $CURRENT_VERSION $((TARGET_VERSION - 1)) and exits early when CURRENT_VERSION >= TARGET_VERSION. With targetVersion: 38, migration 38 is never picked up, so the vm-image-* → vm-default-images-* rename will not happen on upgrade — reintroducing the original breakage this PR was supposed to fix.
Fix: rebase on main and bump packages/core/platform/values.yaml → migrations.targetVersion to 39 (this migration stamps version=39 on completion, so targetVersion must match).
There was a problem hiding this comment.
Good catch. Rebased on main and bumped targetVersion to 39 in 0b5a1df. Migration 38 will now be picked up by run-migrations.sh correctly.
| | jq --arg new "$NEWNAME" ' | ||
| del(.metadata.resourceVersion, .metadata.uid, .metadata.creationTimestamp, | ||
| .metadata.generation, .metadata.selfLink, .metadata.managedFields, | ||
| .status) |
There was a problem hiding this comment.
Observation (non-blocking): this migration re-downloads every golden image instead of renaming the underlying PVC. It copies the old DataVolume's spec (including source.http.url) into a new DV with a different name, which CDI treats as a fresh import. For 16 default images at ~20 GiB each, that's up to ~320 GiB of cluster bandwidth per upgrade, plus a window between old DV deletion and new DV reaching Succeeded during which source.image lookups in vm-disk fail.
Existing VMDisks that already cloned from vm-image-* PVCs are unaffected; the end state is correct. Consider using CDI's PVC clone source (source.pvc) to do a true in-place rename and avoid the re-download.
There was a problem hiding this comment.
Acknowledged — using source.pvc for in-place rename instead of re-downloading would be significantly more efficient. Will address in a follow-up to keep this PR focused on correctness.
Signed-off-by: Myasnikov Daniil <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
…fault-images-* The vm-disk package previously referenced golden-image DataVolumes with the prefix "vm-image-<name>" in the cozy-public namespace. The new vm-default-images package creates them as "vm-default-images-<name>", so any existing vm-disk that sources an image by name would fail to find its PVC after upgrade. Migration 38 renames every DataVolume in cozy-public whose name starts with "vm-image-" to "vm-default-images-" (dropping the old object and re-creating with the new name) so live vm-disk resources keep resolving their source PVC transparently. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The PackageSource for vm-default-images existed but was never included in any bundle, so it would never be installed. Add it to iaas.yaml immediately after kubevirt-cdi (which it depends on) so it is deployed together with the rest of the KubeVirt IaaS stack. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The \$dv variable assigned by the lookup call on the image PVC was never read. Remove the dead line to keep the template tidy. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The x86_64 architecture token was placed inside the filename segment (CentOS-Stream-GenericCloud-x86_64-N-latest.x86_64.qcow2) which does not match the actual CentOS mirror layout. Correct both CentOS Stream 9 and 10 URLs to the canonical form without the extra architecture infix. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Accessing imgName["properties"].(map[string]any) and diskName["properties"].(map[string]any) without an ok-check would panic if the schema does not contain a "properties" key. Wrap both assertions in ok-guarded form consistent with the outer defensive style. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Running make generate at repo root regenerates the zz_generated.deepcopy.go files. Key fix: Source.DeepCopyInto in the vmdisk package now handles the new Disk *SourceDisk pointer field, which was previously missing and would cause pointer aliasing on deep-copy. Other generated changes are incidental updates to vminstance (Networks field added, Subnet renamed to Network) and backups CRD manifests picked up by controller-gen from the current HEAD. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…torage requirements The package is wired into the iaas bundle where LINSTOR replicated storage is available. Set storageClass to "replicated" by default and add a note that the full image set needs ~320Gi (16 images × 20Gi). Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Address review feedback from lexfrei on migrations/38:2: Migration 38 requires targetVersion 39 to execute, since run-migrations.sh iterates seq $CURRENT_VERSION $((TARGET_VERSION - 1)). Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
9e5b7d2 to
0b5a1df
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/vm-default-images/templates/dv.yaml (1)
36-44: Consider quoting the storage value.The
storagevalue on line 39 is not quoted. While Kubernetes typically handles quantity strings well, if.storagecontains a value that YAML interprets differently (e.g., scientific notation edge cases), this could cause unexpected behavior. Consider quoting for consistency with other Helm charts.♻️ Suggested fix
storage: resources: requests: - storage: {{ default "5Gi" .storage }} + storage: {{ default "5Gi" .storage | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/vm-default-images/templates/dv.yaml` around lines 36 - 44, The resources.requests.storage template should emit a YAML string, so change the template that sets storage (the expression using the default function and the .storage value) to render as a quoted string; specifically wrap the rendered value from the default("5Gi", .storage) expression in YAML quotes so resources.requests.storage becomes a string literal rather than an unquoted scalar, and leave the storageClassName branches (using .storageClass and $.Values.storageClass) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/system/vm-default-images/templates/dv.yaml`:
- Around line 36-44: The resources.requests.storage template should emit a YAML
string, so change the template that sets storage (the expression using the
default function and the .storage value) to render as a quoted string;
specifically wrap the rendered value from the default("5Gi", .storage)
expression in YAML quotes so resources.requests.storage becomes a string literal
rather than an unquoted scalar, and leave the storageClassName branches (using
.storageClass and $.Values.storageClass) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0acd5420-b296-42b3-846a-e72cae3ebbfc
📒 Files selected for processing (22)
api/apps/v1alpha1/vmdisk/types.goapi/apps/v1alpha1/vmdisk/zz_generated.deepcopy.goapi/apps/v1alpha1/vminstance/zz_generated.deepcopy.goapi/apps/v1alpha1/vpc/zz_generated.deepcopy.goapi/backups/v1alpha1/zz_generated.deepcopy.gohack/cdi_golden_image_create.shinternal/controller/dashboard/customformsoverride.gointernal/controller/dashboard/customformsoverride_test.gopackages/apps/vm-disk/README.mdpackages/apps/vm-disk/templates/dv.yamlpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-disk/values.yamlpackages/core/platform/images/migrations/migrations/38packages/core/platform/sources/vm-default-images.yamlpackages/core/platform/templates/bundles/iaas.yamlpackages/core/platform/values.yamlpackages/system/backup-controller/definitions/backups.cozystack.io_backups.yamlpackages/system/vm-default-images/Chart.yamlpackages/system/vm-default-images/Makefilepackages/system/vm-default-images/templates/dv.yamlpackages/system/vm-default-images/values.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yaml
✅ Files skipped from review due to trivial changes (9)
- packages/core/platform/templates/bundles/iaas.yaml
- packages/core/platform/values.yaml
- api/backups/v1alpha1/zz_generated.deepcopy.go
- packages/system/vm-default-images/Chart.yaml
- hack/cdi_golden_image_create.sh
- packages/apps/vm-disk/values.schema.json
- packages/apps/vm-disk/README.md
- packages/core/platform/sources/vm-default-images.yaml
- packages/system/vm-default-images/values.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/apps/vm-disk/templates/dv.yaml
- packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml
- internal/controller/dashboard/customformsoverride_test.go
- api/apps/v1alpha1/vpc/zz_generated.deepcopy.go
- packages/apps/vm-disk/values.yaml
- api/apps/v1alpha1/vmdisk/zz_generated.deepcopy.go
Follow-up to PR #2258: adds two new @test cases in hack/e2e-apps/vm-disk.bats covering the new vm-disk clone paths — source.image (golden image from cozy-public) and source.disk (PVC clone within tenant namespace). Existing source.http coverage in vminstance.bats is not touched. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. The core functionality is sound: the new vm-default-images chart correctly generates annotated DataVolumes, the source.disk clone path correctly references PVCs by the vm-disk- prefix convention, the dashboard override logic for both image and disk dropdowns is well-tested, and the bundle wiring in iaas.yaml correctly places the package after kubevirt-cdi.
Non-blocking action items for follow-up
A1: Migration 38 causes full re-import of golden images on upgrade
The migration creates a new DataVolume then deletes the old one. CDI cascade-deletes the PVC, triggering a fresh import from the original URL. Could be optimized with kubectl delete --cascade=orphan to preserve PVCs and avoid re-downloading all images during upgrade.
A2: Migrated DataVolumes lack dashboard annotations
DataVolumes renamed by migration 38 don't receive the vm-default-images.cozystack.io/name and vm-default-images.cozystack.io/description annotations. Migrated images won't appear in the dashboard image selector dropdown.
A3: Unrelated generated code changes in the diff
Deepcopy regeneration for vminstance, vpc, backups, and backup-controller CRD field reordering — artifacts from make generate that add noise. Ideally separate from the feature PR.
A4: hack/cdi_golden_image_create.sh diverges from new chart defaults
Still hardcodes storageClassName: replicated and 5Gi storage, while the new chart defaults to 20Gi. Consider deprecating or aligning.
The package imports ~320Gi of golden-image PVCs (ubuntu-noble, fedora,
debian, centos, etc.) as soon as it's installed. On small test and dev
clusters that's enough to consume the entire replicated storage pool,
after which no tenant PVCs — including the ones E2E itself provisions —
can be bound. It's also unreasonable to force that cost on every iaas
user: many deployments don't need prebuilt images at all, and the ones
that do often want to curate their own subset.
Switch the bundle entry from 'package.default' to 'package.optional.default',
matching the treatment already applied to gpu-operator directly below it.
Users who want the golden images can opt in via:
bundles:
enabledPackages:
- cozystack.vm-default-images
The package, its Source, and migration 38 all stay in place — nothing
else changes for users who explicitly enable it. Users who previously
relied on the bundle auto-installing it will need to add the package
to enabledPackages on upgrade; this is called out in the release note.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
|
Aleksei Sviridkin (@lexfrei) reversing the "add it to the iaas bundle" part of your earlier review — want to flag it explicitly. While testing this PR I hit a case where the package's golden images consumed the cluster's replicated pool on a fresh install, and every subsequent PVC (including E2E's own) failed to bind. More generally, forcing that much storage on every iaas user isn't a good default: many deployments don't need prebuilt images, and those that do usually want to curate their own subset. Pushed bundles:
enabledPackages:
- cozystack.vm-default-imagesPackage, Source, and migration 38 stay as-is — nothing changes for users who enable it. Accepted the bundle inclusion originally; on reflection "install by default" was the wrong call. Happy to discuss if you disagree. |
Follow-up to PR #2258: adds two new @test cases in hack/e2e-apps/vm-disk.bats covering the new vm-disk clone paths — source.image (golden image from cozy-public) and source.disk (PVC clone within tenant namespace). Existing source.http coverage in vminstance.bats is not touched. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…#2406) ## What this PR does Stacks on top of #2258. Adds `dependsOn: ingress-nginx-system` to the inner `seaweedfs-system` HelmRelease so flux blocks its install until the ingress chart is Ready. ## Why In #2258's E2E run (and the `cozyreport-2258` bundle from this morning), `seaweedfs-system` failed to install with: ``` Helm install failed for release tenant-root/seaweedfs-system: 1 error occurred: * Internal error occurred: failed calling webhook "validate.nginx.ingress.kubernetes.io": failed to call webhook: Post "https://root-ingress-controller-admission.tenant-root.svc:443/networking/v1/ingresses?timeout=10s": dial tcp 10.96.33.42:443: connect: operation not permitted ``` ### Root cause class The outer tenant HelmReleases (`apps/tenant/templates/*.yaml`) render one inner HR per subsystem and let flux reconcile them. Both `seaweedfs` and `ingress` outer HRs get applied in parallel, and each of them spawns its own inner HR — which flux then installs in parallel too. When `seaweedfs-system` gets to the `creating 39 resource(s)` step, it tries to create the tenant Ingress. The apiserver calls the ingress-nginx `ValidatingWebhookConfiguration` (which uses `failurePolicy: Fail` by default). If the nginx controller pod/Service/endpoint state hasn't finished reconciling on the CNI yet, the connect to the ClusterIP fails with `EPERM`, the webhook errors, Helm aborts, and flux rolls back. E2E recovers via the `flux reconcile --force` retry loop in `hack/e2e-install-cozystack.bats`, but it's flaky and adds minutes. ### Fix Introduce an explicit ordering dependency at the inner HR layer: ```yaml {{- if $ingress }} dependsOn: - name: ingress-nginx-system {{- end }} ``` Flux waits for `ingress-nginx-system` to reach `Ready=true` before installing seaweedfs — which, under Flux's wait semantics, means the ingress-nginx Deployment is Available and its webhook endpoint is routable. The race disappears. Guarded by `{{- if $ingress }}` so this only kicks in for tenants that actually have ingress enabled. ## Test plan - [ ] Rerun E2E on #2258 — `seaweedfs-system` should install on the first try without a `Retry install` iteration. - [ ] Verify rendered template still matches expectations when `$ingress` is unset (no `dependsOn` block). ### Release note ```release-note [seaweedfs] Make tenant seaweedfs wait for ingress-nginx to be Ready before creating its Ingress resource, avoiding a webhook race that intermittently failed install on fresh clusters. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved deployment sequencing so the system now waits for the Ingress controller to be ready before creating or updating resources that include Ingress. This prevents premature deployments and related failures, reduces rollout errors, and provides more reliable, predictable deployments for applications relying on Ingress resources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
What this PR does
Release note
Summary by CodeRabbit
New Features
UI
Migration
Documentation