[kubeovn] Package from external repo #1535
Conversation
Previously, all system charts have had their versions bumped with every update of Cozystack, even if the underlying images and templates did not change. Additionally, some components would have their docker images rebuilt and get updated digests because of a changed docker context, even though no relevant code had been changed. This would cause an upgrade of several HelmReleases on every upgrade of Cozystack, even when in practice nothing had changed. This is especially noticeable for charts, such as kubeovn, which is a dependency for almost all other system HelmReleases. By moving kubeovn out of the monorepo and decoupling its release cycle from the release cycle of Cozystack, this issue can be resolved and is another step towards decomposing Cozystack. ```release-note [kubeovn] Decouple the release cycle of kubeovn from Cozystack's release cycle, preventing unnecessary redeployments of all Helm releases, when upgrading Cozystack. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
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. WalkthroughReplaces local kube-ovn image build with a fixed external tag, splits chart packaging into placeholder and versioned sets, updates kube-ovn chart/version and image digest, and applies patches: concurrent northd health checks, configmap-driven kube-ovn config, and removal of MTU templating. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 implements a significant architectural change by decoupling the Kube-OVN component's management from the main Cozystack monorepo. By transitioning Kube-OVN from an internally built and patched component to an externally sourced package, it addresses issues of frequent, redundant HelmRelease upgrades and Docker image rebuilds. This strategic decoupling aims to enhance the efficiency of Cozystack upgrades and simplify the overall system by reducing tight interdependencies. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively decouples the kubeovn chart from the main Cozystack release cycle by fetching it from an external repository. This is a good architectural improvement that will prevent unnecessary redeployments. The changes are generally well-implemented, but I've identified a couple of areas in the Makefiles where the scripting could be made more robust and maintainable. My review includes suggestions to address these points.
| CHARTS := $(shell grep -F 'version: 0.0.0' */Chart.yaml | cut -f1 -d/) | ||
| VERSIONED_CHARTS := $(shell grep '^version:' */Chart.yaml | grep -Fv '0.0.0' | cut -f1 -d/) |
There was a problem hiding this comment.
This new logic for identifying charts is a good improvement. However, there are a couple of potential issues:
-
The
grep -Fv '0.0.0'command forVERSIONED_CHARTSis a bit risky as it filters any line containing the substring0.0.0. This could lead to false negatives if a versioned chart'sChart.yamlcontains0.0.0in a comment. It's safer to filter based on the exactversion: 0.0.0string. -
The
fix-chartstarget (not in this diff) will reset all chart versions to0.0.0, includingkubeovn. This would undo the decoupling this PR aims to achieve. You should consider updatingfix-chartsto only process charts that are meant to have their version managed by the main project, for example by operating on the$(CHARTS)variable.
Here's a suggestion to address the first point:
CHARTS := $(shell grep -F 'version: 0.0.0' */Chart.yaml | cut -f1 -d/)
VERSIONED_CHARTS := $(shell grep '^version:' */Chart.yaml | grep -v 'version: 0.0.0' | cut -f1 -d/)
packages/system/kubeovn/Makefile
Outdated
| @@ -1,4 +1,4 @@ | |||
| KUBEOVN_TAG=$(shell awk '$$1 == "version:" {print $$2}' charts/kube-ovn/Chart.yaml) | |||
| KUBEOVN_TAG=v0.37.3 | |||
There was a problem hiding this comment.
The KUBEOVN_TAG is hardcoded. This value is also present in packages/system/kubeovn/Chart.yaml. To avoid inconsistencies and the need to update the version in two places, it would be better to derive the tag from Chart.yaml.
KUBEOVN_TAG := v$(shell awk '/^version:/ {print $$2}' Chart.yaml)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/kubeovn/Makefile (1)
1-13: CRITICAL: Build is broken—make buildwill fail when callingmake -C packages/system/kubeovn image.The top-level Makefile's
buildtarget explicitly callsmake -C packages/system/kubeovn image, but the refactored kubeovn/Makefile has noimage:target defined. The old Makefile had this target; it was removed in commit d26d3e1 when moving chart packaging to an external repository.While the architectural change to decouple kubeovn from Cozystack is intentional, the image build step was not addressed. Either:
- Restore an
image:target in packages/system/kubeovn/Makefile (can be empty/stub if external repo handles image builds), or- Remove
make -C packages/system/kubeovn imagefrom the top-levelbuildtarget if kubeovn no longer requires local image building.This must be fixed before merging.
🧹 Nitpick comments (1)
packages/system/kubeovn/charts/kube-ovn/Chart.yaml (1)
18-18: Chart version uses non-standard format.The chart version is set to
v1.14.11with a leading 'v', which deviates from standard Semantic Versioning and Helm conventions (which use1.14.11without a prefix). While theappVersioncorrectly omits the prefix, the chart version should too.Consider standardizing to
version: 1.14.11for consistency with Helm best practices.However, if the project intentionally uses the 'v' prefix across all charts, this can be deferred or handled uniformly in a separate pass.
Also applies to: 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/system/Makefile(1 hunks)packages/system/kubeovn/Chart.yaml(1 hunks)packages/system/kubeovn/Makefile(2 hunks)packages/system/kubeovn/charts/kube-ovn/Chart.yaml(1 hunks)packages/system/kubeovn/charts/kube-ovn/templates/kube-ovn-crd.yaml(8 hunks)packages/system/kubeovn/charts/kube-ovn/templates/ovncni-ds.yaml(1 hunks)packages/system/kubeovn/charts/kube-ovn/values.yaml(2 hunks)packages/system/kubeovn/images/kubeovn/Dockerfile(0 hunks)packages/system/kubeovn/images/kubeovn/patches/northd_probe.diff(0 hunks)packages/system/kubeovn/patches/cozyconfig.diff(0 hunks)packages/system/kubeovn/patches/mtu.diff(0 hunks)
💤 Files with no reviewable changes (4)
- packages/system/kubeovn/images/kubeovn/Dockerfile
- packages/system/kubeovn/patches/mtu.diff
- packages/system/kubeovn/images/kubeovn/patches/northd_probe.diff
- packages/system/kubeovn/patches/cozyconfig.diff
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/system/kubeovn/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: Pull Request
packages/system/kubeovn/Makefile
[error] 20-20: Command failed: make -C packages/system/kubeovn image. No rule to make target 'image'.
🔇 Additional comments (8)
packages/system/kubeovn/charts/kube-ovn/templates/kube-ovn-crd.yaml (3)
261-262: Schema validation improvements for affinity weight constraints.Affinity weight fields across nodeAffinity, podAffinity, and podAntiAffinity now enforce explicit bounds (minimum: 1, maximum: 100). These align with Kubernetes' Pod Affinity weight semantics and are unlikely to conflict with valid configurations.
Also applies to: 354-355, 443-444, 600-601, 693-694, 782-783
900-901: Replica count bounds added to VpcEgressGateway status schema.The status.replicas field now enforces a maximum of 10 (with minimum: 0). Combined with the default: 1 in spec.replicas (line 997), this is a conservative constraint unlikely to affect real deployments. Verify no production VpcEgressGateway instances require replica counts beyond 10.
1045-1046: BFD timing constraints now explicitly bounded.BFD fields (minRX, minTX, multiplier) in VpcEgressGateway spec now enforce minimum: 1 and maximum: 3600000 (milliseconds). The upper bound of ~1 hour is reasonable for BFD keepalive intervals. Confirm no existing deployments rely on values outside this range.
Also applies to: 1050-1051, 1055-1056
packages/system/kubeovn/Chart.yaml (1)
3-3: Clarify the version value and comment.The comment states the version is a "Placeholder" that will be "automatically set during the build process," but the version is explicitly set to 0.37.3 rather than 0.0.0. These two statements conflict.
If 0.37.3 is the intended fixed version for this wrapper chart (which aligns with the new
VERSIONED_CHARTSapproach inpackages/system/Makefile), update the comment to reflect this. If the version should remain a placeholder, revert to 0.0.0.packages/system/kubeovn/charts/kube-ovn/values.yaml (1)
12-12: Image tag and IPsec configuration changes look good.The bump from v1.14.5 to v1.14.11 is aligned with the external repository versioning. The new
OVN_IPSEC_KEY_DIRconfiguration is properly positioned and will be correctly referenced in the template conditional blocks for IPsec support.Also applies to: 126-126
packages/system/kubeovn/charts/kube-ovn/templates/ovncni-ds.yaml (1)
275-275: Volume mount path update is properly aligned.The change from
OPENVSWITCH_DIRtoOVN_IPSEC_KEY_DIRis correctly coordinated with the new configuration value invalues.yamland respects the ENABLE_OVN_IPSEC conditional guard. This improves configuration clarity by separating IPsec-specific paths.packages/system/Makefile (2)
2-3: Chart discovery patterns could be fragile to YAML format variations.The grep patterns for identifying placeholder vs. versioned charts assume exact YAML formatting:
- Line 2:
grep -F 'version: 0.0.0'requires exactly this spacing; variations likeversion: '0.0.0'or leading/trailing spaces will be missed.- Line 3:
grep -Fv '0.0.0'will exclude any chart whose version field contains the substring '0.0.0', which could incorrectly filter out versions like "1.0.0-0.0.0" if such a scheme were used.Consider using more defensive patterns, such as:
- Using
yqor similar YAML parser instead of grep, or- Using a more precise regex like
version:\s+0\.0\.0\s*$Verify that the current grep patterns correctly identify all intended charts in the repository. You could test this by running the patterns against actual
Chart.yamlfiles.
9-10: Verify versioned chart packaging logic.The
repotarget now packages bothCHARTS(with--version $(COZYSTACK_VERSION)) andVERSIONED_CHARTS(without version override).Confirm that this dual-packaging approach correctly handles the kubeovn chart and any other versioned charts, ensuring:
- VERSIONED_CHARTS are packaged with their declared versions (e.g., 0.37.3 for kubeovn), not overridden by COZYSTACK_VERSION
- No duplicate charts in the repository index
- Chart naming and versioning are consistent in the final helm repo index
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/system/kubeovn/Makefile (2)
1-1: Consider deriving KUBEOVN_TAG from Chart.yaml to reduce maintenance burden.The KUBEOVN_TAG is currently hardcoded to v0.38.0. Since Chart.yaml presumably contains the corresponding version, hardcoding both values creates a single point of failure where version updates must be coordinated across two files.
The previous reviewer suggested deriving the tag directly from Chart.yaml:
KUBEOVN_TAG := v$(shell awk '/^version:/ {print $$2}' Chart.yaml)This approach ensures version consistency and reduces manual synchronization overhead.
10-13: Add error handling to the shell script in the update target.The
curlandtarcommands lack error handling. If either command fails, the target may report success while leaving the charts directory in an inconsistent state. This can cause downstream build failures that are difficult to diagnose.Apply this diff to add explicit error handling:
update: - rm -rf charts values.yaml Chart.yaml - tag=$(KUBEOVN_TAG) && \ - curl -sSL https://github.com/cozystack/kubeovn/archive/refs/tags/$${tag}.tar.gz | \ - tar xzvf - --strip 2 kubeovn-$${tag#*v}/chart + set -e; \ + rm -rf charts values.yaml Chart.yaml; \ + tag=$(KUBEOVN_TAG); \ + curl -sSL https://github.com/cozystack/kubeovn/archive/refs/tags/$${tag}.tar.gz | \ + tar xzvf - --strip 2 kubeovn-$${tag#*v}/chart || exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(0 hunks)packages/system/kubeovn/Chart.yaml(1 hunks)packages/system/kubeovn/Makefile(2 hunks)packages/system/kubeovn/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/kubeovn/Chart.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-14T16:58:01.478Z
Learnt from: nbykov0
Repo: cozystack/cozystack PR: 932
File: packages/system/dashboard/images/kubeapps-apis/Dockerfile:15-17
Timestamp: 2025-05-14T16:58:01.478Z
Learning: When working with Dockerfiles for cross-architecture builds in the kubeapps-apis component, the `TARGETOS` and `TARGETARCH` build arguments should be declared without default values so that Docker's buildx system can automatically set these variables based on the specified target platform.
Applied to files:
packages/system/kubeovn/Makefile
📚 Learning: 2025-11-03T15:25:49.150Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1579
File: packages/system/cozystack-api/templates/hook.yaml:32-32
Timestamp: 2025-11-03T15:25:49.150Z
Learning: The Docker image tag alpine/k8s:1.33.4 is valid and exists (published Aug 17, 2025).
Applied to files:
packages/system/kubeovn/values.yaml
🪛 checkmake (0.2.2)
packages/system/kubeovn/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)
⏰ 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
fc4286a to
278998a
Compare
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
278998a to
e50950b
Compare
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(0 hunks)packages/system/kubeovn/Chart.yaml(1 hunks)packages/system/kubeovn/Makefile(2 hunks)packages/system/kubeovn/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/kubeovn/Chart.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/charts/** : Do not directly modify upstream charts; use vendoring approach instead
Applied to files:
packages/system/kubeovn/Makefile
🪛 checkmake (0.2.2)
packages/system/kubeovn/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)
⏰ 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 (2)
packages/system/kubeovn/Makefile (2)
10-13: Simplified update logic aligns with external packaging approach.The removal of patch application, Dockerfile edits, and image building steps streamlines the update process. This change appropriately reflects the shift from in-tree image construction to externally-packaged artifacts, decoupling kubeovn's release cycle from Cozystack's.
1-1: No action needed. TheKUBEOVN_TAGvalue ofv0.39.0is properly aligned with the Chart.yaml version0.39.0. The version prefix difference ('v' in the tag vs. bare version number) follows standard Git tagging conventions.
| kubeovn: | ||
| repository: kubeovn | ||
| tag: v1.14.11@sha256:8e6cf216687b4a80c35fa7c60bb4d511dd6aaaaf19d1ec53321dfef98d343f51 | ||
| tag: v1.14.11@sha256:0e3e9db960a9600d58c33c0787cabd0e9bf263930fd8c9fe65417e258c383d01 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cozystack kubeovn v1.14.11 image sha256 digest
💡 Result:
kubeovn/kube-ovn:v1.14.11 image digest: sha256:a46dd1c9f6d769ae87a98e4cf631cd0832616359fed1021dc5314af8b634ca1d. [1]
(Alternate repo digest on a mirror: sha256:0aeb91aa7ffbb0e74000eb715eac68b46ada797abcf08cf7a5fafbac239c6a3b). [1]
Sources: Docker image metadata. [1]
🌐 Web query:
kubeovn v1.14.11 docker image official registry
💡 Result:
The official image is on Docker Hub: docker.io/kubeovn/kube-ovn:v1.14.11 (repo digest sha256:a46dd1c9f6d769ae87a98e4cf631cd0832616359fed1021dc5314af8b634ca1d). [1][2]
Sources:
- Docker Hub image listing. [1]
- Kube-OVN documentation / release notes (images pushed to Docker Hub). [2]
Update the kubeovn image digest to match the official Docker Hub source.
The digest 0e3e9db960a9600d58c33c0787cabd0e9bf263930fd8c9fe65417e258c383d01 does not match the official kubeovn/kube-ovn:v1.14.11 image on Docker Hub (sha256:a46dd1c9f6d769ae87a98e4cf631cd0832616359fed1021dc5314af8b634ca1d) or known mirror digests. Replace with the official digest to ensure image integrity and traceability.
🤖 Prompt for AI Agents
In packages/system/kubeovn/values.yaml around line 68, the image tag digest for
kubeovn (v1.14.11) is incorrect; replace the current digest value (0e3e9db96...)
with the official Docker Hub digest for kubeovn/kube-ovn:v1.14.11
(sha256:a46dd1c9f6d769ae87a98e4cf631cd0832616359fed1021dc5314af8b634ca1d) so the
tag line reads the same version with the official SHA256 digest to ensure image
integrity and traceability.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.39
git worktree add -d .worktree/backport-1535-to-release-0.39 origin/release-0.39
cd .worktree/backport-1535-to-release-0.39
git switch --create backport-1535-to-release-0.39
git cherry-pick -x d26d3e1f40f8a3bd2dd775e5162c39ae66cb4413 e50950b7a1711b09e226e8cd96eef847e8c5cb08 |
Previously, all system charts have had their versions bumped with every update of Cozystack, even if the underlying images and templates did not change. Additionally, some components would have their docker images rebuilt and get updated digests because of a changed docker context, even though no relevant code had been changed. This would cause an upgrade of several HelmReleases on every upgrade of Cozystack, even when in practice nothing had changed. This is especially noticeable for charts, such as kubeovn, which is a dependency for almost all other system HelmReleases. By moving kubeovn out of the monorepo and decoupling its release cycle from the release cycle of Cozystack, this issue can be resolved and is another step towards decomposing Cozystack. ```release-note [kubeovn] Decouple the release cycle of kubeovn from Cozystack's release cycle, preventing unnecessary redeployments of all Helm releases, when upgrading Cozystack. ```
Previously, all system charts have had their versions bumped with every update of Cozystack, even if the underlying images and templates did not change. Additionally, some components would have their docker images rebuilt and get updated digests because of a changed docker context, even though no relevant code had been changed. This would cause an upgrade of several HelmReleases on every upgrade of Cozystack, even when in practice nothing had changed. This is especially noticeable for charts, such as kubeovn, which is a dependency for almost all other system HelmReleases. By moving kubeovn out of the monorepo and decoupling its release cycle from the release cycle of Cozystack, this issue can be resolved and is another step towards decomposing Cozystack. ```release-note [kubeovn] Decouple the release cycle of kubeovn from Cozystack's release cycle, preventing unnecessary redeployments of all Helm releases, when upgrading Cozystack. ```
What this PR does
Previously, all system charts have had their versions bumped with every update of Cozystack, even if the underlying images and templates did not change. Additionally, some components would have their docker images rebuilt and get updated digests because of a changed docker context, even though no relevant code had been changed. This would cause an upgrade of several HelmReleases on every upgrade of Cozystack, even when in practice nothing had changed. This is especially noticeable for charts, such as kubeovn, which is a dependency for almost all other system HelmReleases. By moving kubeovn out of the monorepo and decoupling its release cycle from the release cycle of Cozystack, this issue can be resolved and is another step towards decomposing Cozystack.
Release note
Summary by CodeRabbit
New Features
Improvements
Updates
✏️ Tip: You can customize this high-level summary in your review settings.