Merge pull request #139 from shiftstack/merge-bot-master

Merge https://github.com/kubernetes/cloud-provider-openstack:master into master

Author: openshift-merge-robot

docs/developers-guide.md

@@ -58,6 +58,48 @@ If you are ready for code contribution, you need to have development environment
 1. IDE. You can choose any IDE that supports Golang, e.g. Visual Studio Code, GoLand, etc.
 1. Install [Docker Engine](https://docs.docker.com/engine/install/) or other tools that could build container images such as [podman](https://podman.io/). After writing the code, the best way to test openstack-cloud-controller-manager is to replace the container image specified in its Deployment/StatefulSet/DaemonSet manifest in the kubernetes cluster. We are using docker in this guide.
 
+### Testing
+
+*cloud-provider-openstack* includes a number of different types of test. As is usual for Kubernetes projects, jobs definitions live in the [test-infra](https://github.com/kubernetes/test-infra/tree/master/config/jobs/kubernetes/cloud-provider-openstack) project. A variety of tests run on each PR, however, you can also run your tests locally.
+
+### Unit tests
+
+Unit tests require a go environment with a suitable go version, as noted previously. Assuming you have this, you can run unit tests using the `unit` Makefile target.
+
+```
+make unit
+```
+
+### E2E tests
+
+End-to-end or _e2e_ tests are more complex to run as they require a functioning OpenStack cloud and Kubernetes (well, k3s) deployment. Fortunately, you can rely on the infrastructure used for CI to run this on your own machines.
+
+For example, to run the Cinder CSI e2e tests, the CI calls the `tests/ci-csi-cinder-e2e.sh` script. Inspecting this, you'll note that a lot of the commands in here are simply provisioning an instance on GCE, using [Boskos](https://github.com/kubernetes-sigs/boskos) to manage static resources (projects, in this case) if needed. If you have a set of GCE credentials, then in theory you could run this script as-is. However, all you need is a VM with sufficient resources and network connectivity running the correct image (Ubuntu 20.04 cloud image as of writing - check `tests/scripts/create-gce-vm.sh` for the latest info). For example, using OpenStack:
+
+```
+openstack server create \
+  --image ubuntu2004 \
+  --flavor m1.large \
+  --key-name <key-name> \
+  --network <network> \
+  --wait \
+  ${USER}-csi-cinder-e2e-tests
+openstack server add floating ip <server> <ip-address>
+```
+
+Once done, you can run the same Ansible commands seen in `tests/ci-csi-cinder-e2e.sh`:
+
+```
+ansible-playbook \
+  -v \
+  --user ubuntu \
+  --inventory 10.0.110.127, \
+  --ssh-common-args "-o StrictHostKeyChecking=no" \
+  tests/playbooks/test-csi-cinder-e2e.yaml
+```
+
+As you can see, this whole area needs improvement to make this a little more approachable for non-CI use cases. This should be enough direction to get started though.
+
 ### Build openstack-cloud-controller-manager image
 In cloud-provider-openstack repo directory, run:
 
@@ -96,7 +138,7 @@ IMAGE_NAMES=openstack-cloud-controller-manager \
 make upload-image-amd64
 ```
 
-Now, you can change openstack-cloud-controller-manager image in the kubernetes cluster, assuming the openstack-cloud-controller-manager is running as a DaemonSet:
+Now you can change openstack-cloud-controller-manager image in the kubernetes cluster, assuming the openstack-cloud-controller-manager is running as a DaemonSet:
 
 ```
 kubectl -n kube-system set image \

pkg/ingress/controller/controller.go

@@ -57,6 +57,7 @@ import (
 	"k8s.io/cloud-provider-openstack/pkg/ingress/config"
 	"k8s.io/cloud-provider-openstack/pkg/ingress/controller/openstack"
 	"k8s.io/cloud-provider-openstack/pkg/ingress/utils"
+	cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors"
 	openstackutil "k8s.io/cloud-provider-openstack/pkg/util/openstack"
 )
 
@@ -456,7 +457,7 @@ func (c *Controller) nodeSyncLoop() {
 		lbName := utils.GetResourceName(ing.Namespace, ing.Name, c.config.ClusterName)
 		loadbalancer, err := openstackutil.GetLoadbalancerByName(c.osClient.Octavia, lbName)
 		if err != nil {
-			if err != openstackutil.ErrNotFound {
+			if err != cpoerrors.ErrNotFound {
 				log.WithFields(log.Fields{"name": lbName}).Errorf("Failed to retrieve loadbalancer from OpenStack: %v", err)
 			}
 
@@ -564,7 +565,7 @@ func (c *Controller) deleteIngress(ing *nwv1.Ingress) error {
 	// If load balancer doesn't exist, assume it's already deleted.
 	loadbalancer, err := openstackutil.GetLoadbalancerByName(c.osClient.Octavia, lbName)
 	if err != nil {
-		if err != openstackutil.ErrNotFound {
+		if err != cpoerrors.ErrNotFound {
 			return fmt.Errorf("error getting loadbalancer %s: %v", ing.Name, err)
 		}
 

pkg/ingress/controller/openstack/octavia.go

@@ -287,7 +287,7 @@ func (os *OpenStack) EnsureLoadBalancer(name string, subnetID string, ingNamespa
 
 	loadbalancer, err := openstackutil.GetLoadbalancerByName(os.Octavia, name)
 	if err != nil {
-		if err != openstackutil.ErrNotFound {
+		if err != cpoerrors.ErrNotFound {
 			return nil, fmt.Errorf("error getting loadbalancer %s: %v", name, err)
 		}
 
@@ -339,7 +339,7 @@ func (os *OpenStack) UpdateLoadBalancerDescription(lbID string, newDescription s
 func (os *OpenStack) EnsureListener(name string, lbID string, secretRefs []string, listenerAllowedCIDRs []string) (*listeners.Listener, error) {
 	listener, err := openstackutil.GetListenerByName(os.Octavia, name, lbID)
 	if err != nil {
-		if err != openstackutil.ErrNotFound {
+		if err != cpoerrors.ErrNotFound {
 			return nil, fmt.Errorf("error getting listener %s: %v", name, err)
 		}
 
@@ -394,7 +394,7 @@ func (os *OpenStack) EnsurePoolMembers(deleted bool, poolName string, lbID strin
 	if deleted {
 		pool, err := openstackutil.GetPoolByName(os.Octavia, poolName, lbID)
 		if err != nil {
-			if err != openstackutil.ErrNotFound {
+			if err != cpoerrors.ErrNotFound {
 				return nil, fmt.Errorf("error getting pool %s: %v", poolName, err)
 			}
 			return nil, nil
@@ -416,7 +416,7 @@ func (os *OpenStack) EnsurePoolMembers(deleted bool, poolName string, lbID strin
 
 	pool, err := openstackutil.GetPoolByName(os.Octavia, poolName, lbID)
 	if err != nil {
-		if err != openstackutil.ErrNotFound {
+		if err != cpoerrors.ErrNotFound {
 			return nil, fmt.Errorf("error getting pool %s: %v", poolName, err)
 		}
 

pkg/openstack/loadbalancer.go

@@ -984,7 +984,7 @@ func (lbaas *LbaasV2) deleteListeners(lbID string, listenerList []listeners.List
 		klog.InfoS("Deleting listener", "listenerID", listener.ID, "lbID", lbID)
 
 		pool, err := openstackutil.GetPoolByListener(lbaas.lb, lbID, listener.ID)
-		if err != nil && err != openstackutil.ErrNotFound {
+		if err != nil && err != cpoerrors.ErrNotFound {
 			return fmt.Errorf("error getting pool for obsolete listener %s: %v", listener.ID, err)
 		}
 		if pool != nil {
@@ -1013,7 +1013,7 @@ func (lbaas *LbaasV2) deleteOctaviaListeners(lbID string, listenerList []listene
 			klog.InfoS("Deleting listener", "listenerID", listener.ID, "lbID", lbID)
 
 			pool, err := openstackutil.GetPoolByListener(lbaas.lb, lbID, listener.ID)
-			if err != nil && err != openstackutil.ErrNotFound {
+			if err != nil && err != cpoerrors.ErrNotFound {
 				return fmt.Errorf("error getting pool for listener %s: %v", listener.ID, err)
 			}
 			if pool != nil {
@@ -1243,7 +1243,7 @@ func (lbaas *LbaasV2) buildMonitorCreateOpts(svcConf *serviceConfig, port corev1
 // Make sure the pool is created for the Service, nodes are added as pool members.
 func (lbaas *LbaasV2) ensureOctaviaPool(lbID string, name string, listener *listeners.Listener, service *corev1.Service, port corev1.ServicePort, nodes []*corev1.Node, svcConf *serviceConfig) (*v2pools.Pool, error) {
 	pool, err := openstackutil.GetPoolByListener(lbaas.lb, lbID, listener.ID)
-	if err != nil && err != openstackutil.ErrNotFound {
+	if err != nil && err != cpoerrors.ErrNotFound {
 		return nil, fmt.Errorf("error getting pool for listener %s: %v", listener.ID, err)
 	}
 
@@ -1285,7 +1285,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(lbID string, name string, listener *list
 		klog.Errorf("failed to get members in the pool %s: %v", pool.ID, err)
 	}
 	for _, m := range poolMembers {
-		curMembers.Insert(fmt.Sprintf("%s-%d-%d", m.Address, m.ProtocolPort, m.MonitorPort))
+		curMembers.Insert(fmt.Sprintf("%s-%s-%d-%d", m.Name, m.Address, m.ProtocolPort, m.MonitorPort))
 	}
 
 	members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(port, nodes, svcConf)
@@ -1364,7 +1364,7 @@ func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes
 			member.MonitorPort = &svcConf.healthCheckNodePort
 		}
 		members = append(members, member)
-		newMembers.Insert(fmt.Sprintf("%s-%d-%d", addr, member.ProtocolPort, svcConf.healthCheckNodePort))
+		newMembers.Insert(fmt.Sprintf("%s-%s-%d-%d", node.Name, addr, member.ProtocolPort, svcConf.healthCheckNodePort))
 	}
 	return members, newMembers, nil
 }
@@ -2344,7 +2344,7 @@ func (lbaas *LbaasV2) ensureLoadBalancer(ctx context.Context, clusterName string
 		oldListeners = popListener(oldListeners, listener.ID)
 
 		pool, err := openstackutil.GetPoolByListener(lbaas.lb, loadbalancer.ID, listener.ID)
-		if err != nil && err != openstackutil.ErrNotFound {
+		if err != nil && err != cpoerrors.ErrNotFound {
 			return nil, fmt.Errorf("error getting pool for listener %s: %v", listener.ID, err)
 		}
 		if pool == nil {
@@ -2460,7 +2460,7 @@ func (lbaas *LbaasV2) ensureLoadBalancer(ctx context.Context, clusterName string
 		klog.V(4).Infof("Deleting obsolete listener %s:", listener.ID)
 		// get pool for listener
 		pool, err := openstackutil.GetPoolByListener(lbaas.lb, loadbalancer.ID, listener.ID)
-		if err != nil && err != openstackutil.ErrNotFound {
+		if err != nil && err != cpoerrors.ErrNotFound {
 			return nil, fmt.Errorf("error getting pool for obsolete listener %s: %v", listener.ID, err)
 		}
 		if pool != nil {
@@ -3366,7 +3366,7 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeletedLegacy(loadbalancer *loadbalancer
 	var monitorIDs []string
 	for _, listener := range listenerList {
 		pool, err := openstackutil.GetPoolByListener(lbaas.lb, loadbalancer.ID, listener.ID)
-		if err != nil && err != openstackutil.ErrNotFound {
+		if err != nil && err != cpoerrors.ErrNotFound {
 			return fmt.Errorf("error getting pool for listener %s: %v", listener.ID, err)
 		}
 		if pool != nil {
@@ -3416,7 +3416,7 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName
 		// This may happen when this Service creation was failed previously.
 		loadbalancer, err = getLoadbalancerByName(lbaas.lb, lbName, legacyName)
 	}
-	if err != nil && err != cpoerrors.ErrNotFound {
+	if err != nil && !cpoerrors.IsNotFound(err) {
 		return err
 	}
 	if loadbalancer == nil {
@@ -3522,7 +3522,7 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName
 		var monitorIDs []string
 		for _, listener := range listenerList {
 			pool, err := openstackutil.GetPoolByListener(lbaas.lb, loadbalancer.ID, listener.ID)
-			if err != nil && err != openstackutil.ErrNotFound {
+			if err != nil && err != cpoerrors.ErrNotFound {
 				return fmt.Errorf("error getting pool for listener %s: %v", listener.ID, err)
 			}
 			if pool != nil {

pkg/util/errors/errors.go

@@ -40,6 +40,10 @@ var ErrIPv6SupportDisabled = errors.New("IPv6 support is disabled")
 var ErrNoRouterID = errors.New("router-id not set in cloud provider config")
 
 func IsNotFound(err error) bool {
+	if err == ErrNotFound {
+		return true
+	}
+
 	if _, ok := err.(gophercloud.ErrDefault404); ok {
 		return true
 	}
@@ -54,10 +58,6 @@ func IsNotFound(err error) bool {
 		}
 	}
 
-	if err == ErrNotFound {
-		return true
-	}
-
 	return false
 }
 

pkg/util/openstack/keymanager.go

@@ -23,13 +23,14 @@ import (
 	"github.com/gophercloud/gophercloud"
 	"github.com/gophercloud/gophercloud/openstack/keymanager/v1/secrets"
 	"k8s.io/cloud-provider-openstack/pkg/metrics"
+	cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors"
 )
 
 // EnsureSecret creates a secret if it doesn't exist.
 func EnsureSecret(client *gophercloud.ServiceClient, name string, secretType string, payload string) (string, error) {
 	secret, err := GetSecret(client, name)
 	if err != nil {
-		if err == ErrNotFound {
+		if err == cpoerrors.ErrNotFound {
 			// Create a new one
 			return CreateSecret(client, name, secretType, payload)
 		}
@@ -56,10 +57,10 @@ func GetSecret(client *gophercloud.ServiceClient, name string) (*secrets.Secret,
 	}
 
 	if len(allSecrets) == 0 {
-		return nil, ErrNotFound
+		return nil, cpoerrors.ErrNotFound
 	}
 	if len(allSecrets) > 1 {
-		return nil, ErrMultipleResults
+		return nil, cpoerrors.ErrMultipleResults
 	}
 
 	return &allSecrets[0], nil

pkg/util/openstack/loadbalancer.go

@@ -17,7 +17,6 @@ limitations under the License.
 package openstack
 
 import (
-	"errors"
 	"fmt"
 	"time"
 
@@ -56,11 +55,15 @@ const (
 var (
 	octaviaVersion string
 
-	// ErrNotFound is used to inform that the object is missing
-	ErrNotFound = errors.New("failed to find object")
+	// ErrNotFound is used to inform that the object is missing.
+	// Deprecated: use cpoerrors.ErrNotFound instead.
+	// TODO: remove in v1.27.0.
+	ErrNotFound = cpoerrors.ErrNotFound
 
-	// ErrMultipleResults is used when we unexpectedly get back multiple results
-	ErrMultipleResults = errors.New("multiple results where only one expected")
+	// ErrMultipleResults is used when we unexpectedly get back multiple results.
+	// Deprecated: use cpoerrors.ErrMultipleResults instead.
+	// TODO: remove in v1.27.0.
+	ErrMultipleResults = cpoerrors.ErrMultipleResults
 )
 
 // getOctaviaVersion returns the current Octavia API version.
@@ -221,10 +224,10 @@ func GetLoadbalancerByName(client *gophercloud.ServiceClient, name string) (*loa
 	}
 
 	if len(loadbalancerList) > 1 {
-		return nil, ErrMultipleResults
+		return nil, cpoerrors.ErrMultipleResults
 	}
 	if len(loadbalancerList) == 0 {
-		return nil, ErrNotFound
+		return nil, cpoerrors.ErrNotFound
 	}
 
 	return &loadbalancerList[0], nil
@@ -364,19 +367,19 @@ func GetListenerByName(client *gophercloud.ServiceClient, name string, lbID stri
 		}
 		listenerList = append(listenerList, v...)
 		if len(listenerList) > 1 {
-			return false, ErrMultipleResults
+			return false, cpoerrors.ErrMultipleResults
 		}
 		return true, nil
 	})
 	if mc.ObserveRequest(err) != nil {
 		if cpoerrors.IsNotFound(err) {
-			return nil, ErrNotFound
+			return nil, cpoerrors.ErrNotFound
 		}
 		return nil, err
 	}
 
 	if len(listenerList) == 0 {
-		return nil, ErrNotFound
+		return nil, cpoerrors.ErrNotFound
 	}
 
 	return &listenerList[0], nil
@@ -430,21 +433,21 @@ func GetPoolByName(client *gophercloud.ServiceClient, name string, lbID string)
 		}
 		listenerPools = append(listenerPools, v...)
 		if len(listenerPools) > 1 {
-			return false, ErrMultipleResults
+			return false, cpoerrors.ErrMultipleResults
 		}
 		return true, nil
 	})
 	if mc.ObserveRequest(err) != nil {
 		if cpoerrors.IsNotFound(err) {
-			return nil, ErrNotFound
+			return nil, cpoerrors.ErrNotFound
 		}
 		return nil, err
 	}
 
 	if len(listenerPools) == 0 {
-		return nil, ErrNotFound
+		return nil, cpoerrors.ErrNotFound
 	} else if len(listenerPools) > 1 {
-		return nil, ErrMultipleResults
+		return nil, cpoerrors.ErrMultipleResults
 	}
 
 	return &listenerPools[0], nil
@@ -468,19 +471,19 @@ func GetPoolByListener(client *gophercloud.ServiceClient, lbID, listenerID strin
 			}
 		}
 		if len(listenerPools) > 1 {
-			return false, ErrMultipleResults
+			return false, cpoerrors.ErrMultipleResults
 		}
 		return true, nil
 	})
 	if mc.ObserveRequest(err) != nil {
 		if cpoerrors.IsNotFound(err) {
-			return nil, ErrNotFound
+			return nil, cpoerrors.ErrNotFound
 		}
 		return nil, err
 	}
 
 	if len(listenerPools) == 0 {
-		return nil, ErrNotFound
+		return nil, cpoerrors.ErrNotFound
 	}
 
 	return &listenerPools[0], nil

tests/playbooks/roles/install-csi-cinder/tasks/main.yaml

@@ -158,7 +158,7 @@
         -storage.testdriver=tests/e2e/csi/cinder/test-driver.yaml \
         -ginkgo.focus='External.Storage' \
         -ginkgo.skip='\[Disruptive\]|\[Testpattern:\s+Dynamic\s+PV\s+\(default\s+fs\)\]\s+provisioning\s+should\s+mount\s+multiple\s+PV\s+pointing\s+to\s+the\s+same\s+storage\s+on\s+the\s+same\s+node|\[Testpattern:\s+Dynamic\s+PV\s+\(default\s+fs\)\]\s+provisioning\s+should\s+provision\s+storage\s+with\s+any\s+volume\s+data\s+source\s+\[Serial\]' \
-        -ginkgo.noColor \
+        -ginkgo.no-color \
         -ginkgo.progress \
         -ginkgo.v \
         -test.timeout=0 \
@@ -172,7 +172,7 @@
     cmd: |
       set -x
       set -e
-      
+
       kubectl logs deployment/csi-cinder-controllerplugin -n kube-system -c cinder-csi-plugin > /var/log/csi-pod/csi-cinder-controllerplugin.log
       kubectl logs daemonset/csi-cinder-nodeplugin -n kube-system -c cinder-csi-plugin > /var/log/csi-pod/csi-cinder-nodeplugin.log
   ignore_errors: true