Copilot commented on code in PR #105: URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3903527639
########## hack/e2e/90-collect-artifacts.sh: ########## @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Collects debugging artifacts from the simulator, the kind cluster and the +# CloudStack API into _out/artifacts. Never fails. + +set -uo pipefail +source "$(dirname "${BASH_SOURCE[0]}")/env.sh" +source "${E2E_ROOT}/lib/log.sh" +source "${E2E_ROOT}/lib/cmk.sh" + +ART="${E2E_OUT}/artifacts" +mkdir -p "$ART" + +log "collecting artifacts into ${ART}" + +docker logs "$SIM_NAME" >"${ART}/simulator.log" 2>&1 + +# kubectl logs may stop working once the CCM rewrites node addresses, so fall +# back to reading container logs on the control-plane node directly. +if ! kubectl -n kube-system logs deployment/cloud-controller-manager --tail=-1 \ + >"${ART}/ccm.log" 2>&1; then + docker exec "${KIND_CLUSTER}-control-plane" bash -c \ + 'crictl ps -a --name cloud-controller-manager -q | head -1 | xargs -r crictl logs' \ + >"${ART}/ccm.log" 2>&1 +fi + +kubectl get nodes -o yaml >"${ART}/nodes.yaml" 2>&1 +kubectl get svc -A -o yaml >"${ART}/services.yaml" 2>&1 +kubectl describe svc -A >"${ART}/svc-describe.txt" 2>&1 +kubectl get events -A --sort-by=.lastTimestamp >"${ART}/events.txt" 2>&1 +kubectl -n kube-system get pods -o wide >"${ART}/kube-system-pods.txt" 2>&1 + +cmk_init +if cmk_ready; then + # Dump each resource twice: without a project (the isolated-network phase) + # and with projectid=-1, which for an admin spans all projects (the VPC + # phase). Otherwise the VPC phase's resources are invisible here. + for cmd in listLoadBalancerRules listPublicIpAddresses listFirewallRules \ + listNetworkACLs listVirtualMachines listNetworks; do + name="cs-$(echo "$cmd" | tr '[:upper:]' '[:lower:]')" + cmk -c "$CMK_CONFIG" "$cmd" listall=true | jq . >"${ART}/${name}.json" 2>&1 + cmk -c "$CMK_CONFIG" "$cmd" listall=true projectid=-1 | jq . >"${ART}/${name}-projects.json" 2>&1 + done +fi Review Comment: This script claims it "Never fails", but cmk_init calls die when cmk is not installed, exiting non-zero. That can cause the always-run artifact collection step to fail (and potentially mask the original failure). Make CloudStack API dumping conditional on cmk being present, and keep artifact collection best-effort. ########## test/e2e/framework.go: ########## @@ -0,0 +1,464 @@ +//go:build e2e + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Package e2e contains end-to-end tests that run against a live Kubernetes +// cluster whose cloud-controller-manager talks to a CloudStack management +// server (normally the simulator brought up by hack/e2e/up.sh). +// +// Configuration comes from the environment: +// +// KUBECONFIG kubeconfig of the cluster under test +// CS_API_URL CloudStack API endpoint (as reachable from the test process) +// CS_API_KEY CloudStack API key +// CS_SECRET_KEY CloudStack secret key +// CS_PROJECT_ID optional project scoping (set for the VPC phase) +// +// When any required variable is missing, the tests skip. +package e2e + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/blang/semver/v4" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" +) + +const ( + lbSyncTimeout = 3 * time.Minute + lbSyncInterval = 3 * time.Second +) + +// Framework bundles the clients and helpers shared by all e2e tests. +type Framework struct { + T *testing.T + K8s kubernetes.Interface + CS *cloudstack.CloudStackClient + Namespace string + ProjectID string + Version semver.Version +} + +// NewFramework builds clients from the environment, skipping the test when +// the environment is not configured. It creates a per-test namespace that is +// deleted on cleanup. +func NewFramework(t *testing.T) *Framework { + t.Helper() + + apiURL := os.Getenv("CS_API_URL") + apiKey := os.Getenv("CS_API_KEY") + secretKey := os.Getenv("CS_SECRET_KEY") + if apiURL == "" || apiKey == "" || secretKey == "" { + t.Skip("CS_API_URL/CS_API_KEY/CS_SECRET_KEY not set; skipping e2e test") + } + + kubeconfig := os.Getenv("KUBECONFIG") + if kubeconfig == "" { + t.Skip("KUBECONFIG not set; skipping e2e test") + } + restCfg, err := clientcmd.BuildConfigFromFlags("", kubeconfig) + if err != nil { + t.Fatalf("building kubeconfig: %v", err) + } + k8s, err := kubernetes.NewForConfig(restCfg) + if err != nil { + t.Fatalf("building kubernetes client: %v", err) + } + + verifySSL := true + if noVerify, err := strconv.ParseBool(os.Getenv("CS_SSL_NO_VERIFY")); err == nil { + verifySSL = !noVerify + } + cs := cloudstack.NewAsyncClient(apiURL, apiKey, secretKey, verifySSL) + + f := &Framework{ + T: t, + K8s: k8s, + CS: cs, + ProjectID: os.Getenv("CS_PROJECT_ID"), + } + f.Version = f.managementServerVersion() + f.Namespace = f.createNamespace() + return f +} + +func (f *Framework) managementServerVersion() semver.Version { + f.T.Helper() + resp, err := f.CS.Management.ListManagementServersMetrics( + f.CS.Management.NewListManagementServersMetricsParams()) + if err != nil { + f.T.Fatalf("listing management servers: %v", err) + } + if resp.Count == 0 { + f.T.Fatal("no management servers found") + } + raw := majorMinorPatch(resp.ManagementServersMetrics[0].Version) + v, err := semver.ParseTolerant(raw) + if err != nil { + f.T.Fatalf("parsing management server version %q: %v", raw, err) + } + return v +} + +// majorMinorPatch trims a CloudStack version such as "4.22.1.0" down to the +// first three components. Slicing blindly would panic on a version string with +// fewer than three, so the length is checked first. +func majorMinorPatch(version string) string { + parts := strings.Split(version, ".") + if len(parts) > 3 { + parts = parts[:3] + } + return strings.Join(parts, ".") +} + +func (f *Framework) createNamespace() string { + f.T.Helper() + buf := make([]byte, 4) + if _, err := rand.Read(buf); err != nil { + f.T.Fatalf("generating namespace suffix: %v", err) + } + name := "ccm-e2e-" + hex.EncodeToString(buf) + _, err := f.K8s.CoreV1().Namespaces().Create(context.Background(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}}, metav1.CreateOptions{}) + if err != nil { + f.T.Fatalf("creating namespace %s: %v", name, err) + } + f.T.Cleanup(func() { + err := f.K8s.CoreV1().Namespaces().Delete( + context.Background(), name, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + // Not fatal -- deletion is asynchronous and the namespace will be + // reaped anyway -- but silence here hides a cluster that has + // stopped accepting writes. + f.T.Logf("warning: deleting namespace %s: %v", name, err) + } + }) + return name +} + +// Eventually polls cond until it returns true or the timeout elapses. +func (f *Framework) Eventually(timeout, interval time.Duration, desc string, cond func() (bool, error)) { + f.T.Helper() + deadline := time.Now().Add(timeout) + var lastErr error + for time.Now().Before(deadline) { + ok, err := cond() + lastErr = err + if ok { + return + } + time.Sleep(interval) + } + if lastErr != nil { + f.T.Fatalf("timed out after %s waiting for %s; last error: %v", timeout, desc, lastErr) + } + f.T.Fatalf("timed out after %s waiting for %s; the condition was evaluated "+ + "without error but never became true", timeout, desc) +} + +// CreateLBService creates a LoadBalancer service in the test namespace and +// registers cleanup that deletes it and waits for the CloudStack rules to +// disappear, failing the test if they do not. Later tests share this +// simulator and its public IP pool, so a leaked rule has to be reported here +// rather than left to surface as an unrelated failure downstream. +func (f *Framework) CreateLBService(mutate func(*corev1.Service)) *corev1.Service { + f.T.Helper() + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "e2e", + Namespace: f.Namespace, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Selector: map[string]string{"app": "e2e"}, + Ports: []corev1.ServicePort{ + {Name: "http", Port: 80, Protocol: corev1.ProtocolTCP}, + }, + }, + } + if mutate != nil { + mutate(svc) + } + created, err := f.K8s.CoreV1().Services(f.Namespace).Create( + context.Background(), svc, metav1.CreateOptions{}) + if err != nil { + f.T.Fatalf("creating service: %v", err) + } + f.T.Cleanup(func() { f.DeleteServiceAndWait(created) }) + return created +} + +// DeleteServiceAndWait deletes the service (if it still exists) and waits for +// its CloudStack load balancer rules to be cleaned up. +func (f *Framework) DeleteServiceAndWait(svc *corev1.Service) { + f.T.Helper() + err := f.K8s.CoreV1().Services(svc.Namespace).Delete( + context.Background(), svc.Name, metav1.DeleteOptions{}) + if apierrors.IsNotFound(err) { + return + } + if err != nil { + // Anything other than NotFound means the service may still exist, so + // skipping the cleanup wait below would leak CloudStack rules into + // later tests sharing this simulator. + f.T.Fatalf("deleting service %s/%s: %v", svc.Namespace, svc.Name, err) + } Review Comment: DeleteServiceAndWait returns immediately when the Service is already NotFound, which skips the load-balancer rule cleanup wait. A deleted Service can still have CloudStack LB rules pending deletion; skipping the wait can leak rules into later tests and contradicts the function’s doc comment (“deletes … and waits for … rules to be cleaned up”). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
