[tests] Add requests and limits to tests - #1130
Conversation
WalkthroughAdds a withResources toggle and conditional resources blocks to multiple e2e tests. Reworks readiness checks to use kubectl wait with updated timeouts and ordering. Removes some bespoke polling and cleanup steps. Minor manifest generation changes across DBs, Redis, Kafka, and VM tests; adds an extra vmdisks wait. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as E2E Test
participant K as kubectl/K8s API
participant HR as HelmRelease
participant CR as Custom Resource (DB/VM/etc.)
participant WL as Workloads (STS/DEP/VM/VMI)
rect rgb(245,248,250)
note over T: Manifest generation
T->>T: Build manifest\n(withResources? resources : {})
end
T->>K: kubectl apply -f manifest
K-->>T: Accepted
rect rgb(240,250,240)
note over T,K: Readiness sequence (standardized)
T->>K: kubectl wait HR ready
K-->>T: Ready/timeout
T->>K: kubectl wait CR ready
K-->>T: Ready/timeout
T->>K: kubectl wait WL conditions\n(PVC bound, STS replicas,\nDeployment available, VM/VMI Running)
K-->>T: Ready/timeout
end
alt Cleanup
T->>K: kubectl delete CR
K-->>T: Deleted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
hack/e2e-apps/kubernetes.bats (1)
68-68: Prefer minute-based notation for readability and consistencyOther waits in this file use the
Xmstyle (4m,1m,10m). Switching one to a raw second count introduces cognitive friction. Consider expressing the new value as3m30s(kubectl accepts mixed-unit notation) to keep the script uniform and easier to scan.- kubectl wait tcp -n tenant-test kubernetes-test --timeout=210s --for=jsonpath='{.status.kubernetesResources.version.status}'=Ready + kubectl wait tcp -n tenant-test kubernetes-test --timeout=3m30s --for=jsonpath='{.status.kubernetesResources.version.status}'=ReadyDouble-check that your CI environment is running a kubectl ≥ 1.19; older versions don’t understand the
Xs/XmYsshorthand.hack/e2e-apps/redis.bats (1)
5-19: Make the resource toggle configurable instead of hardcoded.The
withResourcesvariable is hardcoded to'true', making the conditional logic effectively dead code. Consider making this configurable via an environment variable to allow tests to run with or without resource constraints.- withResources='true' + withResources="${WITH_RESOURCES:-true}"hack/e2e-apps/clickhouse.bats (1)
5-19: Fix indentation inconsistency and make resource toggle configurable.Two issues:
- The
withResourcesvariable is hardcoded to'true', making the else branch unreachable- The resource YAML block has inconsistent indentation (lines 8-14 have 2 spaces, but line 18 has 2 spaces as well, which should match)
- withResources='true' + withResources="${WITH_RESOURCES:-true}" if [ "$withResources" == 'true' ]; then resources=$(cat <<EOF - resources: - requests: - cpu: 500m - memory: 768Mi - limits: - cpu: "1000m" - memory: "1Gi" +resources: + requests: + cpu: 500m + memory: 768Mi + limits: + cpu: "1000m" + memory: "1Gi" EOF ) else - resources=' resources: {}' + resources='resources: {}' fihack/e2e-apps/kafka.bats (2)
5-19: Make the resource toggle configurable.The
withResourcesvariable is hardcoded to'true', making the conditional logic dead code. Consider making this configurable via environment variable like the other test scripts should be.- withResources='true' + withResources="${WITH_RESOURCES:-true}"
33-40: Consider different resource requirements for Kafka vs Zookeeper.Both Kafka and Zookeeper are using identical resource limits and requests. Consider whether they actually need the same resources, as Kafka typically requires more memory for message buffering while Zookeeper has different resource patterns.
hack/e2e-apps/mysql.bats (1)
5-19: Make the resource toggle configurable.The
withResourcesvariable is hardcoded to'true', making the conditional logic dead code, consistent with the same issue in other test scripts.- withResources='true' + withResources="${WITH_RESOURCES:-true}"hack/e2e-apps/postgres.bats (1)
5-19: Consider making the resource flag configurable.The
withResourcesvariable is hardcoded to'true', which makes the conditional logic unnecessary. Consider making this configurable via an environment variable to allow testing both scenarios.- withResources='true' + withResources=${WITH_RESOURCES:-'true'}This would allow running tests without resource limits by setting
WITH_RESOURCES=false.hack/e2e-apps/virtualmachine.bats (1)
5-5: Consider making the resource flag configurable.Same as the postgres test, the
withResourcesvariable is hardcoded, making the conditional logic unnecessary.- withResources='true' + withResources=${WITH_RESOURCES:-'true'}hack/cdi_golden_image_create.sh (1)
8-12: Consider adding validation for URL format.While the current validation checks for argument presence, consider adding basic URL format validation to catch common input errors early.
Add this validation after the existing checks:
if [ -z "$name" ] || [ -z "$url" ]; then echo "Usage: <name> <url>" echo "Example: 'ubuntu' 'https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img'" exit 1 fi + +# Basic URL validation +if [[ ! "$url" =~ ^https?:// ]]; then + echo "Error: URL must start with http:// or https://" + exit 1 +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
hack/cdi_golden_image_create.sh(1 hunks)hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(1 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)packages/apps/virtual-machine/templates/vm.yaml(2 hunks)packages/apps/vm-disk/templates/dv.yaml(1 hunks)packages/apps/vm-disk/values.yaml(1 hunks)packages/system/kubevirt-cdi/templates/cdi-cr.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (11)
hack/e2e-apps/clickhouse.bats (1)
56-56: Good addition of resource cleanup.Adding the deletion of the ClickHouse resource at the end ensures proper test cleanup.
hack/e2e-apps/kafka.bats (1)
58-58: Good fix using variable instead of hardcoded value.The kubectl wait command now correctly uses the
$namevariable instead of a hardcoded resource name, improving test flexibility.packages/system/kubevirt-cdi/templates/cdi-cr.yaml (2)
24-46: RBAC configuration looks correct but verify it aligns with PR objectives.The ClusterRole and RoleBinding configuration for DataVolume cloning appears correct, but this change seems unrelated to the stated PR objective of "adding resource limits to tests."
The RBAC permissions grant service accounts in the
cozy-publicnamespace the ability to createdatavolumes/sourceresources, which is appropriate for the golden image cloning functionality mentioned in the AI summary.Likely an incorrect or invalid review comment.
6-6: Assess and justifycloneStrategyOverrideusageThe
cloneStrategyOverride: copysetting forces host-assisted copy, which is the slowest and most resource-intensive cloning strategy. When possible, prefersnapshotorcsi-clonefor better performance and lower resource impact.• File: packages/system/kubevirt-cdi/templates/cdi-cr.yaml
Line 6:cloneStrategyOverride: copyBefore merging, please:
- Confirm that your CSI driver and storage class lack snapshot or csi-clone support, necessitating the
copyoverride.- Consider removing this override to let CDI default to snapshot-based cloning.
- Verify this change aligns with the PR’s goal of adding resource limits to tests; if not, split it into its own PR.
hack/e2e-apps/mysql.bats (1)
58-58: Good timeout adjustment for metrics deployment.Increasing the timeout from 90s to 130s for the metrics deployment wait is a reasonable adjustment for test stability.
hack/e2e-apps/postgres.bats (1)
59-59: Timeout increase looks appropriate.The HelmRelease timeout increase from 100s to 150s aligns with the addition of resource constraints, which may slow down pod scheduling and startup.
packages/apps/vm-disk/values.yaml (1)
3-7: Clear documentation improvement for golden image support.The updated documentation clearly explains the golden image functionality and provides a helpful example. This aligns well with the template changes that implement the golden image lookup logic.
packages/apps/virtual-machine/templates/vm.yaml (1)
42-61: No changes needed:lookupsafely handles missing namespacesThe Helm
lookupfunction returns an empty result when thecozy-publicnamespace (or the specified DataVolume) doesn’t exist, so the template will correctly fall back to the HTTP download. This pattern is already used elsewhere without issue.• packages/apps/virtual-machine/templates/vm.yaml (lines 42–61)
• packages/apps/vm-disk/templates/dv.yaml (lookup usage)hack/e2e-apps/vminstance.bats (1)
80-80: Timeout increase is appropriate for VM IP address allocation.Increasing the timeout from 20 to 40 seconds provides more reasonable time for VM instances to report their IP addresses, which can vary based on infrastructure and network conditions.
hack/cdi_golden_image_create.sh (2)
1-12: Well-structured script initialization and validation.The script properly handles error conditions with
set -e, validates required arguments, and provides clear usage instructions with an example. The parameter validation ensures the script fails early with helpful guidance.
14-32: DataVolume specification is correctly configured.The DataVolume manifest is properly structured with:
- Appropriate naming convention using "goldenimage-" prefix
- Correct namespace (
cozy-public) for shared golden images- CDI annotation for immediate storage binding
- Reasonable storage allocation (5Gi) with replicated storage class
| {{- $goldenImageName := .Values.source.image }} | ||
| {{- if $goldenImageName }} | ||
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | ||
| pvc: | ||
| name: goldenimage-{{ $goldenImageName }} | ||
| namespace: cozy-public |
There was a problem hiding this comment.
Unused lookup result variable.
The $dv variable stores the lookup result but is not used in the conditional logic. The code only checks if $goldenImageName exists, not if the DataVolume was actually found.
- {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }}
+ {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }}
+ {{- if $dv }}
pvc:
name: goldenimage-{{ $goldenImageName }}
namespace: cozy-public
+ {{- else }}
+ http:
+ url: {{ required "A valid .Values.source.image requires a golden image DataVolume or .Values.source.http.url!" "" }}
+ {{- end }}Or handle the case where golden image is specified but doesn't exist.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/apps/vm-disk/templates/dv.yaml around lines 23 to 28, the variable
$dv stores the result of the lookup for the DataVolume but is never used to
verify if the DataVolume actually exists. Update the conditional logic to check
both that $goldenImageName is set and that $dv is not empty or null, ensuring
the DataVolume exists before proceeding. Alternatively, add handling for the
case where $goldenImageName is specified but the DataVolume is not found, such
as displaying an error or skipping related configuration.
| withResources='' | ||
| if [ "$withResources" == 'true' ]; then | ||
| resources=$(cat <<EOF | ||
| resources: | ||
| requests: | ||
| cpu: 500m | ||
| memory: 768Mi | ||
| limits: | ||
| cpu: "1000m" | ||
| memory: "1Gi" | ||
| EOF | ||
| ) | ||
| else | ||
| resources='resources: {}' | ||
| fi |
There was a problem hiding this comment.
Resource specification is prepared but not used in the manifest.
The conditional logic correctly prepares resource requests and limits, but the $resources variable is not referenced in the VM disk creation manifest (lines 21-34). This means the resource constraints won't be applied even when withResources is set to 'true'.
Apply this diff to integrate the resources into the VM disk manifest:
kubectl create -f - <<EOF
apiVersion: apps.cozystack.io/v1alpha1
kind: VMDisk
metadata:
name: $name
namespace: tenant-test
spec:
+ $resources
source:
http:
url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img
optical: false
storage: 5Gi
storageClass: replicated
EOF📝 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.
| withResources='' | |
| if [ "$withResources" == 'true' ]; then | |
| resources=$(cat <<EOF | |
| resources: | |
| requests: | |
| cpu: 500m | |
| memory: 768Mi | |
| limits: | |
| cpu: "1000m" | |
| memory: "1Gi" | |
| EOF | |
| ) | |
| else | |
| resources='resources: {}' | |
| fi | |
| kubectl create -f - <<EOF | |
| apiVersion: apps.cozystack.io/v1alpha1 | |
| kind: VMDisk | |
| metadata: | |
| name: $name | |
| namespace: tenant-test | |
| spec: | |
| $resources | |
| source: | |
| http: | |
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | |
| optical: false | |
| storage: 5Gi | |
| storageClass: replicated | |
| EOF |
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 5 to 19, the resources variable is
correctly set based on the withResources flag but is not used in the VM disk
creation manifest between lines 21 and 34. To fix this, modify the manifest to
include the $resources variable so that the resource requests and limits are
applied when withResources is true. This involves inserting the $resources
content into the manifest YAML at the appropriate place where resource
specifications belong.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
hack/e2e-apps/kubernetes.bats (1)
68-68: Prefer consistent timeout notation (4minstead of210s)The surrounding
kubectl waitinvocations use theXmnotation (4m,1m,10m).
Switching this line to4mkeeps the file stylistically consistent and slightly extends the window ( +30 s ), giving the control-plane a bit more breathing room without changing intent.- kubectl wait tcp -n tenant-test kubernetes-test --timeout=210s --for=jsonpath='{.status.kubernetesResources.version.status}'=Ready + kubectl wait tcp -n tenant-test kubernetes-test --timeout=4m --for=jsonpath='{.status.kubernetesResources.version.status}'=Readyhack/cdi_golden_image_create.sh (1)
14-14: Minor: Consider improving the comment.The comment could be more descriptive about the script's purpose.
Apply this diff to improve the comment:
-#### create DV ubuntu source for CDI image cloning +#### Create DataVolume golden image source for CDI image cloning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
hack/cdi_golden_image_create.sh(1 hunks)hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(1 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)packages/apps/virtual-machine/templates/vm.yaml(2 hunks)packages/apps/vm-disk/templates/dv.yaml(1 hunks)packages/apps/vm-disk/values.yaml(1 hunks)packages/system/kubevirt-cdi/templates/cdi-cr.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (20)
hack/e2e-apps/redis.bats (2)
5-19: LGTM! Clean conditional resource specification logic.The implementation correctly adds conditional resource requests and limits. The heredoc formatting and variable substitution logic are appropriate for the Kubernetes manifest generation.
33-33: Good integration of dynamic resource specification.The variable substitution properly injects the conditional resources into the Redis manifest, maintaining consistency with the overall pattern.
hack/e2e-apps/kafka.bats (3)
5-19: Consistent implementation of conditional resources pattern.The logic matches the pattern established in other test files, providing standardized resource constraint handling across the test suite.
33-33: Good application of resources to both Kafka and Zookeeper specs.Properly applies the conditional resource specifications to both components, ensuring consistent resource constraints across the entire Kafka deployment.
Also applies to: 39-39
58-58: Improved kubectl wait command using variable name.The fix to use
$namevariable instead of a hardcoded value makes the command more flexible and consistent with the test structure.packages/system/kubevirt-cdi/templates/cdi-cr.yaml (2)
6-6: Appropriate clone strategy configuration.Setting
cloneStrategyOverride: copyenables the desired cloning behavior for DataVolumes, supporting the golden image functionality described in the PR objectives.
24-46: Well-scoped RBAC configuration for DataVolume cloning.The RBAC setup follows security best practices:
- ClusterRole grants minimal necessary permissions (
createondatavolumes/source)- RoleBinding is appropriately scoped to the
cozy-publicnamespace- Subjects target
system:serviceaccountswhich is standard for service-to-service operationsThis enables the required functionality while maintaining proper access controls.
hack/e2e-apps/postgres.bats (2)
5-19: Consistent conditional resource specification with proper indentation.The implementation follows the established pattern. The 2-space indentation in the resources heredoc is appropriate for the YAML structure where it will be injected.
59-59: Reasonable timeout increase for PostgreSQL initialization.The timeout increase to 150s is appropriate given that PostgreSQL deployments can take longer to become ready, especially with the additional initialization jobs and resource constraints.
hack/e2e-apps/mysql.bats (2)
5-19: Consistent implementation of conditional resource pattern.The conditional resource specification follows the same pattern as other test files, maintaining consistency across the test suite.
58-58: Appropriate timeout increase for metrics deployment.The timeout increase to 130s for the MySQL metrics deployment is reasonable given the additional resource constraints and initialization time required.
packages/apps/vm-disk/values.yaml (1)
3-7: LGTM! Clear documentation improvement.The updated comment and example clearly explain the new golden image functionality and provide a helpful usage example.
hack/e2e-apps/clickhouse.bats (3)
5-19: LGTM! Proper resource specification implementation.The conditional resource logic is well-structured with reasonable CPU (500m request, 1000m limit) and memory (768Mi request, 1Gi limit) specifications.
50-50: Good improvement: Increased timeout for better reliability.Increasing the timeout from 20s to 40s should improve test reliability for the ClickHouse readiness condition.
56-56: Good practice: Added cleanup step.Adding the cleanup step ensures proper resource management in the test environment.
packages/apps/virtual-machine/templates/vm.yaml (1)
42-61: LGTM! Correct golden image implementation.The logic properly checks for the existence of the DataVolume lookup result (
$dv) before referencing the PVC, with appropriate fallback to HTTP sources when the golden image doesn't exist.hack/e2e-apps/vminstance.bats (1)
80-80: LGTM: Timeout increase improves test reliability.Increasing the timeout from 20s to 40s for VM instance IP address availability is a sensible change that should reduce test flakiness.
hack/cdi_golden_image_create.sh (3)
1-3: LGTM: Proper script setup with error handling.The shebang and
set -eare correctly configured for robust script execution.
8-12: LGTM: Clear argument validation with helpful usage message.The argument validation properly checks for required parameters and provides a clear usage example.
15-32: LGTM: Well-structured DataVolume specification.The DataVolume resource is properly configured with:
- Appropriate naming convention (
goldenimage-$name)- Correct namespace (
cozy-public)- Proper HTTP source configuration
- Reasonable storage allocation (5Gi)
- Immediate binding annotation for performance
| {{- $goldenImageName := .Values.source.image }} | ||
| {{- if $goldenImageName }} | ||
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | ||
| pvc: | ||
| name: goldenimage-{{ $goldenImageName }} | ||
| namespace: cozy-public |
There was a problem hiding this comment.
Fix the golden image existence check logic.
The code performs a DataVolume lookup but incorrectly checks $goldenImageName instead of the lookup result $dv. This will attempt to reference a PVC even when the DataVolume doesn't exist.
Apply this diff to fix the logic:
{{- $goldenImageName := .Values.source.image }}
- {{- if $goldenImageName }}
{{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }}
+ {{- if $dv }}
pvc:
name: goldenimage-{{ $goldenImageName }}
namespace: cozy-public📝 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.
| {{- $goldenImageName := .Values.source.image }} | |
| {{- if $goldenImageName }} | |
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | |
| pvc: | |
| name: goldenimage-{{ $goldenImageName }} | |
| namespace: cozy-public | |
| {{- $goldenImageName := .Values.source.image }} | |
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | |
| {{- if $dv }} | |
| pvc: | |
| name: goldenimage-{{ $goldenImageName }} | |
| namespace: cozy-public |
🤖 Prompt for AI Agents
In packages/apps/vm-disk/templates/dv.yaml around lines 23 to 28, the condition
incorrectly checks the variable $goldenImageName instead of the lookup result
$dv to determine if the DataVolume exists. To fix this, change the if condition
to check if $dv is defined and not empty before referencing the PVC, ensuring
the PVC is only referenced when the DataVolume actually exists.
| withResources='' | ||
| if [ "$withResources" == 'true' ]; then | ||
| resources=$(cat <<EOF | ||
| resources: | ||
| requests: | ||
| cpu: 500m | ||
| memory: 768Mi | ||
| limits: | ||
| cpu: "1000m" | ||
| memory: "1Gi" | ||
| EOF | ||
| ) | ||
| else | ||
| resources='resources: {}' | ||
| fi |
There was a problem hiding this comment.
Critical Issue: Resource configuration is not applied to the VM disk manifest.
The conditional resource configuration logic is defined but never actually used in the VM disk creation YAML. The $resources variable is not referenced anywhere in the kubectl create command (lines 21-34), making this code effectively dead code.
Apply this diff to integrate the resource configuration into the VM disk manifest:
spec:
source:
http:
url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img
optical: false
storage: 5Gi
storageClass: replicated
+ $resources
EOFNote: Ensure the YAML indentation is correct when the resources are injected.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 5 to 19, the resource
configuration stored in the $resources variable is defined but never used in the
VM disk manifest YAML passed to kubectl create. To fix this, modify the VM disk
manifest YAML in the kubectl create command (lines 21-34) to include the
$resources variable at the appropriate indentation level so that the resource
requests and limits are applied correctly. Ensure the YAML indentation is
consistent and correct when injecting $resources into the manifest.
f413caa to
c2a6e4f
Compare
c2a6e4f to
969f8f3
Compare
969f8f3 to
272093c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hack/e2e-apps/kafka.bats (1)
5-19: Consider making the withResources flag configurable.The
withResourcesflag is hardcoded to'true', which means resource constraints will always be applied. Consider making this configurable via an environment variable to allow flexibility in different test environments.- withResources='true' + withResources="${WITH_RESOURCES:-true}"This would allow controlling resource application via the
WITH_RESOURCESenvironment variable while defaulting totrue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(3 hunks)hack/e2e-apps/vminstance.bats(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/clickhouse.bats
- hack/e2e-apps/kubernetes.bats
- hack/e2e-apps/redis.bats
- hack/e2e-apps/vminstance.bats
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/postgres.bats
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: pre-commit
🔇 Additional comments (4)
hack/e2e-apps/kafka.bats (4)
6-19: Resource specifications look appropriate for test environment.The CPU and memory requests/limits are well-suited for a test environment:
- Requests: 500m CPU, 768Mi memory
- Limits: 1000m CPU, 1Gi memory
The YAML formatting and conditional logic are correct.
20-20: Good practice: Check resource existence before creation.Adding the existence check prevents duplicate creation errors and makes the test more robust. The
||operator ensures that if thegetcommand fails (resource doesn't exist), the script proceeds to create the resource.
33-33: Resource injection implemented correctly.The
$resourcesvariable substitution properly replaces the previously static empty resource blocks in both Kafka and Zookeeper specifications. This allows the conditional resources to be applied consistently to both components.Also applies to: 39-39
57-60: Improved wait logic with appropriate timeouts.The replacement of manual polling loops with
kubectl waitcommands is a significant improvement:
- HelmRelease readiness: 30s timeout
- Kafka resource readiness: 1m timeout
- PVC bound status: 50s timeout
- Zookeeper service availability: 40s timeout
The timeouts are reasonable for the respective resources, and the JSONPath conditions are correctly specified.
272093c to
7685c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-apps/clickhouse.bats (1)
5-19: Make the resource flag configurable via environment variable.The resource configuration logic is well-implemented, but hardcoding
withResources='true'reduces flexibility. Consider making this configurable through an environment variable to allow different test scenarios.-withResources='true' +withResources=${WITH_RESOURCES:-'true'}This allows overriding the default behavior when needed for different test environments or CI/CD scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(3 hunks)hack/e2e-apps/vminstance.bats(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/kubernetes.bats
- hack/e2e-apps/vminstance.bats
- hack/e2e-apps/postgres.bats
🧰 Additional context used
🪛 Gitleaks (8.26.0)
hack/e2e-apps/clickhouse.bats
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
48-48: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
49-49: 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: pre-commit
- GitHub Check: Build
🔇 Additional comments (4)
hack/e2e-apps/clickhouse.bats (4)
46-46: LGTM!The resource variable integration is correctly implemented and maintains proper YAML structure.
50-52: LGTM! Improved wait logic with appropriate timeouts.The increased timeouts and additional wait conditions provide better visibility into the deployment process and account for potential delays when resource constraints are applied.
54-54: LGTM! Good test cleanup practice.Adding resource cleanup at the end of the test prevents resource accumulation and follows testing best practices.
43-45: Static analysis false positive: Test credentials are acceptable here.The static analysis tool flagged these as potential API keys, but these are clearly test/dummy credentials hardcoded in a test file. This is acceptable for testing purposes as they're not real secrets.
7685c97 to
45158e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-apps/vminstance.bats (1)
30-44: Consider making the resource flag configurable instead of hardcoded.The
withResourcesvariable is hardcoded to 'true', which means the test will always include resource specifications. This reduces testing flexibility and doesn't allow validation of both scenarios (with and without resources).Consider making this configurable via an environment variable:
- withResources='true' + withResources="${WITH_RESOURCES:-false}"This would allow the test to be run with or without resources based on the environment setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(3 hunks)hack/e2e-apps/vminstance.bats(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/clickhouse.bats
- hack/e2e-apps/kubernetes.bats
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/postgres.bats
- hack/e2e-apps/mysql.bats
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: pre-commit
🔇 Additional comments (2)
hack/e2e-apps/vminstance.bats (2)
21-24: Wait timeout adjustments look appropriate.The updated wait commands with explicit timeouts provide better control over test execution timing and should improve test reliability.
81-87: Wait timeout configurations are well-structured.The updated wait commands with appropriate timeouts for different resource types (hr, vminstances, vm, vmi) provide good coverage of the VM instance lifecycle and should improve test reliability.
f5b04a4 to
701fab7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/e2e-apps/clickhouse.bats (1)
50-50: Fix the kubectl wait command syntax.This command mixes
kubectl waitwith jsonpath comparison incorrectly, as noted in the previous review. Thekubectl waitcommand doesn't support waiting for specific jsonpath values.Use one of these corrected approaches:
-timeout 210 sh -ec "until kubectl -n tenant-test wait svc chendpoint-clickhouse-$name --for=jsonpath='{.spec.ports[0].port}'=8123; do sleep 10; done" +timeout 210 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[0].port}' | grep -q 8123; do sleep 10; done"Or wait for the service to be ready:
-timeout 210 sh -ec "until kubectl -n tenant-test wait svc chendpoint-clickhouse-$name --for=jsonpath='{.spec.ports[0].port}'=8123; do sleep 10; done" +kubectl -n tenant-test wait --timeout=210s svc chendpoint-clickhouse-$name --for=condition=ready
🧹 Nitpick comments (1)
hack/e2e-apps/clickhouse.bats (1)
5-5: Consider making the withResources flag configurable.The
withResourcesvariable is hardcoded to'true'. Consider making it configurable via environment variable to allow testing both with and without resource constraints.- withResources='true' + withResources=${WITH_RESOURCES:-'true'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(4 hunks)hack/e2e-apps/vminstance.bats(3 hunks)packages/core/installer/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/installer/values.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/redis.bats
- hack/e2e-apps/vminstance.bats
- hack/e2e-apps/postgres.bats
- hack/e2e-apps/kubernetes.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (2)
hack/e2e-apps/clickhouse.bats (2)
47-49: LGTM on the wait command improvements.The updated wait commands with increased timeouts and streamlined logic improve test reliability. The progression from HelmRelease → ClickHouse resource → StatefulSet readiness is logical.
51-51: LGTM on the cleanup command.The cleanup command correctly deletes the created ClickHouse resource, ensuring test isolation.
701fab7 to
6924e4d
Compare
6924e4d to
0f42ca3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-apps/vminstance.bats (1)
30-36: Consider clarifying the resource allocation logic.The current logic assigns smaller resources when
withResources='true'(1000m CPU, 1Gi memory) and larger resources when it's false (2000m CPU, 2Gi memory). This seems counterintuitive - typically you'd expectwithResources=trueto indicate more resources should be allocated, not fewer.Consider either:
- Reversing the logic if the intent is to provide more resources when the flag is true
- Renaming the flag to better reflect its purpose (e.g.,
withLimitedResources)- Adding a comment explaining the intended behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/kubernetes.bats
- hack/e2e-apps/redis.bats
- hack/e2e-apps/postgres.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/clickhouse.bats
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pre-commit
- GitHub Check: Build
🔇 Additional comments (3)
hack/e2e-apps/vminstance.bats (3)
20-23: LGTM: Wait commands properly updated with explicit timeouts.The wait commands have been improved with explicit resource types and appropriate timeouts, making the test more reliable and standardized.
45-50: LGTM: Resource values properly integrated into VM instance manifest.The dynamic resource values are correctly integrated into the VM instance manifest with proper YAML structure and variable substitution.
76-79: LGTM: Wait commands improved with standardized timeouts.The wait commands have been updated with explicit resource types and appropriate timeouts, improving test reliability and consistency with other test files in the PR.
0f42ca3 to
4821cc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hack/e2e-apps/vminstance.bats (1)
29-36: Fix critical syntax errors in memory variable assignmentsThere are missing closing quotes in the memory variable assignments that will cause the script to fail.
Apply this diff to fix the syntax errors:
if [ "$withResources" == 'true' ]; then cores="1000m" - memory="1Gi + memory="1Gi" else cores="2000m" - memory="2Gi + memory="2Gi" fi
🧹 Nitpick comments (1)
hack/e2e-apps/vminstance.bats (1)
29-29: Consider making the withResources flag configurableThe
withResourcesflag is hardcoded to 'true', which limits test flexibility. Consider making this configurable through an environment variable to allow testing both resource-constrained and unconstrained scenarios.- withResources='true' + withResources="${WITH_RESOURCES:-true}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(2 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/redis.bats
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/kubernetes.bats
- hack/e2e-apps/clickhouse.bats
- hack/e2e-apps/postgres.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pre-commit
🔇 Additional comments (3)
hack/e2e-apps/vminstance.bats (3)
20-23: LGTM: Improved wait logic with comprehensive resource readiness checksThe updated wait commands provide better test reliability by:
- Adding explicit wait for
vmdisksresource readiness- Using consistent and appropriate timeouts across resources
- Following the standardized pattern mentioned in the AI summary
45-50: LGTM: Resource configuration properly integrated into VM manifestThe conditional resource configuration is now correctly applied to the VM instance manifest with proper YAML structure and variable interpolation.
76-79: LGTM: Comprehensive wait strategy with appropriate timeoutsThe updated wait commands provide robust readiness checking:
- Sequential waits for different resource types (hr, vminstances, vm, vmi)
- Appropriate timeouts for each resource type
- Proper condition and jsonpath specifications
This follows the standardized pattern mentioned in the AI summary across other e2e test files.
c4b8bfa to
46624ff
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Just a note: we need to check resource quotas first, not a resources specification for applications.
14a5605 to
c8c8a0b
Compare
Signed-off-by: Ahmad Murzahmatov <gwynbleidd2106@yandex.com>
c8c8a0b to
47dd7d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
hack/e2e-apps/kubernetes.bats (3)
67-81: Improved waiting logic with proper timeouts.The refactored waiting logic using
kubectl waitcommands with explicit timeouts is a significant improvement over manual polling. The timeout values appear reasonable:
- 210s for TCP readiness (increased from 2m)
- 4m for control plane creation
- 10m for machine deployment readiness
However, consider adding error handling for wait command failures.
Consider adding error handling for wait commands:
- kubectl -n tenant-test wait --timeout=20s namespace tenant-test --for=jsonpath='{.status.phase}'=Active + kubectl -n tenant-test wait --timeout=20s namespace tenant-test --for=jsonpath='{.status.phase}'=Active || { echo "Namespace failed to become active"; exit 1; }
78-80: Verify kubeconfig extraction and usage.The script extracts the admin kubeconfig and uses it to wait for ingress-nginx and monitoring deployments. Ensure that the kubeconfig file is properly cleaned up after the test.
Consider adding cleanup for the kubeconfig file:
+ # Cleanup kubeconfig file + trap "rm -f admin.conf" EXIT
83-102: New PVC test adds valuable storage validation.The new test creates a PVC, waits for it to become bound, then cleans up properly. This adds good coverage for storage functionality in the tenant cluster.
Consider parameterizing the storage size and namespace:
+ storage_size='1Gi' + pvc_namespace='cozy-monitoring' KUBECONFIG=admin.conf kubectl apply -f - <<EOF apiVersion: v1 kind: PersistentVolumeClaim metadata: name: pvc-$name - namespace: cozy-monitoring + namespace: $pvc_namespace spec: accessModes: - ReadWriteOnce resources: requests: - storage: 1Gi + storage: $storage_size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/kubernetes.bats(3 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/postgres.bats(2 hunks)hack/e2e-apps/redis.bats(2 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/e2e-apps/mysql.bats
- hack/e2e-apps/virtualmachine.bats
- hack/e2e-apps/kafka.bats
- hack/e2e-apps/vminstance.bats
- hack/e2e-apps/clickhouse.bats
- hack/e2e-apps/redis.bats
- hack/e2e-apps/postgres.bats
⏰ 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)
hack/e2e-apps/kubernetes.bats (2)
4-4: Good parameterization improvement.The introduction of the
namevariable improves maintainability by eliminating hardcoded resource names throughout the test.
14-14: Verify that enabling these addons aligns with test requirements.The test now enables
certManagerandmonitoringAgents, and addshostsandexposeMethodconfiguration foringressNginx. These changes expand the test scope significantly but don't appear to add the resource requests and limits mentioned in the PR objectives.#!/bin/bash # Description: Check if there are other variants of this test file that include resource requests/limits # Expected: Find files with withResources flag or resource specifications # Search for withResources flag mentioned in the AI summary rg -A 5 -B 5 "withResources" # Search for resource requests/limits patterns in similar test files rg -A 3 -B 3 "resources:" hack/e2e-apps/ # Look for CPU/memory resource specifications in test files rg -A 5 "cpu:|memory:" hack/e2e-apps/Also applies to: 28-30, 33-33
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hack/e2e-apps/clickhouse.bats (1)
7-13: Fix nested resources: invalid YAML structure.You have
resources:underresources:; this will not parse as intended by the CRD.Apply:
resources=$(cat <<EOF resources: - resources: - cpu: 500m - memory: 768Mi + cpu: 500m + memory: 768Mi EOF )hack/e2e-apps/mysql.bats (1)
7-13: Fix nested resources: invalid YAML structure.Same double
resources:issue as in ClickHouse.resources=$(cat <<EOF resources: - resources: - cpu: 3000m - memory: 3Gi + cpu: 3000m + memory: 3Gi EOF )
🧹 Nitpick comments (8)
hack/e2e-apps/clickhouse.bats (4)
46-47: Be explicit with CRD resource in kubectl wait.Use the fully-qualified resource to avoid alias issues.
- kubectl -n tenant-test wait --timeout=130s clickhouses $name --for=condition=ready + kubectl -n tenant-test wait --timeout=130s clickhouses.apps.cozystack.io $name --for=condition=ready
48-48: Wait for readyReplicas, not replicas.
status.replicascan be non-zero before pods are ready.- kubectl -n tenant-test wait --timeout=120s sts chi-clickhouse-$name-clickhouse-0-0 --for=jsonpath='{.status.replicas}'=1 + kubectl -n tenant-test wait --timeout=120s sts chi-clickhouse-$name-clickhouse-0-0 --for=jsonpath='{.status.readyReplicas}'=1
49-55: Prefer kubectl wait over a manual polling loop.Unifies style and avoids subshell + timeout handling.
- timeout 210 sh -ec " - until [ \"\$(kubectl -n tenant-test get svc chendpoint-clickhouse-$name \ - -o jsonpath='{.spec.ports[?(@.port==8123)].port}')\" = \"8123\" ]; do - echo 'Waiting for ClickHouse service port 8123...' - sleep 10 - done - " + kubectl -n tenant-test wait --timeout=210s svc chendpoint-clickhouse-$name \ + --for=jsonpath='{.spec.ports[?(@.port==8123)].port}'=8123
3-6: Ensure cleanup runs on failure.Trap deletion so the CR is removed even if waits fail.
Add at the start of the test:
trap "kubectl -n tenant-test delete clickhouses.apps.cozystack.io $name --ignore-not-found=true" EXITWant me to push a commit adding this across all e2e apps?
hack/e2e-apps/mysql.bats (4)
52-52: Wait for ready replicas, not desired replicas.Improves correctness of StatefulSet readiness.
- kubectl -n tenant-test wait --timeout=110s sts mysql-$name --for=jsonpath='{.status.replicas}'=2 + kubectl -n tenant-test wait --timeout=110s sts mysql-$name --for=jsonpath='{.status.readyReplicas}'=2
53-54: Use deployment condition instead of sleep + replica count.Replace fixed sleep and replica count with availability condition.
- sleep 60 - kubectl -n tenant-test wait --timeout=60s deploy mysql-$name-metrics --for=jsonpath='{.status.replicas}'=1 + kubectl -n tenant-test wait --timeout=120s deploy mysql-$name-metrics --for=condition=available
55-55: Make service port check robust.Index 0 isn’t guaranteed to be the MySQL port.
- kubectl -n tenant-test wait --timeout=100s svc mysql-$name --for=jsonpath='{.spec.ports[0].port}'=3306 + kubectl -n tenant-test wait --timeout=100s svc mysql-$name --for=jsonpath='{.spec.ports[?(@.port==3306)].port}'=3306
3-6: Add cleanup trap for resilience.Ensure deletion runs on failure too.
At test start:
trap "kubectl -n tenant-test delete mysqls.apps.cozystack.io $name --ignore-not-found=true" EXIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hack/e2e-apps/clickhouse.bats(2 hunks)hack/e2e-apps/kafka.bats(3 hunks)hack/e2e-apps/mysql.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-apps/vminstance.bats
- hack/e2e-apps/kafka.bats
🔇 Additional comments (2)
hack/e2e-apps/clickhouse.bats (1)
42-42: LGTM: dynamic resources injection is correctly placed.hack/e2e-apps/mysql.bats (1)
49-51: LGTM: wait sequence for HR and CR is clear and specific.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/e2e-apps/clickhouse.bats (1)
51-54: Make port checks order-agnostic to avoid flakes.Array order for
.spec.ports[*].portisn’t guaranteed. Grepping for'8123 9000'or'9000 8123 9009'by sequence is brittle.Apply:
- timeout 180 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 180 sh -ec "until ports=\$(kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}'); echo \"\$ports\" | tr ' ' '\n' | grep -qx 8123 && echo \"\$ports\" | tr ' ' '\n' | grep -qx 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[*].port}' | grep -q '9000 8123 9009'; do sleep 10; done" + timeout 100 sh -ec "until ports=\$(kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[*].port}'); echo \"\$ports\" | tr ' ' '\n' | grep -qx 9000 && echo \"\$ports\" | tr ' ' '\n' | grep -qx 8123 && echo \"\$ports\" | tr ' ' '\n' | grep -qx 9009; do sleep 10; done"
♻️ Duplicate comments (1)
hack/e2e-apps/clickhouse.bats (1)
51-57: Fix delete: use the plural CRD resource name.Using the singular kind will fail deletion of the CR. Switch to the plural, fully-qualified resource.
Apply:
- kubectl -n tenant-test delete clickhouse $name + kubectl -n tenant-test delete clickhouses.apps.cozystack.io $name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/e2e-apps/clickhouse.bats(1 hunks)hack/e2e-apps/virtualmachine.bats(2 hunks)hack/e2e-apps/vminstance.bats(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/e2e-apps/virtualmachine.bats
⏰ 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 (1)
hack/e2e-apps/vminstance.bats (1)
23-23: LGTM: extra wait for VMDisk readiness.This reduces race conditions before VM creation.
| withResources='true' | ||
| if [ "$withResources" == 'true' ]; then | ||
| resources=$(cat <<EOF | ||
| resources: | ||
| resources: | ||
| cpu: 500m | ||
| memory: 768Mi | ||
| EOF | ||
| ) | ||
| else | ||
| resources=' resources: {}' | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resources block is malformed and not applied to the manifest.
- The nested
resources:underresources:is invalid YAML. $resourcesis never injected into the ClickHouse manifest, so the toggle has no effect.
Fix the block and inject it under spec (and drop/override the static resources: {} / resourcesPreset to avoid conflicts).
Apply this diff to fix the construction:
if [ "$withResources" == 'true' ]; then
resources=$(cat <<EOF
resources:
- resources:
- cpu: 500m
- memory: 768Mi
+ cpu: 500m
+ memory: 768Mi
EOF
)
else
resources=' resources: {}'
fiThen replace the static fields in the manifest with the injected block (snippet shows the intended placement/indent):
spec:
size: 10Gi
logStorageSize: 2Gi
shards: 1
replicas: 2
storageClass: ""
logTTL: 15
$resources
users:
testuser:
password: xai7Wepo
# Remove/avoid conflicting:
# resources: {}
# resourcesPreset: "nano"| withResources='true' | ||
| if [ "$withResources" == 'true' ]; then | ||
| cores="1000m" | ||
| memory="1Gi" | ||
| else | ||
| cores="2000m" | ||
| memory="2Gi" | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
CPU quantity assigned to domain.cpu.cores is likely invalid; route CPU to requests instead.
domain.cpu.cores generally expects an integer (cores count). Values like "1000m" are CPU quantities meant for resources.requests.cpu. This will likely be rejected or misinterpreted.
Apply:
- if [ "$withResources" == 'true' ]; then
- cores="1000m"
- memory="1Gi"
- else
- cores="2000m"
- memory="2Gi"
- fi
+ if [ "$withResources" == 'true' ]; then
+ cpu="1000m"
+ memory="1Gi"
+ else
+ cpu="2000m"
+ memory="2Gi"
+ fiAnd update the manifest to set CPU via requests (drop the domain.cores override):
-spec:
- domain:
- cpu:
- cores: "$cores"
- resources:
- requests:
- memory: "$memory"
+spec:
+ resources:
+ requests:
+ cpu: "$cpu"
+ memory: "$memory"Run to confirm schema expectations in-repo:
Also applies to: 45-50
🏁 Script executed:
#!/bin/bash
# Inspect examples/usages to validate expected fields
rg -nC3 -iP 'kind:\s*VMInstance' --type yaml
rg -nC2 -iP 'domain:\s*\n\s*cpu:\s*\n\s*cores:\s*\d+' --type yaml
rg -nC2 -iP 'resources:\s*\n\s*requests:\s*\n\s*cpu:\s*".*m"' --type yamlLength of output: 1241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show hack/e2e-apps/vminstance.bats (if present) ==="
if [ -f hack/e2e-apps/vminstance.bats ]; then
echo "FOUND: hack/e2e-apps/vminstance.bats"
nl -ba hack/e2e-apps/vminstance.bats | sed -n '1,200p'
else
echo "NOT FOUND: hack/e2e-apps/vminstance.bats"
fi
echo
echo "=== search for 'domain:' followed by 'cpu' and 'cores' in YAML files ==="
rg -n -U -P --hidden --no-ignore-vcs 'domain:\s*[\r\n]+\s*cpu:\s*[\r\n]+\s*cores' --type yaml -C3 || true
echo
echo "=== search for any 'domain:' blocks (context) in YAML ==="
rg -n --hidden --no-ignore-vcs -S --type yaml -C3 '\bdomain\b' || true
echo
echo "=== search for resources.requests.cpu in YAML ==="
rg -n -U -P --hidden --no-ignore-vcs 'resources:\s*[\r\n]+\s*requests:\s*[\r\n]+\s*cpu' --type yaml -C3 || true
echo
echo "=== search for 'cores:' occurrences in YAML ==="
rg -n --hidden --no-ignore-vcs --type yaml '\bcores:' -C3 || true
echo
echo "=== search for any 'cpu:' occurrences in YAML (broader) ==="
rg -n --hidden --no-ignore-vcs --type yaml '\bcpu:' -C3 || trueLength of output: 133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show hack/e2e-apps/vminstance.bats ==="
if [ -f hack/e2e-apps/vminstance.bats ]; then
awk '{printf "%4d: %s\n", NR, $0}' hack/e2e-apps/vminstance.bats
else
echo "NOT FOUND: hack/e2e-apps/vminstance.bats"
fi
echo
echo "=== search for 'cores' (identifiers) across repo ==="
rg -n --hidden --no-ignore-vcs '\bcores\b' -S || true
echo
echo "=== search for literal usage of the shell variable '$cores' in repo ==="
rg -n --hidden --no-ignore-vcs '\$cores' -S || true
echo
echo "=== locate VMInstance CRD/template and show surrounding lines ==="
rg -n --hidden --no-ignore-vcs -S 'kind:\s*VMInstance' || true
if [ -f packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml ]; then
sed -n '1,800p' packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml | sed -n '320,520p'
else
echo "NOT FOUND: packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml"
fi
echo
echo "=== search for 'domain:' blocks in YAML files ==="
rg -n --hidden --no-ignore-vcs -S --type yaml '\bdomain\b' -C3 || true
echo
echo "=== search for resources.requests.cpu occurrences in YAML ==="
rg -n -U --hidden --no-ignore-vcs 'resources:\s*[\r\n]+\s*requests:\s*[\r\n]+\s*cpu' --type yaml -C3 || trueLength of output: 18407
🏁 Script executed:
#!/bin/bash
set -e -o pipefail
echo "=== show hack/e2e-apps/vminstance.bats ==="
if [ -f hack/e2e-apps/vminstance.bats ]; then
cat -n hack/e2e-apps/vminstance.bats | sed -n '1,220p'
else
echo "NOT FOUND: hack/e2e-apps/vminstance.bats"
fi
echo
echo "=== locate and show VMInstance CRD/template (cozystack-resource-definitions.yaml) around 'kind: VMInstance' ==="
if [ -f packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml ]; then
rg -n --hidden --no-ignore-vcs -S 'kind:\s*VMInstance' packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml || true
sed -n '1,800p' packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml | sed -n '340,520p'
else
echo "NOT FOUND: packages/system/cozystack-api/templates/cozystack-resource-definitions.yaml"
fi
echo
echo "=== show packages/apps/vm-instance/templates/vm.yaml (to confirm how 'cores' is used) ==="
if [ -f packages/apps/vm-instance/templates/vm.yaml ]; then
sed -n '1,240p' packages/apps/vm-instance/templates/vm.yaml
else
echo "NOT FOUND: packages/apps/vm-instance/templates/vm.yaml"
fi
echo
echo "=== show packages/apps/vm-instance/values.schema.json (type info) ==="
if [ -f packages/apps/vm-instance/values.schema.json ]; then
sed -n '1,400p' packages/apps/vm-instance/values.schema.json
else
echo "NOT FOUND: packages/apps/vm-instance/values.schema.json"
fi
echo
echo "=== show README snippets mentioning resources.cpu / cores ==="
if [ -f packages/apps/vm-instance/README.md ]; then
rg -n --hidden --no-ignore-vcs -n 'resources.cpu|cores' packages/apps/vm-instance/README.md || true
sed -n '1,260p' packages/apps/vm-instance/README.md | sed -n '1,200p'
else
echo "NOT FOUND: packages/apps/vm-instance/README.md"
fi
echo
echo "=== search repo for YAMLs that set domain.cpu.cores or resources.requests.cpu ==="
rg -n --hidden --no-ignore-vcs -S --type yaml 'domain:\s*[\r\n]+\s*cpu:\s*[\r\n]+\s*cores' -C3 || true
rg -n -U --hidden --no-ignore-vcs 'resources:\s*[\r\n]+\s*requests:\s*[\r\n]+\s*cpu' --type yaml -C3 || trueLength of output: 51776
domain.cpu.cores must be an integer (vCPU count); don’t pass Kubernetes CPU quantities like "1000m".
hack/e2e-apps/vminstance.bats sets cores="1000m" and injects it into spec.domain.cpu.cores (lines 29–36; manifest lines 44–50) — same pattern in hack/e2e-apps/virtualmachine.bats. Change domain.cpu.cores to an integer (e.g. "1") and, if you need a millicpu request, put that value in spec.resources.requests.cpu (e.g. "1000m") instead.
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 29 to 36 (and similarly in
hack/e2e-apps/virtualmachine.bats), the test sets cores="1000m" which
incorrectly writes a Kubernetes CPU quantity into spec.domain.cpu.cores (which
must be an integer vCPU count); change cores to an integer string like "1" (or
"2") to represent vCPU count, and if you need to express millicpu values for
requests/limits, place "1000m" into spec.resources.requests.cpu (or
spec.resources.limits.cpu) instead of domain.cpu.cores; update any manifest
injection lines referenced (manifest lines ~44–50) to use the integer cores
value and move the millicpu value to the resource requests field.
| kubectl -n tenant-test wait --timeout=5s hr vm-instance-$name --for=condition=ready | ||
| kubectl -n tenant-test wait --timeout=130s vminstances $name --for=condition=ready | ||
| kubectl -n tenant-test wait --timeout=20s vm vm-instance-$name --for=condition=ready | ||
| kubectl -n tenant-test wait --timeout=40s vmi vm-instance-$name --for=jsonpath='{status.phase}'=Running |
There was a problem hiding this comment.
Fix jsonpath: missing leading dot.
kubectl wait --for=jsonpath='{.status.phase}'=Running requires the leading dot.
Apply:
- kubectl -n tenant-test wait --timeout=40s vmi vm-instance-$name --for=jsonpath='{status.phase}'=Running
+ kubectl -n tenant-test wait --timeout=40s vmi vm-instance-$name --for=jsonpath='{.status.phase}'=Running📝 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 --timeout=5s hr vm-instance-$name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=130s vminstances $name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=20s vm vm-instance-$name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=40s vmi vm-instance-$name --for=jsonpath='{status.phase}'=Running | |
| kubectl -n tenant-test wait --timeout=5s hr vm-instance-$name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=130s vminstances $name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=20s vm vm-instance-$name --for=condition=ready | |
| kubectl -n tenant-test wait --timeout=40s vmi vm-instance-$name --for=jsonpath='{.status.phase}'=Running |
🤖 Prompt for AI Agents
In hack/e2e-apps/vminstance.bats around lines 76 to 79, the kubectl wait using
--for=jsonpath is missing the required leading dot in the jsonpath expression;
update the last line to use --for=jsonpath='{.status.phase}'=Running (i.e., add
the leading dot before status) so the jsonpath is valid and the wait condition
evaluates correctly.
|
Already implemented. |
Add requests and limits for each test to run in limited state
Summary by CodeRabbit