Merge pull request #569 from ricardomaraschini/bz-1846263

Bug 1846263: Avoiding bootstrap failure on unavailable swift
openshift · Jul 9, 2020 · 8e6f79d · 8e6f79d
2 parents e752804 + e1b3a84
commit 8e6f79d
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 110 deletions.
diff --git a/pkg/operator/bootstrap.go b/pkg/operator/bootstrap.go
@@ -12,15 +12,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog"
 
-	configapiv1 "github.com/openshift/api/config/v1"
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
 	operatorapi "github.com/openshift/api/operator/v1"
 
 	"github.com/openshift/cluster-image-registry-operator/pkg/defaults"
 	"github.com/openshift/cluster-image-registry-operator/pkg/storage"
 	"github.com/openshift/cluster-image-registry-operator/pkg/storage/pvc"
-	"github.com/openshift/cluster-image-registry-operator/pkg/storage/swift"
-	"github.com/openshift/cluster-image-registry-operator/pkg/storage/util"
 )
 
 // randomSecretSize is the number of random bytes to generate
@@ -36,14 +33,12 @@ func (c *Controller) Bootstrap() error {
 		return fmt.Errorf("unable to get the registry custom resources: %s", err)
 	}
 
-	// If the registry resource already exists,
-	// no bootstrapping is required
+	// If the registry resource already exists, no bootstrapping is required
 	if cr != nil {
 		return nil
 	}
 
-	// If no registry resource exists,
-	// let's create one with sane defaults
+	// If no registry resource exists, let's create one with sane defaults
 	klog.Infof("generating registry custom resource")
 
 	var secretBytes [randomSecretSize]byte
@@ -67,30 +62,12 @@ func (c *Controller) Bootstrap() error {
 		mgmtState = operatorapi.Removed
 	}
 
-	infra, err := util.GetInfrastructure(c.listers)
-	if err != nil {
-		return err
-	}
-
 	rolloutStrategy := appsapi.RollingUpdateDeploymentStrategyType
-
-	// If Swift service is not available for OpenStack, we have to start using
-	// Cinder with RWO PVC backend. It means that we need to create an RWO claim
-	// and set the rollout strategy to Recreate.
-	switch infra.Status.PlatformStatus.Type {
-	case configapiv1.OpenStackPlatformType:
-		isSwiftEnabled, err := swift.IsSwiftEnabled(c.listers)
-		if err != nil {
+	if platformStorage.PVC != nil {
+		if err = c.createPVC(corev1.ReadWriteOnce, platformStorage.PVC.Claim); err != nil {
 			return err
 		}
-		if !isSwiftEnabled {
-			err = c.createPVC(corev1.ReadWriteOnce)
-			if err != nil {
-				return err
-			}
-
-			rolloutStrategy = appsapi.RecreateDeploymentStrategyType
-		}
+		rolloutStrategy = appsapi.RecreateDeploymentStrategyType
 	}
 
 	cr = &imageregistryv1.Config{
@@ -118,49 +95,42 @@ func (c *Controller) Bootstrap() error {
 	return nil
 }
 
-func (c *Controller) createPVC(accessMode corev1.PersistentVolumeAccessMode) error {
-	claimName := defaults.PVCImageRegistryName
-
+func (c *Controller) createPVC(accessMode corev1.PersistentVolumeAccessMode, claimName string) error {
 	// Check that the claim does not exist before creating it
-	_, err := c.clients.Core.PersistentVolumeClaims(defaults.ImageRegistryOperatorNamespace).Get(
+	if _, err := c.clients.Core.PersistentVolumeClaims(defaults.ImageRegistryOperatorNamespace).Get(
 		context.TODO(), claimName, metav1.GetOptions{},
-	)
-	if err != nil {
-		if !errors.IsNotFound(err) {
-			return err
-		}
+	); err == nil {
+		return nil
+	} else if !errors.IsNotFound(err) {
+		return err
+	}
 
-		// "standard" is the default StorageClass name, that was provisioned by the cloud provider
-		storageClassName := "standard"
+	// "standard" is the default StorageClass name, that was provisioned by the cloud provider
+	storageClassName := "standard"
 
-		claim := &corev1.PersistentVolumeClaim{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      claimName,
-				Namespace: defaults.ImageRegistryOperatorNamespace,
-				Annotations: map[string]string{
-					pvc.PVCOwnerAnnotation: "true",
-				},
+	claim := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      claimName,
+			Namespace: defaults.ImageRegistryOperatorNamespace,
+			Annotations: map[string]string{
+				pvc.PVCOwnerAnnotation: "true",
 			},
-			Spec: corev1.PersistentVolumeClaimSpec{
-				AccessModes: []corev1.PersistentVolumeAccessMode{
-					accessMode,
-				},
-				Resources: corev1.ResourceRequirements{
-					Requests: corev1.ResourceList{
-						corev1.ResourceStorage: resource.MustParse("100Gi"),
-					},
+		},
+		Spec: corev1.PersistentVolumeClaimSpec{
+			AccessModes: []corev1.PersistentVolumeAccessMode{
+				accessMode,
+			},
+			Resources: corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					corev1.ResourceStorage: resource.MustParse("100Gi"),
 				},
-				StorageClassName: &storageClassName,
 			},
-		}
-
-		_, err = c.clients.Core.PersistentVolumeClaims(defaults.ImageRegistryOperatorNamespace).Create(
-			context.TODO(), claim, metav1.CreateOptions{},
-		)
-		if err != nil {
-			return err
-		}
+			StorageClassName: &storageClassName,
+		},
 	}
 
-	return nil
+	_, err := c.clients.Core.PersistentVolumeClaims(defaults.ImageRegistryOperatorNamespace).Create(
+		context.TODO(), claim, metav1.CreateOptions{},
+	)
+	return err
 }
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
@@ -150,20 +150,15 @@ func GetPlatformStorage(listers *regopclient.Listers) (imageregistryv1.ImageRegi
 		cfg.GCS = &imageregistryv1.ImageRegistryConfigStorageGCS{}
 		replicas = 2
 	case configapiv1.OpenStackPlatformType:
-		isSwiftEnabled, err := swift.IsSwiftEnabled(listers)
-		if err != nil {
-			return imageregistryv1.ImageRegistryConfigStorage{}, replicas, err
-		}
-		if !isSwiftEnabled {
-			cfg.PVC = &imageregistryv1.ImageRegistryConfigStoragePVC{
-				Claim: defaults.PVCImageRegistryName,
-			}
-			replicas = 1
-		} else {
+		if swift.IsSwiftEnabled(listers) {
 			cfg.Swift = &imageregistryv1.ImageRegistryConfigStorageSwift{}
 			replicas = 2
+			break
 		}
-
+		cfg.PVC = &imageregistryv1.ImageRegistryConfigStoragePVC{
+			Claim: defaults.PVCImageRegistryName,
+		}
+		replicas = 1
 	// Unknown platforms or LibVirt: we configure image registry using
 	// EmptyDir storage.
 	case configapiv1.LibvirtPlatformType:

diff --git a/pkg/storage/swift/swift.go b/pkg/storage/swift/swift.go
@@ -20,6 +20,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/klog"
 
 	imageregistryv1 "github.com/openshift/api/imageregistry/v1"
 	operatorapi "github.com/openshift/api/operator/v1"
@@ -58,35 +59,19 @@ func replaceEmpty(a string, b string) string {
 }
 
 // IsSwiftEnabled checks if Swift service is available for OpenStack platform
-func IsSwiftEnabled(listers *regopclient.Listers) (bool, error) {
+func IsSwiftEnabled(listers *regopclient.Listers) bool {
 	driver := NewDriver(&imageregistryv1.ImageRegistryConfigStorageSwift{}, listers)
 	conn, err := driver.getSwiftClient()
 	if err != nil {
-		// ErrEndpointNotFound means that Swift is not available
-		if _, ok := err.(*gophercloud.ErrEndpointNotFound); ok {
-			return false, nil
-		}
-		return false, err
+		klog.Errorf("swift storage inaccessible: %v", err)
+		return false
 	}
-
 	// Try to list containers to make sure the user has required permissions to do that
-	listOpts := containers.ListOpts{Full: false}
-	_, err = containers.List(conn, listOpts).AllPages()
-	if err != nil {
-		// If the user has no permissions, we consider that Swift is unavailable
-		// Depending on the configuration swift returns different error codes:
-		// 403 with Keystone and 401 with internal Swauth.
-		// It means we have to catch them both.
-		// More information about Swith auth: https://docs.openstack.org/swift/latest/overview_auth.html
-		if _, ok := err.(gophercloud.ErrDefault403); ok {
-			return false, nil
-		}
-		if _, ok := err.(gophercloud.ErrDefault401); ok {
-			return false, nil
-		}
-		return false, err
+	if _, err = containers.List(conn, containers.ListOpts{}).AllPages(); err != nil {
+		klog.Errorf("error listing swift containers: %v", err)
+		return false
 	}
-	return true, nil
+	return true
 }
 
 // GetConfig reads credentials

diff --git a/pkg/storage/swift/swift_test.go b/pkg/storage/swift/swift_test.go
@@ -750,9 +750,7 @@ func TestSwiftIsAvailable(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	res, err := IsSwiftEnabled(listers)
-	th.AssertNoErr(t, err)
-	th.AssertEquals(t, true, res)
+	th.AssertEquals(t, true, IsSwiftEnabled(listers))
 }
 
 func TestSwiftIsNotAvailable(t *testing.T) {
@@ -804,9 +802,7 @@ func TestSwiftIsNotAvailable(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	res, err := IsSwiftEnabled(listers)
-	th.AssertNoErr(t, err)
-	th.AssertEquals(t, false, res)
+	th.AssertEquals(t, false, IsSwiftEnabled(listers))
 }
 
 func TestNoPermissionsKeystone(t *testing.T) {
@@ -856,9 +852,7 @@ func TestNoPermissionsKeystone(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	res, err := IsSwiftEnabled(listers)
-	th.AssertNoErr(t, err)
-	th.AssertEquals(t, false, res)
+	th.AssertEquals(t, false, IsSwiftEnabled(listers))
 }
 
 func TestNoPermissionsSwauth(t *testing.T) {
@@ -908,7 +902,5 @@ func TestNoPermissionsSwauth(t *testing.T) {
 		Infrastructures: fakeInfrastructureLister(cloudName),
 		OpenShiftConfig: MockConfigMapNamespaceLister{},
 	}
-	res, err := IsSwiftEnabled(listers)
-	th.AssertNoErr(t, err)
-	th.AssertEquals(t, false, res)
+	th.AssertEquals(t, false, IsSwiftEnabled(listers))
 }