-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
e2e: wait for cluster api providers #417
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
hack/e2e.sh (2)
Line range hint
281-286
: Incorrect usage of 'kubectl wait' with '--for=jsonpath='The
kubectl wait
command does not support the--for=jsonpath=
parameter. The--for
flag should specify conditions likecondition=Available
ordelete
. Using--for=jsonpath=
will result in an error.Consider replacing these commands with a loop that checks the desired condition using
kubectl get
. For example:-kubectl wait --timeout=5m --for=jsonpath=.status.readyReplicas=3 -n tenant-root sts etcd +timeout 300 sh -c 'until [ $(kubectl get sts etcd -n tenant-root -o jsonpath="{.status.readyReplicas}") -eq 3 ]; do sleep 5; done' -kubectl wait --timeout=5m --for=jsonpath=.status.updateStatus=operational -n tenant-root vmalert/vmalert-longterm vmalert/vmalert-shortterm vmalertmanager/alertmanager +timeout 300 sh -c 'until [ "$(kubectl get vmalert/vmalert-longterm -n tenant-root -o jsonpath="{.status.updateStatus}")" = "operational" ] && \ [ "$(kubectl get vmalert/vmalert-shortterm -n tenant-root -o jsonpath="{.status.updateStatus}")" = "operational" ] && \ [ "$(kubectl get vmalertmanager/alertmanager -n tenant-root -o jsonpath="{.status.updateStatus}")" = "operational" ]; do sleep 5; done' -kubectl wait --timeout=5m --for=jsonpath=.status.status=operational -n tenant-root vlogs/generic +timeout 300 sh -c 'until [ "$(kubectl get vlogs/generic -n tenant-root -o jsonpath="{.status.status}")" = "operational" ]; do sleep 5; done' -kubectl wait --timeout=5m --for=jsonpath=.status.clusterStatus=operational -n tenant-root vmcluster/shortterm vmcluster/longterm +timeout 300 sh -c 'until [ "$(kubectl get vmcluster/shortterm -n tenant-root -o jsonpath="{.status.clusterStatus}")" = "operational" ] && \ [ "$(kubectl get vmcluster/longterm -n tenant-root -o jsonpath="{.status.clusterStatus}")" = "operational" ]; do sleep 5; done'This change uses a loop with
timeout
andkubectl get
to repeatedly check the status until the desired condition is met or the timeout expires.
Line range hint
292-294
: Ensure proper validation of Grafana accessibilityUsing
grep Found
to check for Grafana's availability might not be reliable if the response does not contain the word "Found". Consider checking the HTTP status code instead to verify successful access.Apply this diff to improve the validation:
-ip=$(kubectl get svc -n tenant-root root-ingress-controller -o jsonpath='{.status.loadBalancer.ingress..ip}') - -# Check Grafana -curl -sS -k "https://$ip" -H 'Host: grafana.example.org' | grep Found +ip=$(kubectl get svc -n tenant-root root-ingress-controller -o jsonpath='{.status.loadBalancer.ingress..ip}') + +# Check Grafana +status_code=$(curl -s -o /dev/null -w "%{http_code}" -k "https://$ip" -H 'Host: grafana.example.org') +if [ "$status_code" -ne 200 ]; then + echo "Grafana is not accessible. HTTP status code: $status_code" + exit 1 +fiThis change checks the HTTP status code returned by the
curl
command, ensuring that Grafana is accessible and returns a200 OK
response.
# Wait for Cluster-API providers | ||
kubectl wait deploy --timeout=30s --for=condition=available -n cozy-cluster-api capi-controller-manager capi-kamaji-controller-manager capi-kubeadm-bootstrap-controller-manager capi-operator-cluster-api-operator capk-controller-manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider increasing the timeout for Cluster-API providers
The current timeout of 30 seconds may not be sufficient for all Cluster-API providers to become available, especially in environments with slower resources or network latency. Increasing the timeout ensures that the script waits long enough for the deployments to be ready.
Apply this diff to increase the timeout:
-kubectl wait deploy --timeout=30s --for=condition=available -n cozy-cluster-api capi-controller-manager capi-kamaji-controller-manager capi-kubeadm-bootstrap-controller-manager capi-operator-cluster-api-operator capk-controller-manager
+kubectl wait deploy --timeout=5m --for=condition=available -n cozy-cluster-api capi-controller-manager capi-kamaji-controller-manager capi-kubeadm-bootstrap-controller-manager capi-operator-cluster-api-operator capk-controller-manager
📝 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.
# Wait for Cluster-API providers | |
kubectl wait deploy --timeout=30s --for=condition=available -n cozy-cluster-api capi-controller-manager capi-kamaji-controller-manager capi-kubeadm-bootstrap-controller-manager capi-operator-cluster-api-operator capk-controller-manager | |
# Wait for Cluster-API providers | |
kubectl wait deploy --timeout=5m --for=condition=available -n cozy-cluster-api capi-controller-manager capi-kamaji-controller-manager capi-kubeadm-bootstrap-controller-manager capi-operator-cluster-api-operator capk-controller-manager |
Signed-off-by: Andrei Kvapil <[email protected]> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced orchestration of the Kubernetes environment with added wait commands for critical controllers and nodes. - Created and configured two new storage classes (`local` and `replicated`) for improved storage management. - Introduced MetalLB resources for effective load balancing within the cluster. - **Improvements** - Implemented checks to ensure all necessary components are online before proceeding with configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Andrei Kvapil <[email protected]>
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
local
andreplicated
) for improved storage management.Improvements