Testing/add vpc #1558
Testing/add vpc #1558
Conversation
Signed-off-by: nbykov0 <166552198+nbykov0@users.noreply.github.com>
Signed-off-by: nbykov0 <166552198+nbykov0@users.noreply.github.com>
Signed-off-by: nbykov0 <166552198+nbykov0@users.noreply.github.com>
Signed-off-by: nbykov0 <166552198+nbykov0@users.noreply.github.com>
dosubot
bot
added
the
size:L
|
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. WalkthroughAdds multi‑subnet VPC support: new VPC chart and resources, VM / VMInstance values and templates to attach subnets and generate per‑subnet Multus interfaces, a conditional network-config Secret for ubuntu/alpine images, RBAC permission for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm as Helm Chart(s)
participant VPC as VPC Templates
participant VM as VM Templates
participant Secret as Network Secret
participant CloudInit as Cloud-Init
User->>VPC: Deploy VPC chart with subnets
VPC->>VPC: Compute vpcId/subnetId (sha256 -> truncate)
VPC->>K8s: Create NetworkAttachmentDefinitions
VPC->>K8s: Create Subnet resources & ConfigMap(s)
User->>Helm: Deploy VM/VMInstance with subnets
Helm->>VM: Render VM spec with per‑subnet interfaces & Multus entries
alt image is ubuntu|alpine and subnets present
Helm->>Secret: Render network-config Secret (per‑subnet interfaces)
VM->>CloudInit: Reference networkDataSecretRef in cloud-init
CloudInit->>CloudInit: Apply DHCP network config for interfaces
else
VM->>CloudInit: Use existing cloudInit/sshKeys flow (no network secret)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 @lllamnyp, 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 lays the groundwork for Virtual Private Cloud (VPC) functionality, enabling the creation and management of isolated networks within the platform. It integrates subnet attachment capabilities into virtual machine deployments and introduces a dedicated VPC application, providing a more robust and configurable networking environment for users. 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-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
|
![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 vpc application and integrates it with virtual-machine and vm-instance to allow attaching VMs to subnets. The changes are comprehensive, including new Helm charts, Kubernetes resource definitions, and updates to existing VM templates. The implementation is mostly solid, but I've identified a few opportunities to improve maintainability and readability by reducing code duplication in Helm templates. I've also noted a minor formatting issue in a JSON schema file. My review comments provide specific suggestions to address these points.
| @@ -0,0 +1,40 @@ | |||
| # if subnets are specified and image is either ubunu or alpine | |||
There was a problem hiding this comment.
There's a typo in the comment. It should be ubuntu instead of ubunu.
# if subnets are specified and image is either ubuntu or alpine| bus: scsi | ||
| name: systemdisk | ||
| {{- if or .Values.cloudInit .Values.sshKeys }} | ||
| {{- if or .Values.cloudInit .Values.sshKeys (and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine"))) }} |
There was a problem hiding this comment.
The condition (and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine"))) is used multiple times in this file (e.g., here and on lines 132-143). To improve readability and maintainability, consider defining it as a variable at the top of the file and reusing it.
For example:
{{- $needsNetconfig := and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")) -}}
...
{{- if or .Values.cloudInit .Values.sshKeys $needsNetconfig }}
...
{{- if $needsNetconfig }}
networkDataSecretRef:
name: {{ include "virtual-machine.fullname" . }}-network-config
{{- end }}| subnets: | | ||
| {{- range $subnetName, $subnetConfig := .Values.subnets }} | ||
| - subnetName: {{ $subnetName }} | ||
| subnetId: {{ print "subnet-" (print $.Release.Namespace "/" $vpcId "/" $subnetName | sha256sum | trunc 8) }} |
There was a problem hiding this comment.
The logic to generate the subnetId is duplicated from line 20. This could lead to inconsistencies if one is updated and the other is missed. To avoid this, you could define a helper template for generating the subnet ID and use it in both places.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file is missing a newline character at the end. While many tools handle this gracefully, some might have issues. It's a good practice to end files with a newline.
| } | |
| } | |
![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 (1)
packages/apps/vpc/templates/vpc.yaml (1)
1-72: Verify Helm template syntax and add defensive checks for subnet iteration.The YAMLlint reports a syntax error at line 20, though this may be a false positive due to Go template syntax. Please verify:
- The
truncfunction is properly defined as a Helm template function accepting an integer argument.- The template functions
sha256sumandtruncare available in the Helm chart's template context.Additionally, consider adding a defensive check before the range loop to handle cases where
.Values.subnetsis undefined or empty:+{{- if .Values.subnets }} {{- range $subnetName, $subnetConfig := .Values.subnets }}This prevents unintended behavior if subnets aren't defined and improves robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/apps/vpc/logos/vpc.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
packages/apps/tenant/templates/tenant.yaml(1 hunks)packages/apps/virtual-machine/README.md(1 hunks)packages/apps/virtual-machine/templates/secret-network-config.yaml(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(4 hunks)packages/apps/virtual-machine/values.schema.json(1 hunks)packages/apps/virtual-machine/values.yaml(2 hunks)packages/apps/vm-instance/README.md(1 hunks)packages/apps/vm-instance/templates/vm.yaml(2 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(2 hunks)packages/apps/vpc/Chart.yaml(1 hunks)packages/apps/vpc/Makefile(1 hunks)packages/apps/vpc/templates/vpc.yaml(1 hunks)packages/apps/vpc/values.schema.json(1 hunks)packages/apps/vpc/values.yaml(1 hunks)packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml(2 hunks)packages/system/cozystack-resource-definitions/cozyrds/virtualprivatecloud.yaml(1 hunks)packages/system/cozystack-resource-definitions/cozyrds/vm-instance.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T04:52:22.093Z
Learnt from: lllamnyp
PR: cozystack/cozystack#1515
File: packages/system/lineage-controller-webhook/templates/daemonset.yaml:29-34
Timestamp: 2025-10-14T04:52:22.093Z
Learning: In Kubernetes YAML manifests and Helm templates, list items can be formatted with the leading `- ` aligned at the same level as the parent key (e.g., `args:\n- item1\n- item2`) because Kubernetes uses the YAML v2 library which considers the leading `- ` to be part of the indentation. This is the standard style for Kubernetes resources.
Applied to files:
packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml
🪛 checkmake (0.2.2)
packages/apps/vpc/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: Pre-Commit Checks
packages/apps/vpc/values.schema.json
[error] 1-1: Chart Values schema regression: the 'subnets' property appears removed (now null) in openAPISchema.
🪛 YAMLlint (1.37.1)
packages/apps/virtual-machine/templates/secret-network-config.yaml
[error] 2-2: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/vpc/templates/vpc.yaml
[error] 20-20: syntax error: could not find expected ':'
(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 (18)
packages/apps/vpc/values.yaml (1)
1-13: Well-documented subnet configuration.The values file is properly structured with clear documentation and a practical example. The empty default map correctly makes subnets optional.
packages/apps/vm-instance/README.md (1)
52-53: Clear and helpful subnet parameter documentation.The addition succinctly explains the purpose and behavior of the subnets parameter, including the note that the management subnet is attached by default. Documentation is consistent with existing parameter style.
packages/apps/virtual-machine/README.md (1)
39-61: Documentation consistent across VM components.The subnet parameter documentation mirrors the vm-instance README appropriately, maintaining consistency across the codebase. Table formatting is clean and aligned.
packages/apps/vm-instance/templates/vm.yaml (1)
80-120: Subnet networking integration is well-implemented.The template correctly:
- Guards both interface and network additions with conditional checks
- Maintains consistency between interface definitions (lines 80–85) and network definitions (lines 114–120)
- Uses proper Helm templating and variable scoping with
$subnet.name- Preserves the default pod network alongside additional Multus networks
packages/apps/vpc/Chart.yaml (1)
1-6: Chart structure is correct; icon asset confirmed.The Helm chart manifest is well-structured with proper metadata, and the referenced icon asset
logos/vpc.svgexists in the repository. No issues found.packages/apps/vpc/values.schema.json (1)
1-24: ****The original review comment incorrectly claims that the 'subnets' property "appears removed (now null) in openAPISchema." However, verification shows:
- The
subnetsproperty is fully defined in the VirtualPrivateCloud CRD (packages/system/cozystack-resource-definitions/cozyrds/virtualprivatecloud.yaml)- The CRD's openAPISchema matches exactly the schema in
values.schema.json- The property is included in the keysOrder declaration
- The schema syntax is valid
The schema is properly integrated and there is no evidence of a regression or pipeline failure related to this change.
Likely an incorrect or invalid review comment.
packages/apps/vpc/Makefile (1)
1-8: No changes needed in vpc/Makefile.The phony targets are correctly provided by the included
../../../scripts/package.mk, which declares them and provides the corresponding target implementations. This Makefile properly follows the project's package structure pattern, consistent with all other app-level Makefiles (bucket, clickhouse, ferretdb, etc.).packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml (1)
31-31: Verify keysOrder consistency across VM-related CRDs.The subnets field is positioned after systemDisk and before gpus in this CRD's keysOrder. Please verify this positioning is consistent across related VM resources (e.g., vm-instance.yaml).
Also applies to: 35-39
packages/apps/vm-instance/values.schema.json (1)
191-207: Schema definition for subnets field is well‑structured.The subnets property is correctly defined as an array of objects with a required name field, consistent with the values.yaml typedef and CRD definitions.
packages/apps/vm-instance/values.yaml (1)
21-22: Subnet typedef and parameter documentation is clear and well‑formatted.The new Subnet typedef and subnets parameter follow established documentation patterns with helpful examples. The positioning before resources aligns with logical parameter grouping.
Also applies to: 57-63
packages/apps/tenant/templates/tenant.yaml (1)
273-273: RBAC update appropriately grants VPC management permissions.The virtualprivateclouds resource is correctly added to the admin role with full CRU+D permissions. The exclusion from view and use roles is appropriate for this resource.
packages/apps/virtual-machine/values.schema.json (1)
170-186: Schema for virtual-machine subnets field is consistent and well‑defined.The subnets property correctly mirrors the vm-instance schema implementation and validates the required name field.
packages/apps/virtual-machine/values.yaml (1)
29-30: Virtual-machine subnets documentation and examples are well‑structured.The Subnet typedef and subnets parameter follow established documentation patterns with clear examples consistent with vm-instance implementation.
Also applies to: 56-62
packages/apps/vpc/templates/vpc.yaml (1)
4-4: ID generation strategy is sound but relies on undocumented hash functions.The vpcId and subnetId calculations use sha256sum and trunc functions to generate deterministic, collision-resistant identifiers. This is a good approach for:
- Consistent resource naming across Helm releases
- Short, URL-safe identifiers (6-8 chars)
- Predictable naming without sequential counters
However, ensure these template functions are documented or have fallback implementations if they're custom-defined.
Also applies to: 20-20
packages/system/cozystack-resource-definitions/cozyrds/virtualprivatecloud.yaml (1)
1-34: Resource definition structure looks good.The CozystackResourceDefinition follows expected patterns with proper OpenAPI schema for subnets, dashboard metadata, and release configuration. The inline JSON schema correctly enforces the required
cidrfield with CIDR format validation for each subnet.packages/apps/virtual-machine/templates/vm.yaml (3)
96-96: Ensure subnet condition is consistently applied.The cloudInit disk inclusion condition (lines 96 and 132) and networkDataSecretRef inclusion condition (line 139) use related but slightly different logic. Verify this is intentional:
- Lines 96 & 132:
or .Values.cloudInit .Values.sshKeys (and .Values.subnets (or ...))- Line 139:
and .Values.subnets (or ...)The narrower condition on line 139 appears correct (network config only needed when subnets exist), but confirm this distinction is intentional and doesn't cause the volume to be created without the secret reference.
Also applies to: 132-132, 139-139
105-110: The subnet name field is already required in the schema.The verification confirms that
nameis explicitly listed in therequiredarray for each subnet object invalues.schema.json. The template code at lines 105-110 is correctly protected by this schema validation, ensuring all subnets have a name field before being rendered.
132-142: I can see the secret is created with the correct name. Now I need to verify if thenetworkDataSecretRefreference in vm.yaml requires akeyfield to properly reference thenetworkDatafield in the secret.I can see from the KubeVirt documentation that the secret should be created with a
networkdatakey using--from-file=networkdata=network-config. However, thesecret-network-config.yamlfile usesnetworkData(camelCase) instead. Let me verify if this is an issue by checking the KubeVirt source code more carefully.Based on my verification, I found a potential issue. KubeVirt's documented examples for networkDataSecretRef use the secret key
networkdata(lowercase), but thesecret-network-config.yamlfile usesnetworkData(camelCase). While KubeVirt's cloud-init code attempts to read from both "networkdata" and "networkData" keys as a fallback, this inconsistency with documentation and a reported issue where networkDataSecretRef did not apply the network config (though networkDataBase64 worked) suggests this may not be entirely reliable.Additionally, I notice the secret name is created with
{{ include "virtual-machine.fullname" $ }}while the reference uses{{ include "virtual-machine.fullname" . }}— the inconsistent context variables ($vs.) should produce the same result, but it's worth verifying they're equivalent in this Helm template context.Standardize the secret key name and verify context variable consistency.
Change the secret key in
secret-network-config.yamlfromnetworkDatatonetworkdata(lowercase) to align with KubeVirt's documented expectations and official examples. Also verify that{{ include "virtual-machine.fullname" $ }}and{{ include "virtual-machine.fullname" . }}produce identical values in your Helm templates to ensure the references match correctly.
| # if subnets are specified and image is either ubunu or alpine | ||
| {{- if and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")) }} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ include "virtual-machine.fullname" $ }}-network-config | ||
| stringData: | ||
| networkData: | | ||
| network: | ||
| version: 1 | ||
| config: | ||
| {{- if eq .Values.systemDisk.image "ubuntu" }} | ||
| # main iface | ||
| - type: physical | ||
| name: enp1s0 | ||
| subnets: | ||
| - type: dhcp | ||
| # additional ifaces attached to subnets | ||
| {{- range $i, $subnet := .Values.subnets }} | ||
| - type: physical | ||
| name: enp{{ add 2 $i }}s0 | ||
| subnets: | ||
| - type: dhcp | ||
| {{- end }} | ||
| {{- else if eq .Values.systemDisk.image "alpine" }} | ||
| #main iface | ||
| - type: physical | ||
| name: eth0 | ||
| subnets: | ||
| - type: dhcp | ||
| # additional ifaces attached to subnets | ||
| {{- range $i, $subnet := .Values.subnets }} | ||
| - type: physical | ||
| name: eth{{ add1 $i }} | ||
| subnets: | ||
| - type: dhcp | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
CRITICAL: Multiple syntax errors blocking template execution.
The template has three critical issues:
- Line 2 – YAML syntax error: The condition line appears malformed, causing YAMLlint to fail. The multiline condition needs proper formatting.
- Line 2 – Typo:
"ubunu"should be"ubuntu"— this would cause Ubuntu image detection to fail. - Line 35 – Invalid Helm function:
add1should beadd 1(with a space) to properly invoke the arithmetic function.
Apply this diff to fix all three issues:
-# if subnets are specified and image is either ubunu or alpine
-{{- if and .Values.subnets (or (eq .Values.systemDisk.image "ubunu") (eq .Values.systemDisk.image "alpine")) }}
+# if subnets are specified and image is either ubuntu or alpine
+{{- if and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")) }}- name: eth{{ add1 $i }}
+ name: eth{{ add 1 $i }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # if subnets are specified and image is either ubunu or alpine | |
| {{- if and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")) }} | |
| --- | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: {{ include "virtual-machine.fullname" $ }}-network-config | |
| stringData: | |
| networkData: | | |
| network: | |
| version: 1 | |
| config: | |
| {{- if eq .Values.systemDisk.image "ubuntu" }} | |
| # main iface | |
| - type: physical | |
| name: enp1s0 | |
| subnets: | |
| - type: dhcp | |
| # additional ifaces attached to subnets | |
| {{- range $i, $subnet := .Values.subnets }} | |
| - type: physical | |
| name: enp{{ add 2 $i }}s0 | |
| subnets: | |
| - type: dhcp | |
| {{- end }} | |
| {{- else if eq .Values.systemDisk.image "alpine" }} | |
| #main iface | |
| - type: physical | |
| name: eth0 | |
| subnets: | |
| - type: dhcp | |
| # additional ifaces attached to subnets | |
| {{- range $i, $subnet := .Values.subnets }} | |
| - type: physical | |
| name: eth{{ add1 $i }} | |
| subnets: | |
| - type: dhcp | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} | |
| name: eth{{ add 1 $i }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In packages/apps/virtual-machine/templates/secret-network-config.yaml lines
1-40: the Helm conditional on line 2 is malformed and must be rewritten with
correct parentheses and spacing, the image name "ubunu" should be corrected to
"ubuntu", and the invalid Helm function `add1` on line 35 must be replaced with
the proper `add 1` call; update the opening if to a valid form (e.g. {{- if and
.Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq
.Values.systemDisk.image "alpine")) }}), fix the typo "ubunu" → "ubuntu"
wherever present, and replace `add1 $i` with `add 1 $i` so the template renders
correctly.
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | ||
| {{- else if eq .Values.systemDisk.image "fedora" }} | ||
| url: https://download.fedoraproject.org/pub/fedora/linux/releases/40/Cloud/x86_64/images/Fedora-Cloud-Base-Generic.x86_64-40-1.14.qcow2 | ||
| url: https://download.fedoraproject.org/pub/fedora/linux/releases/42/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-42-1.1.x86_64.qcow2 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify updated image URLs are accessible and compatible.
The Fedora image has been updated to version 42 (line 54) and Alpine to 3.20.8 (line 56). Confirm that these URLs remain valid and that the new image versions maintain compatibility with cloud-init networking configurations, particularly when subnets are configured.
Also applies to: 56-56
🌐 Web query:
What are the latest stable cloud image URLs for Fedora 42 and Alpine 3.20.8?
💡 Result:
Here are the current stable cloud image URLs I found (checked Oct 30, 2025):
-
Fedora 42 (Generic QEMU qcow2, Fedora Cloud Base 42 — Fedora-Cloud-Base-Generic-42-1.1.x86_64.qcow2):
https://download2.fedora.redhat.com/pub/fedora/linux/releases/42/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-42-1.1.x86_64.qcow2. [1] -
Alpine 3.20.8 — note: Alpine v3.20.8 is available as a package update (packages indexed for v3.20 show 3.20.8), but there is no released alpine-virt image labeled 3.20.8. The latest alpine-virt image in the v3.20 release tree is 3.20.6:
https://dl-cdn.alpinelinux.org/alpine/v3.20/releases/x86_64/alpine-virt-3.20.6-x86_64.iso. [2][3]
Sources: Fedora Cloud images index (Fedora 42 images). [1] Alpine v3.20 releases index (virt images) and alpine-release package showing 3.20.8 existence. [2][3]
Update image URLs and verify Alpine version availability.
The web search reveals URL and version mismatches:
• Fedora 42: The current stable URL uses download2.fedora.redhat.com domain, not download.fedoraproject.org as shown on line 54. Update to https://download2.fedora.redhat.com/pub/fedora/linux/releases/42/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-42-1.1.x86_64.qcow2.
• Alpine 3.20.8: While Alpine v3.20.8 exists as a package update, no released alpine-virt cloud image labeled 3.20.8 is available; the latest alpine-virt in the v3.20 release tree is 3.20.6. Verify line 56 references a valid released image or revert to 3.20.6.
🤖 Prompt for AI Agents
In packages/apps/virtual-machine/templates/vm.yaml around line 54, update the
Fedora cloud image URL to use the download2.fedora.redhat.com domain
(https://download2.fedora.redhat.com/pub/fedora/linux/releases/42/Cloud/x86_64/images/Fedora-Cloud-Base-Generic-42-1.1.x86_64.qcow2)
and on the Alpine image reference (around line 56) verify the exact released
alpine-virt image exists; if 3.20.8 is not available, change it to the latest
released alpine-virt in the 3.20 tree (3.20.6) or replace with the correct
released version URL, ensuring both URLs point to reachable released cloud
images.
| - compute | ||
| icon: PHN2ZyB3aWR0aD0iMTQ0IiBoZWlnaHQ9IjE0NCIgdmlld0JveD0iMCAwIDE0NCAxNDQiIGZpbGw9Im5vbmUiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+CjxyZWN0IHdpZHRoPSIxNDQiIGhlaWdodD0iMTQ0IiByeD0iMjQiIGZpbGw9InVybCgjcGFpbnQwX2xpbmVhcl82ODdfMzQ1NCkiLz4KPGcgY2xpcC1wYXRoPSJ1cmwoI2NsaXAwXzY4N18zNDU0KSI+CjxwYXRoIGQ9Ik04OS41MDM5IDExMS43MDdINTQuNDk3QzU0LjE3MjcgMTExLjcwNyA1NC4wMTA4IDExMS4yMjEgNTQuMzM0OSAxMTEuMDU5TDU3LjI1MjIgMTA4Ljk1MkM2MC4zMzE0IDEwNi42ODMgNjEuOTUyMiAxMDIuNjMxIDYwLjk3OTcgOTguNzQxMkg4My4wMjFDODIuMDQ4NSAxMDIuNjMxIDgzLjY2OTMgMTA2LjY4MyA4Ni43NDg1IDEwOC45NTJMODkuNjY1OCAxMTEuMDU5Qzg5Ljk5IDExMS4yMjEgODkuODI3OSAxMTEuNzA3IDg5LjUwMzkgMTExLjcwN1oiIGZpbGw9IiNCMEI2QkIiLz4KPHBhdGggZD0iTTExMy4zMjggOTguNzQxSDMwLjY3MjVDMjcuNTkzMSA5OC43NDEgMjUgOTYuMTQ4IDI1IDkzLjA2ODdWMzMuMTAzMkMyNSAzMC4wMjM5IDI3LjU5MzEgMjcuNDMwNyAzMC42NzI1IDI3LjQzMDdIMTEzLjMyOEMxMTYuNDA3IDI3LjQzMDcgMTE5IDMwLjAyMzcgMTE5IDMzLjEwMzJWOTMuMDY4N0MxMTkgOTYuMTQ4IDExNi40MDcgOTguNzQxIDExMy4zMjggOTguNzQxWiIgZmlsbD0iI0U4RURFRSIvPgo8cGF0aCBkPSJNMTE5IDg0LjE1NDlIMjVWMzMuMTAzMkMyNSAzMC4wMjM5IDI3LjU5MzEgMjcuNDMwNyAzMC42NzI1IDI3LjQzMDdIMTEzLjMyOEMxMTYuNDA3IDI3LjQzMDcgMTE5IDMwLjAyMzcgMTE5IDMzLjEwMzJMMTE5IDg0LjE1NDlaIiBmaWxsPSIjMDBCM0ZGIi8+CjxwYXRoIGQ9Ik05MC42Mzc0IDExNi41NjlINTMuMzYxNkM1Mi4wNjUxIDExNi41NjkgNTAuOTMwNyAxMTUuNDM1IDUwLjkzMDcgMTE0LjEzOEM1MC45MzA3IDExMi44NDEgNTIuMDY1MSAxMTEuNzA3IDUzLjM2MTYgMTExLjcwN0g5MC42Mzc0QzkxLjkzMzkgMTExLjcwNyA5My4wNjg0IDExMi44NDEgOTMuMDY4NCAxMTQuMTM4QzkzLjA2ODQgMTE1LjQzNSA5MS45MzM5IDExNi41NjkgOTAuNjM3NCAxMTYuNTY5WiIgZmlsbD0iI0U4RURFRSIvPgo8L2c+CjxwYXRoIGQ9Ik03Mi41Mjc1IDUzLjgzNjdDNzIuNDQzMSA1My44MzUxIDcyLjM2MDUgNTMuODEyMiA3Mi4yODczIDUzLjc3MDFMNTYuNDY5OSA0NC43OTM0QzU2LjM5ODMgNDQuNzUxOSA1Ni4zMzg4IDQ0LjY5MjMgNTYuMjk3MyA0NC42MjA3QzU2LjI1NTkgNDQuNTQ5IDU2LjIzMzggNDQuNDY3OCA1Ni4yMzM0IDQ0LjM4NUM1Ni4yMzM0IDQ0LjIxNjkgNTYuMzI1OCA0NC4wNjE3IDU2LjQ2OTkgNDMuOTc4NUw3Mi4xOTEyIDM1LjA2MDlDNzIuMjYzNyAzNS4wMjEgNzIuMzQ1IDM1IDcyLjQyNzcgMzVDNzIuNTEwNSAzNSA3Mi41OTE4IDM1LjAyMSA3Mi42NjQzIDM1LjA2MDlMODguNDg3MiA0NC4wMzk1Qzg4LjU1OTEgNDQuMDgwMSA4OC42MTg4IDQ0LjEzOTIgODguNjYgNDQuMjEwN0M4OC43MDEzIDQ0LjI4MjIgODguNzIyNyA0NC4zNjM1IDg4LjcyMTkgNDQuNDQ2Qzg4LjcyMjUgNDQuNTI4NSA4OC43MDEgNDQuNjA5NyA4OC42NTk4IDQ0LjY4MTJDODguNjE4NSA0NC43NTI2IDg4LjU1ODkgNDQuODExOCA4OC40ODcyIDQ0Ljg1MjVMNzIuNzcxNCA1My43NjgzQzcyLjY5NzIgNTMuODExNCA3Mi42MTMzIDUzLjgzNDkgNzIuNTI3NSA1My44MzY3IiBmaWxsPSJ3aGl0ZSIvPgo8cGF0aCBvcGFjaXR5PSIwLjciIGQ9Ik03MC4yNTUzIDc1LjY1MTdDNzAuMTcxIDc1LjY1MzUgNzAuMDg3OCA3NS42MzE3IDcwLjAxNTEgNzUuNTg4OEw1NC4yNDU4IDY2LjY0MTdDNTQuMTcxNSA2Ni42MDI0IDU0LjEwOTUgNjYuNTQzNiA1NC4wNjYxIDY2LjQ3MTZDNTQuMDIyOCA2Ni4zOTk3IDU0IDY2LjMxNzMgNTQgNjYuMjMzM1Y0OC4yNzhDNTQgNDguMTA4IDU0LjA5MjQgNDcuOTU0NiA1NC4yNDM5IDQ3Ljg2OTZDNTQuMzE3MiA0Ny44MjcxIDU0LjQwMDQgNDcuODA0NyA1NC40ODUxIDQ3LjgwNDdDNTQuNTY5NyA0Ny44MDQ3IDU0LjY1MjkgNDcuODI3MSA1NC43MjYyIDQ3Ljg2OTZMNzAuNDkzNyA1Ni44MTMxQzcwLjU2NDIgNTYuODU2NSA3MC42MjI1IDU2LjkxNyA3MC42NjMyIDU2Ljk4OTFDNzAuNzAzOSA1Ny4wNjEyIDcwLjcyNTcgNTcuMTQyNCA3MC43MjY1IDU3LjIyNTFWNzUuMTgwNUM3MC43MjU5IDc1LjI2MjggNzAuNzA0MiA3NS4zNDM2IDcwLjY2MzUgNzUuNDE1MUM3MC42MjI3IDc1LjQ4NjYgNzAuNTY0MiA3NS41NDY0IDcwLjQ5MzcgNzUuNTg4OEM3MC40MjA2IDc1LjYyOTEgNzAuMzM4NyA3NS42NTA3IDcwLjI1NTMgNzUuNjUxNyIgZmlsbD0id2hpdGUiLz4KPHBhdGggb3BhY2l0eT0iMC40IiBkPSJNNzQuNzE5OCA3NS42NTExQzc0LjYzMzMgNzUuNjUxMiA3NC41NDgyIDc1LjYyOTYgNzQuNDcyMiA3NS41ODgzQzc0LjQwMTYgNzUuNTQ2MSA3NC4zNDMyIDc1LjQ4NjIgNzQuMzAyNyA3NS40MTQ3Qzc0LjI2MjMgNzUuMzQzMSA3NC4yNDExIDc1LjI2MjIgNzQuMjQxMiA3NS4xOFY1Ny4zMzczQzc0LjI0MTIgNTcuMTcxIDc0LjMzMzYgNTcuMDE1OCA3NC40NzIyIDU2LjkyOUw5MC4yMzk3IDQ3Ljk4NTVDOTAuMzExOSA0Ny45NDM4IDkwLjM5MzggNDcuOTIxOSA5MC40NzcxIDQ3LjkyMTlDOTAuNTYwNSA0Ny45MjE5IDkwLjY0MjQgNDcuOTQzOCA5MC43MTQ2IDQ3Ljk4NTVDOTAuNzg3NiA0OC4wMjU1IDkwLjg0ODUgNDguMDg0MiA5MC44OTExIDQ4LjE1NTdDOTAuOTMzNyA0OC4yMjcyIDkwLjk1NjMgNDguMzA4OCA5MC45NTY2IDQ4LjM5MlY2Ni4yMzI4QzkwLjk1NyA2Ni4zMTY0IDkwLjkzNDcgNjYuMzk4NSA5MC44OTIxIDY2LjQ3MDRDOTAuODQ5NSA2Ni41NDI0IDkwLjc4ODEgNjYuNjAxNCA5MC43MTQ2IDY2LjY0MTFMNzQuOTUyNiA3NS41ODgzQzc0Ljg4MjUgNzUuNjMwNyA3NC44MDE4IDc1LjY1MjUgNzQuNzE5OCA3NS42NTExIiBmaWxsPSJ3aGl0ZSIvPgo8ZGVmcz4KPGxpbmVhckdyYWRpZW50IGlkPSJwYWludDBfbGluZWFyXzY4N18zNDU0IiB4MT0iMTYxIiB5MT0iMTgwIiB4Mj0iMy41OTI4NGUtMDciIHkyPSI0Ljk5OTk4IiBncmFkaWVudFVuaXRzPSJ1c2VyU3BhY2VPblVzZSI+CjxzdG9wLz4KPHN0b3Agb2Zmc2V0PSIxIiBzdG9wLWNvbG9yPSIjNTk1NjU2Ii8+CjwvbGluZWFyR3JhZGllbnQ+CjxjbGlwUGF0aCBpZD0iY2xpcDBfNjg3XzM0NTQiPgo8cmVjdCB3aWR0aD0iOTQiIGhlaWdodD0iOTQiIGZpbGw9IndoaXRlIiB0cmFuc2Zvcm09InRyYW5zbGF0ZSgyNSAyNSkiLz4KPC9jbGlwUGF0aD4KPC9kZWZzPgo8L3N2Zz4K | ||
| keysOrder: [["apiVersion"], ["appVersion"], ["kind"], ["metadata"], ["metadata", "name"], ["spec", "external"], ["spec", "externalMethod"], ["spec", "externalPorts"], ["spec", "running"], ["spec", "instanceType"], ["spec", "instanceProfile"], ["spec", "disks"], ["spec", "gpus"], ["spec", "resources"], ["spec", "sshKeys"], ["spec", "cloudInit"], ["spec", "cloudInitSeed"]] | ||
| keysOrder: [["apiVersion"], ["appVersion"], ["kind"], ["metadata"], ["metadata", "name"], ["spec", "external"], ["spec", "externalMethod"], ["spec", "externalPorts"], ["spec", "running"], ["spec", "instanceType"], ["spec", "instanceProfile"], ["spec", "disks"], ["spec", "gpus"], ["spec", "subnets"], ["spec", "resources"], ["spec", "sshKeys"], ["spec", "cloudInit"], ["spec", "cloudInitSeed"]] |
There was a problem hiding this comment.
Resolve keysOrder inconsistency for subnets field across VM CRDs.
The subnets field positioning differs between vm-instance and virtual-machine CRD definitions:
- virtual-machine.yaml: subnets positioned between systemDisk and gpus
- vm-instance.yaml: subnets positioned after gpus and before resources
This creates inconsistent UI field ordering. Align the positioning across both CRDs for a consistent user experience.
🤖 Prompt for AI Agents
packages/system/cozystack-resource-definitions/cozyrds/vm-instance.yaml around
line 31: the keysOrder entry places ["spec","subnets"] after gpus and before
resources, which is inconsistent with virtual-machine.yaml where subnets is
positioned between the systemDisk and gpus; update the keysOrder array so
["spec","subnets"] appears in the same position as in virtual-machine.yaml
(i.e., move the ["spec","subnets"] element to be immediately before
["spec","gpus"] / after the system disk entry) to ensure consistent UI field
ordering across both CRDs.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
lllamnyp
force-pushed
the
testing/add-vpc
branch
from
edd6aa2 to
eeb85ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/apps/virtual-machine/templates/secret-network-config.yaml (2)
1-1: Fix comment typo.Line 1 comment says "ubunu" but should say "ubuntu". While the actual Helm condition on line 2 correctly uses "ubuntu", the comment should be fixed for clarity.
-# if subnets are specified and image is either ubunu or alpine +# if subnets are specified and image is either ubuntu or alpine
35-35: CRITICAL: Fix invalid Helm function syntax.Line 35 uses
add1which is not a valid Helm function. The Helmaddfunction requires explicit spacing:add 1 $i.- name: eth{{ add1 $i }} + name: eth{{ add 1 $i }}This syntax error will cause template rendering to fail when subnets are configured on Alpine images.
🧹 Nitpick comments (3)
packages/apps/vpc/Makefile (1)
1-8: Consider defining standard phony targets.The Makefile defines
generateandupdatetargets appropriately for the VPC component. Per static analysis, consider adding standard.PHONYdeclarations forgenerate,update, and optionallyall,clean,testto follow Make conventions, even if some targets are not implemented yet.Example:
+.PHONY: generate update + include ../../../scripts/package.mkAdditionally, the
updatetarget currently runsechoas a placeholder. Consider whether it should perform meaningful work (e.g., rungenerateor validate resources) or document its intended purpose.packages/apps/vpc/templates/vpc.yaml (1)
20-20: Extract duplicated subnetId calculation to avoid inconsistency.The subnetId generation logic on line 20 is duplicated on line 69 inside the ConfigMap data section. This creates a maintenance risk: if the ID calculation needs to change, both locations must be updated in lockstep or the IDs will diverge.
Define a Helm template or variable to compute subnetId once and reuse it:
{{- $subnetId := print "subnet-" (print $.Release.Namespace "/" $vpcId "/" $subnetName | sha256sum | trunc 8) }}Then reference
$subnetIdin both the loop (line 20) and ConfigMap data (line 69) to ensure consistency.Alternatively, if Helm named templates are used elsewhere in the project, consider defining a named template for this calculation.
Also applies to: 69-69
packages/apps/virtual-machine/templates/vm.yaml (1)
96-96: Extract repeated condition to improve maintainability.The Helm condition
(and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")))appears on lines 96, 132, and 139. This repetition makes the template harder to maintain and increases the risk of inconsistency if the condition needs to change.Consider defining this as a variable at the top of the template:
{{- $needsNetConfig := and .Values.subnets (or (eq .Values.systemDisk.image "ubuntu") (eq .Values.systemDisk.image "alpine")) -}}Then reuse it:
{{- if or .Values.cloudInit .Values.sshKeys $needsNetConfig }} ... {{- if $needsNetConfig }} networkDataSecretRef: name: {{ include "virtual-machine.fullname" . }}-network-config {{- end }}This approach has been previously suggested and improves readability and maintainability.
Also applies to: 132-132, 139-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/apps/vpc/logos/vpc.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
packages/apps/tenant/templates/tenant.yaml(1 hunks)packages/apps/virtual-machine/README.md(1 hunks)packages/apps/virtual-machine/templates/secret-network-config.yaml(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(4 hunks)packages/apps/virtual-machine/values.schema.json(1 hunks)packages/apps/virtual-machine/values.yaml(2 hunks)packages/apps/vm-instance/README.md(1 hunks)packages/apps/vm-instance/templates/vm.yaml(2 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(2 hunks)packages/apps/vpc/Chart.yaml(1 hunks)packages/apps/vpc/Makefile(1 hunks)packages/apps/vpc/templates/vpc.yaml(1 hunks)packages/apps/vpc/values.schema.json(1 hunks)packages/apps/vpc/values.yaml(1 hunks)packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml(2 hunks)packages/system/cozystack-resource-definitions/cozyrds/virtualprivatecloud.yaml(1 hunks)packages/system/cozystack-resource-definitions/cozyrds/vm-instance.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/apps/virtual-machine/README.md
- packages/apps/vm-instance/templates/vm.yaml
- packages/system/cozystack-resource-definitions/cozyrds/vm-instance.yaml
- packages/apps/vm-instance/README.md
- packages/apps/tenant/templates/tenant.yaml
- packages/apps/virtual-machine/values.schema.json
- packages/apps/vm-instance/values.yaml
- packages/apps/vpc/values.schema.json
- packages/apps/virtual-machine/values.yaml
- packages/system/cozystack-resource-definitions/cozyrds/virtualprivatecloud.yaml
- packages/apps/vm-instance/values.schema.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T04:52:22.093Z
Learnt from: lllamnyp
PR: cozystack/cozystack#1515
File: packages/system/lineage-controller-webhook/templates/daemonset.yaml:29-34
Timestamp: 2025-10-14T04:52:22.093Z
Learning: In Kubernetes YAML manifests and Helm templates, list items can be formatted with the leading `- ` aligned at the same level as the parent key (e.g., `args:\n- item1\n- item2`) because Kubernetes uses the YAML v2 library which considers the leading `- ` to be part of the indentation. This is the standard style for Kubernetes resources.
Applied to files:
packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml
🪛 checkmake (0.2.2)
packages/apps/vpc/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 YAMLlint (1.37.1)
packages/apps/virtual-machine/templates/secret-network-config.yaml
[error] 2-2: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/vpc/templates/vpc.yaml
[error] 20-20: syntax error: could not find expected ':'
(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). (4)
- GitHub Check: Test kubernetes-previous
- GitHub Check: Test postgres
- GitHub Check: Test foundationdb
- GitHub Check: Test kafka
🔇 Additional comments (7)
packages/system/cozystack-resource-definitions/cozyrds/virtual-machine.yaml (1)
11-39: Well-integrated CRD schema updates.The openAPISchema addition of the subnets field and keysOrder update are consistent with parallel changes in VM values layers. The services block exposes the VM resource appropriately.
packages/apps/vpc/Chart.yaml (1)
1-6: Chart structure looks good.The VPC chart manifest follows Helm conventions. The version placeholder and build-time substitution pattern align with the project's build process mentioned in the comment.
packages/apps/vpc/values.yaml (1)
1-14: Clear and well-documented subnet configuration.The Subnet typedef and subnets map structure are appropriately defined, with helpful examples showing CIDR configuration. This aligns with subnet usage across VM-related templates.
packages/apps/virtual-machine/templates/secret-network-config.yaml (1)
2-2: Verify YAML syntax after correcting Helm function.YAMLlint reports a syntax error on line 2. This may be a parsing artifact from the
add1error above, but verify that the template parses correctly after applying the syntax fix.packages/apps/vpc/templates/vpc.yaml (1)
33-38: Review NetworkAttachmentDefinition config format.Lines 33–38 define a JSON config string inline within YAML. The format looks valid, but verify that the escaped provider string correctly references the subnet namespace and the OVN network naming convention. A test deployment would confirm correct network attachment.
packages/apps/virtual-machine/templates/vm.yaml (2)
54-54: Verify updated cloud image URLs are accessible.The Fedora image has been updated to version 42 and Alpine to 3.20.8. Previous review feedback indicated these URLs may need correction. Please verify:
- Fedora 42 URL (line 54): Confirm whether the domain should be
download.fedoraproject.orgordownload2.fedora.redhat.com, and that the image is accessible.- Alpine 3.20.8 (line 56): Verify that a released alpine-virt cloud image labeled 3.20.8 exists. Previous feedback suggested the latest alpine-virt in the v3.20 tree may be 3.20.6.
Please either confirm these URLs are correct or update them to valid, accessible images.
Also applies to: 56-56
105-110: Subnet interface and network configuration looks well-structured.The conditional loops over
.Values.subnetsto create per-subnet interfaces (lines 105–110) and corresponding Multus network entries (lines 148–154) are correctly guarded and properly scoped. The naming consistency using$subnet.nameand namespace scoping are appropriate.Also applies to: 148-154
What this PR does
This is a PR to generate a build against the
add-vpcbranch with the latest changes frommainincluded. DO NOT MERGE.Summary by CodeRabbit
New Features
Documentation
Chores