From db86c94911d4d19d636c65e8f858a72630392d55 Mon Sep 17 00:00:00 2001 From: Michal Dulko Date: Thu, 8 Feb 2024 13:43:01 +0100 Subject: [PATCH] [occm] Make sure we don't mask LB tests failures and fix what was failing (#2360) (#2537) * Compare proper LB name for shared LBs With shared LBs we distinguish the elements by tagging them with the proper name of the LB that would be created for a Service if it wasn't created as shared. This commit fixes that comparison for listener deletion as code was always comparing the name of the primary LB. * Fix shared LBs tests PR #2190 prohibited sharing an LB that is internal for security reasons. This commit fixes the shared LBs tests to not create internal LBs. * Make sure we don't mask LB tests failures In `test-lb-service.sh` we do `trap "delete_resources" EXIT` to make sure we cleanup resources on a test failure. In there, we only fetched the `$?` after making a check for `${AUTO_CLEAN_UP}`, which itself alters the code to 0, so function always returns success. This means tests can never really fail. This commit fixes it by making sure `$ERROR_CODE` is fetched at the very beginning of the cleanup function. --- pkg/openstack/loadbalancer.go | 3 ++- tests/e2e/cloudprovider/test-lb-service.sh | 30 ++++++++++++++++------ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 93e62a5194..4c6707695e 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -2469,7 +2469,7 @@ func (lbaas *LbaasV2) deleteLoadBalancer(loadbalancer *loadbalancers.LoadBalance Protocol: proto, Port: int(port.Port), }] - if isPresent && cpoutil.Contains(listener.Tags, loadbalancer.Name) { + if isPresent && cpoutil.Contains(listener.Tags, svcConf.lbName) { listenersToDelete = append(listenersToDelete, *listener) } } @@ -2530,6 +2530,7 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName if err := lbaas.checkServiceDelete(service, svcConf); err != nil { return err } + svcConf.lbName = lbName if svcConf.lbID != "" { loadbalancer, err = openstackutil.GetLoadbalancerByID(lbaas.lb, svcConf.lbID) diff --git a/tests/e2e/cloudprovider/test-lb-service.sh b/tests/e2e/cloudprovider/test-lb-service.sh index c168aa2d1b..6199709284 100755 --- a/tests/e2e/cloudprovider/test-lb-service.sh +++ b/tests/e2e/cloudprovider/test-lb-service.sh @@ -20,12 +20,12 @@ LB_SUBNET_NAME=${LB_SUBNET_NAME:-"private-subnet"} AUTO_CLEAN_UP=${AUTO_CLEAN_UP:-"true"} function delete_resources() { + ERROR_CODE="$?" + if [[ ${AUTO_CLEAN_UP} != "true" ]]; then exit ${ERROR_CODE} fi - ERROR_CODE="$?" - printf "\n>>>>>>> Deleting k8s services\n" kubectl -n ${NAMESPACE} get svc -o name | xargs -r kubectl -n $NAMESPACE delete printf "\n>>>>>>> Deleting k8s deployments\n" @@ -34,6 +34,12 @@ function delete_resources() { printf "\n>>>>>>> Deleting openstack load balancer \n" openstack loadbalancer delete test_shared_user_lb --cascade + printf "\n>>>>>>> Deleting openstack FIPs \n" + fips=$(openstack floating ip list --tag occm-test -f value -c ID) + for fip in $fips; do + openstack floating ip delete ${fip} + done + if [[ "$ERROR_CODE" != "0" ]]; then printf "\n>>>>>>> Dump openstack-cloud-controller-manager logs \n" pod_name=$(kubectl -n kube-system get pod -l k8s-app=openstack-cloud-controller-manager -o name | awk 'NR==1 {print}') @@ -438,7 +444,6 @@ metadata: name: ${service1} namespace: $NAMESPACE annotations: - service.beta.kubernetes.io/openstack-internal-load-balancer: "true" loadbalancer.openstack.org/enable-health-monitor: "false" spec: type: LoadBalancer @@ -474,7 +479,6 @@ metadata: name: ${service2} namespace: $NAMESPACE annotations: - service.beta.kubernetes.io/openstack-internal-load-balancer: "true" loadbalancer.openstack.org/enable-health-monitor: "false" loadbalancer.openstack.org/load-balancer-id: "$lbID" spec: @@ -521,7 +525,6 @@ metadata: name: ${service2} namespace: $NAMESPACE annotations: - service.beta.kubernetes.io/openstack-internal-load-balancer: "true" loadbalancer.openstack.org/enable-health-monitor: "false" loadbalancer.openstack.org/load-balancer-id: "$lbID" spec: @@ -580,7 +583,6 @@ metadata: name: ${service3} namespace: $NAMESPACE annotations: - service.beta.kubernetes.io/openstack-internal-load-balancer: "true" loadbalancer.openstack.org/enable-health-monitor: "false" loadbalancer.openstack.org/load-balancer-id: "$lbID" spec: @@ -614,7 +616,6 @@ metadata: name: ${service4} namespace: $NAMESPACE annotations: - service.beta.kubernetes.io/openstack-internal-load-balancer: "true" loadbalancer.openstack.org/enable-health-monitor: "false" loadbalancer.openstack.org/load-balancer-id: "$lbID" spec: @@ -725,6 +726,20 @@ function test_shared_user_lb { printf "\n>>>>>>> Waiting for openstack load balancer $lbID ACTIVE after creating listener \n" wait_for_loadbalancer $lbID + printf "\n>>>>>>> Getting an external network \n" + extNetID=$(openstack network list --external -f value -c ID | head -1) + if [[ -z extNetID ]]; then + printf "\n>>>>>>> FAIL: failed to find an external network\n" + exit 1 + fi + fip=$(openstack floating ip create --tag occm-test -f value -c id ${extNetID}) + if [ $? -ne 0 ]; then + printf "\n>>>>>>> FAIL: failed to create FIP\n" + exit 1 + fi + vip=$(openstack loadbalancer show $lbID -f value -c vip_port_id) + openstack floating ip set --port ${vip} ${fip} + local service1="test-shared-user-lb" printf "\n>>>>>>> Create Service ${service1}\n" cat <