[tests] VM Disk, VMI, VM, DBs - #1048
Conversation
|
Warning Rate limit exceededAndrei Kvapil (@kvaps) has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 2 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)
WalkthroughThe Bats end-to-end test suite was expanded with six new tests that create and validate VM disk, VM instance, virtual machine, and three types of database resources (PostgreSQL, MySQL, ClickHouse) in the Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Kubernetes
participant Helm
participant Cozystack Controller
Tester->>Kubernetes: kubectl create -f - (inline manifest for VM or DB resource)
Kubernetes->>Helm: Process HelmRelease
Helm->>Cozystack Controller: Deploy VM/DB components
Cozystack Controller->>Kubernetes: Create DataVolume/PVC/VM/VirtualMachineInstance/StatefulSet/Service/Endpoints
Tester->>Kubernetes: Poll for resource readiness and IP/service availability
Kubernetes-->>Tester: Status updates (Ready, Bound, IP assigned, Endpoints available)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
hack/e2e-apps.bats (3)
150-175: Remove duplicate SSH key configuration.The SSH key is configured in both the
cloudInitsection (lines 157-158) and thesshKeyssection (lines 173-175). This duplication is unnecessary and could lead to confusion.Consider removing the SSH key from the
sshKeyssection since it's already configured in the cloud-init:- sshKeys: - - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF - test@test
203-226: Remove duplicate SSH key configuration.Similar to the VM Instance test, the SSH key is configured in both the
cloudInitsection and thesshKeyssection, creating unnecessary duplication.- sshKeys: - - ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPht0dPk5qQ+54g1hSX7A6AUxXJW5T6n/3d7Ga2F8gTF - test@test
233-237: Good timeout strategy, but consider harmonizing HelmRelease timeout.The 200-second timeouts for DataVolume and PVC resources are appropriate for VM provisioning. However, the HelmRelease timeout should also be increased for consistency with the longer resource creation times.
- kubectl -n tenant-test wait hr $vmName --timeout=200s --for=condition=ready + kubectl -n tenant-test wait hr $vmName --timeout=30s --for=condition=readyNote: HelmRelease readiness typically doesn't require the full 200s that DataVolume creation needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-apps.bats(2 hunks)
🔇 Additional comments (1)
hack/e2e-apps.bats (1)
97-238: Overall test structure looks good with room for improvement.The three VM-related tests follow a consistent pattern and integrate well with the existing test suite. The main improvements needed are:
- Fix the critical disk name mismatch in the VM Instance test
- Standardize timeout values across tests
- Remove duplicate SSH key configurations
The tests appropriately validate the end-to-end provisioning of VM resources through HelmReleases and verify the readiness of underlying Kubernetes resources.
| test@test | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr $vmiName --timeout=5s --for=condition=ready |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase HelmRelease timeout for consistency.
Same timeout issue as the VM Disk test - 5 seconds is too short for HelmRelease readiness.
- kubectl -n tenant-test wait hr $vmiName --timeout=5s --for=condition=ready
+ kubectl -n tenant-test wait hr $vmiName --timeout=30s --for=condition=ready📝 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.
| kubectl -n tenant-test wait hr $vmiName --timeout=5s --for=condition=ready | |
| kubectl -n tenant-test wait hr $vmiName --timeout=30s --for=condition=ready |
🤖 Prompt for AI Agents
In hack/e2e-apps.bats at line 178, the HelmRelease readiness timeout is set to 5
seconds, which is too short and inconsistent with other tests. Increase the
timeout value in the kubectl wait command to a longer duration, such as 60
seconds, to allow sufficient time for the HelmRelease to become ready.
| kubectl -n tenant-test wait hr $diskName --timeout=5s --for=condition=ready | ||
| kubectl -n tenant-test wait dv $diskName --timeout=140s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc $diskName --timeout=140s --for=jsonpath='{.status.phase}'=Bound |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase HelmRelease timeout and consider removing arbitrary sleep.
The 5-second timeout for HelmRelease readiness seems very short and could lead to flaky test failures, especially in slower environments. Consider increasing it to be consistent with other resource timeouts.
- sleep 5
- kubectl -n tenant-test wait hr $diskName --timeout=5s --for=condition=ready
+ kubectl -n tenant-test wait hr $diskName --timeout=30s --for=condition=readyCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 125 to 127, the HelmRelease wait command uses
a 5-second timeout which is too short and may cause flaky test failures.
Increase the timeout for the HelmRelease readiness check to match the longer
timeouts used for other resources, such as 140 seconds, to ensure stability.
Also, review and remove any arbitrary sleep commands if they are no longer
necessary after adjusting the timeout.
| diskName='vm-disk-test' | ||
| kubectl create -f - <<EOF | ||
| apiVersion: helm.toolkit.fluxcd.io/v2 | ||
| kind: HelmRelease |
There was a problem hiding this comment.
Cloud you please operate with kind: VMDisk resource in apps.cozystack.io group. This will also test user flow using cozystack-api functionality.
| vmiName='vm-instance-test' | ||
| kubectl create -f - <<EOF | ||
| apiVersion: helm.toolkit.fluxcd.io/v2 | ||
| kind: HelmRelease |
There was a problem hiding this comment.
Cloud you please operate with kind: VMInstance resource in apps.cozystack.io group. This will also test user flow using cozystack-api functionality.
| vmName='virtual-machine-test' | ||
| kubectl create -f - <<EOF | ||
| apiVersion: helm.toolkit.fluxcd.io/v2 | ||
| kind: HelmRelease |
There was a problem hiding this comment.
Cloud you please operate with kind: VirtualMachine resource in apps.cozystack.io group. This will also test user flow using cozystack-api functionality.
There was a problem hiding this comment.
done for all 3 tests. performed test locally, deployment works
0eb9665 to
b49b5da
Compare
b49b5da to
1dab114
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
hack/e2e-apps.bats (1)
335-336: Port check is too strict – order may vary
jsonpath='{.spec.ports[*].port}'can return “8123 9000 9009” or any order; grepping for the whole string is fragile.-timeout 100 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123 9000'; do sleep 10; done" -timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[0].port}' | grep -q '9000 8123 9009'; do sleep 10; done" +timeout 100 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '9000' && grep -q '8123'; do sleep 10; done" +timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[*].port}' | grep -q '9000' && grep -q '8123' && grep -q '9009'; do sleep 10; done"This checks presence of each required port regardless of ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-apps.bats(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
hack/e2e-apps.bats
240-240: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
241-241: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
285-285: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
286-286: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
287-287: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
325-325: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
326-326: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
327-327: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: pre-commit
| sleep 5 | ||
| timeout 15 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" | ||
| kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready | ||
| kubectl -n tenant-test wait vm vm-instance-$name --timeout=20s --for=condition=ready | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timeouts for VM Instance readiness are unrealistically low
Both HelmRelease and VM readiness are capped at 5 s / 20 s – uncommon even for unit tests and certain to flake on CI.
-kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready
-kubectl -n tenant-test wait vm vm-instance-$name --timeout=20s --for=condition=ready
+kubectl -n tenant-test wait hr vm-instance-$name --timeout=60s --for=condition=ready
+kubectl -n tenant-test wait vm vm-instance-$name --timeout=120s --for=condition=readyIncrease the timeout values across all similar waits to avoid random red builds.
📝 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.
| sleep 5 | |
| timeout 15 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" | |
| kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready | |
| kubectl -n tenant-test wait vm vm-instance-$name --timeout=20s --for=condition=ready | |
| } | |
| sleep 5 | |
| timeout 15 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" | |
| kubectl -n tenant-test wait hr vm-instance-$name --timeout=60s --for=condition=ready | |
| kubectl -n tenant-test wait vm vm-instance-$name --timeout=120s --for=condition=ready | |
| } |
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 156 to 160, the timeout values for waiting on
HelmRelease and VM readiness are set too low (5s and 20s), which can cause flaky
test failures. Increase these timeout values to more realistic durations, such
as 60s or higher, to ensure the waits have enough time for the resources to
become ready and reduce random test failures.
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | ||
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=100s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound |
There was a problem hiding this comment.
🛠️ Refactor suggestion
kubectl wait … --timeout=5s is far too aggressive
Helm pulls the image, builds a DataVolume and binds a PVC – 5 seconds is rarely enough even on fast clusters and will cause flaky failures.
-kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready
+kubectl -n tenant-test wait hr vm-disk-$name --timeout=60s --for=condition=readyApply the same 60 s (or more) timeout to the DV/PVC waits for consistency.
📝 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.
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | |
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=100s --for=condition=ready | |
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound | |
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=60s --for=condition=ready | |
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=100s --for=condition=ready | |
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=100s --for=jsonpath='{.status.phase}'=Bound |
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 114 to 116, the kubectl wait command for the
hr resource uses a 5-second timeout, which is too short and causes flaky
failures. Increase this timeout to 60 seconds or more to match the timeouts used
for the dv and pvc resources, ensuring consistency and reducing test flakiness.
68933e6 to
e5e73f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hack/e2e-apps.bats (2)
114-116: Timeout for HelmRelease is still unrealistically low
vm-disk-$nametypically pulls an image and spawns a DataVolume → 5 s is near-certain to flake on CI. Please raise it (≥ 60 s) or remove the manualsleepentirely and rely on a sane timeout.-kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready +kubectl -n tenant-test wait hr vm-disk-$name --timeout=60s --for=condition=ready
157-159: All VM-Instance waits are too aggressive
20 s for IP discovery and 5 s/20 s for HelmRelease / VM readiness will fail on anything slower than a dev laptop. Re-use the 60 s/120 s pattern already adopted elsewhere.-timeout 20 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" -kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready -kubectl -n tenant-test wait vm vm-instance-$name --timeout=20s --for=condition=ready +timeout 120 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" +kubectl -n tenant-test wait hr vm-instance-$name --timeout=60s --for=condition=ready +kubectl -n tenant-test wait vm vm-instance-$name --timeout=120s --for=condition=ready
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-apps.bats(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.26.0)
hack/e2e-apps.bats
240-240: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
241-241: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
285-285: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
286-286: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
287-287: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
325-325: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
326-326: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
327-327: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
hack/e2e-apps.bats (1)
91-91:kubectl wait tcp– verify custom resource abbreviation
tcpis not a standard Kubernetes resource. If this is a shorthand fortenantcontrolplaneensure the CLI plug-in orkubectl api-resourcesrecognises it in CI; otherwise the wait will exit immediately and give a false-positive.
| s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu | ||
| s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog | ||
| resticPassword: ChaXoveekoh6eigh4siesheeda2quai0 |
There was a problem hiding this comment.
Hard-coded access keys & passwords committed to VCS
Real-looking credentials are checked into the repo. Even in test manifests this is a security red-flag and will trigger scanners (see Gitleaks output). Replace them with obviously dummy values or reference Kubernetes Secrets instead.
Example:
- s3AccessKey: oobaiRus9pah8PhohL1ThaeTa4UVa7gu
- s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog
+ s3AccessKey: CHANGEME_ACCESS_KEY # moved to secretRef in CI
+ s3SecretKey: CHANGEME_SECRET_KEY # moved to secretRef in CIor:
backup:
enabled: false
secretRef: postgres-backup-creds # k8s Secret holding AWS keysAlso applies to: 285-287, 325-327
🧰 Tools
🪛 Gitleaks (8.26.0)
240-240: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
241-241: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 240 to 242, replace the hard-coded
real-looking AWS access keys and restic password with clearly dummy placeholder
values or modify the manifest to reference Kubernetes Secrets instead. This
avoids committing sensitive credentials to version control and prevents security
scanner alerts. Apply the same changes to lines 285-287 and 325-327 as well.
Add 'Apps' tests for Virtual Machine Disk Virtual Machine Instance Virtual Machine PostgreSQL MySQL ClickHouse Signed-off-by: Ahmad Murzahmatov <gwynbleidd2106@yandex.com>
e5e73f3 to
75ad269
Compare
Add 'Apps' tests for
Virtual Machine Disk
Virtual Machine Instance
Virtual Machine
PostgreSQL
MySQL
ClickHouse
Summary by CodeRabbit
Summary by CodeRabbit