feat/impruvement-kubernetes-tests - #1485
Conversation
|
Warning Rate limit exceededIvanHunters has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughScript extends e2e Kubernetes validation: increases kube-apiserver port-forward timeout, adds node readiness and kubeletVersion checks, adds per-component HelmRelease waits, and lengthens machine deployment replicas wait. Makefile bumps KUBERNETES_VERSION v1.32 → v1.33. No public APIs changed. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Dev/CI
participant Script as run-kubernetes.sh
participant API as kube-apiserver (port-forward)
participant Cluster as Kubernetes
participant HR as HelmRelease controller
Runner->>Script: start e2e script
Script->>API: port-forward (timeout 200s)
API-->>Script: connection established
Script->>Cluster: wait for 2 Nodes ready (2m)
Cluster-->>Script: nodes list (wide)
Script->>Cluster: verify kubeletVersion across nodes
alt kubelet versions mismatch
Cluster-->>Script: report mismatch
Script-->>Runner: exit with error
else kubelet versions match
Script->>HR: wait cilium Ready (1m)
Script->>HR: wait coredns Ready (1m)
Script->>HR: wait csi Ready (1m)
Script->>HR: wait ingress-nginx Ready (1m)
Script->>HR: wait vsnap-crd Ready (1m)
Script->>Cluster: wait machinedeployment readyReplicas (10m)
Cluster-->>Script: all ready / timeout
Script-->>Runner: proceed or fail
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 IvanHunters, 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 significantly improves the stability and maintainability of Kubernetes deployments and testing. It incorporates an upgrade to the latest Kubernetes version, enhances the reliability of end-to-end tests by adding more thorough validation steps, and refines the installer build process for better platform compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request improves the Kubernetes e2e tests by adding more verification steps, such as checking node readiness and kubelet versions. The changes are a good step towards more robust testing. I've provided a few suggestions to fix a bug in the version check logic and to improve the script's maintainability and robustness. As a minor note, there's a typo in the pull request title: 'impruvement' should be 'improvement'.
| if [[ ! "$v" =~ ^"${k8s_version}"[^$]+ ]]; then | ||
| node_ok=false | ||
| fi |
There was a problem hiding this comment.
The regex used for verifying the kubelet version is incorrect. The pattern ^\"${k8s_version}\"[^$]+ requires at least one character to follow the version string, which means an exact version match will fail this check. For example, if ${k8s_version} is v1.33.0 and the kubelet version is also v1.33.0, the check will incorrectly fail.
I've suggested a fix that corrects the regex and adds a break to exit the loop on the first failure, making it more efficient.
| if [[ ! "$v" =~ ^"${k8s_version}"[^$]+ ]]; then | |
| node_ok=false | |
| fi | |
| if [[ ! "$v" =~ ^"${k8s_version}" ]]; then | |
| node_ok=false | |
| break | |
| fi |
|
|
||
| # Wait for the nodes to be ready (timeout after 2 minutes) | ||
| timeout 2m bash -c ' | ||
| until [ "$(kubectl --kubeconfig tenantkubeconfig get nodes -o jsonpath="{.items[*].metadata.name}" | wc -w)" -eq 2 ]; do |
There was a problem hiding this comment.
Using wc -w to count nodes can be brittle. For example, it would miscount if there were extra spaces between node names. A more robust approach is to squeeze spaces, convert them to newlines with tr -s, and then count the lines with wc -l.
| until [ "$(kubectl --kubeconfig tenantkubeconfig get nodes -o jsonpath="{.items[*].metadata.name}" | wc -w)" -eq 2 ]; do | |
| until [ "$(kubectl --kubeconfig tenantkubeconfig get nodes -o jsonpath="{.items[*].metadata.name}" | tr -s ' ' '\n' | wc -l)" -eq 2 ]; do |
| kubectl wait hr kubernetes-${test_name}-cilium -n tenant-test --timeout=1m --for=condition=ready | ||
| kubectl wait hr kubernetes-${test_name}-coredns -n tenant-test --timeout=1m --for=condition=ready | ||
| kubectl wait hr kubernetes-${test_name}-csi -n tenant-test --timeout=1m --for=condition=ready | ||
| kubectl wait hr kubernetes-${test_name}-ingress-nginx -n tenant-test --timeout=1m --for=condition=ready | ||
| kubectl wait hr kubernetes-${test_name}-vsnap-crd -n tenant-test --timeout=1m --for=condition=ready |
There was a problem hiding this comment.
These kubectl wait commands for HelmReleases are repetitive. You can use a for loop to make the script more concise and easier to maintain. This will be helpful if you need to add or remove components in the future.
| kubectl wait hr kubernetes-${test_name}-cilium -n tenant-test --timeout=1m --for=condition=ready | |
| kubectl wait hr kubernetes-${test_name}-coredns -n tenant-test --timeout=1m --for=condition=ready | |
| kubectl wait hr kubernetes-${test_name}-csi -n tenant-test --timeout=1m --for=condition=ready | |
| kubectl wait hr kubernetes-${test_name}-ingress-nginx -n tenant-test --timeout=1m --for=condition=ready | |
| kubectl wait hr kubernetes-${test_name}-vsnap-crd -n tenant-test --timeout=1m --for=condition=ready | |
| for component in cilium coredns csi ingress-nginx vsnap-crd; do | |
| kubectl wait hr kubernetes-${test_name}-${component} -n tenant-test --timeout=1m --for=condition=ready | |
| done |
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 (5)
hack/e2e-apps/run-kubernetes.sh(1 hunks)packages/apps/kubernetes/Makefile(1 hunks)packages/apps/tenant/values.yaml(1 hunks)packages/core/installer/Makefile(1 hunks)packages/core/installer/values.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- packages/core/installer/values.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/apps/kubernetes/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 (3)
packages/core/installer/Makefile (1)
27-28: Explicit amd64 platform is helpfulLocking the build to
linux/amd64avoids accidental arm64 outputs on Apple Silicon hosts; change looks good.packages/apps/tenant/values.yaml (1)
10-10: Enabling per-tenant EtcdTurning this on by default aligns the test environment with real deployments; no issues on my side.
packages/apps/kubernetes/Makefile (1)
1-1: Kubernetes version bump looks consistentAll build targets pull from this variable, so updating it here keeps the images in sync. Looks good.
ca586b9 to
ae0d0e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hack/e2e-apps/run-kubernetes.sh (1)
103-115: Fix kubelet version check to accept exact matches.Line 107’s regex
^"${k8s_version}"[^$]+insists on an extra character after the version, so nodes reporting the exact semver (e.g.,v1.33.2) always fail this test. Switch to a prefix/exact match and break as soon as a mismatch appears.- for v in $versions; do - if [[ ! "$v" =~ ^"${k8s_version}"[^$]+ ]]; then - node_ok=false - fi - done + for v in $versions; do + case "$v" in + "${k8s_version}"|"${k8s_version}".*|"${k8s_version}"-*) + ;; + *) + node_ok=false + break + ;; + esac + done
🧹 Nitpick comments (1)
hack/e2e-apps/run-kubernetes.sh (1)
89-90: Update comment to reflect 150-second timeout.Line 89 still references 40 seconds, but the command now runs with
timeout 150s. Please sync the comment to avoid confusion.- # Set up port forwarding to the Kubernetes API server for a 40 second timeout + # Set up port forwarding to the Kubernetes API server with a 150-second timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hack/e2e-apps/run-kubernetes.sh(1 hunks)packages/apps/kubernetes/Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/apps/kubernetes/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
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
ae0d0e8 to
670341f
Compare
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-apps/run-kubernetes.sh (1)
89-91: Update the timeout comment.The comment still says “40 second timeout,” but the command now uses
timeout 200s. Please sync the wording to avoid confusion for future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-apps/run-kubernetes.sh(1 hunks)
⏰ 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
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hack/e2e-apps/run-kubernetes.sh (1)
89-90: Align the timeout comment with the code.Line 89 still claims a 40 s timeout, but the command on Line 90 now runs
timeout 200s. Please update the comment to avoid confusing future readers.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
What this PR does
Release note
Summary by CodeRabbit
New Features
Improvements
Chores