Address code review

Signed-off-by: Carlisia <carlisiac@vmware.com>

examples/azure/05-backupstoragelocation.yaml

@@ -21,7 +21,7 @@ metadata:
 spec:
   provider: azure
   objectStorage:
-    bucket: <YOUR_BLOB_CONTAINER> 
+    bucket: velero 
   config:
-    resourceGroup: <YOUR_STORAGE_RESOURCE_GROUP>
-    storageAccount: <YOUR_STORAGE_ACCOUNT>
+    resourceGroup: VeleroDev
+    storageAccount: velero89312100bd0c

examples/azure/06-volumesnapshotlocation.yaml

@@ -21,4 +21,4 @@ metadata:
 spec:
   provider: azure
   config:
-    apiTimeout: <YOUR_TIMEOUT>
+    apiTimeout: 2m0s

pkg/cloudprovider/aws/object_store.go

@@ -220,30 +220,29 @@ func (o *ObjectStore) ObjectExists(bucket, key string) (bool, error) {
 	log.Debug("Checking if object exists")
 	_, err := o.s3.HeadObject(req)
 
-	if err == nil {
-		log.Debug("Object exists")
-		return true, nil
-	}
+	if err != nil {
+		log.Debug("Checking for AWS specific error information")
+		if aerr, ok := err.(awserr.Error); ok {
+			log.WithFields(
+				logrus.Fields{
+					"code":    aerr.Code(),
+					"message": aerr.Message(),
+				},
+			).Debugf("awserr.Error contents (origErr=%v)", aerr.OrigErr())
 
-	log.Debug("Checking for AWS specific error information")
-	if aerr, ok := err.(awserr.Error); ok {
-		log.WithFields(
-			logrus.Fields{
-				"code":    aerr.Code(),
-				"message": aerr.Message(),
-			},
-		).Debugf("awserr.Error contents (origErr=%v)", aerr.OrigErr())
-
-		// The code will be NotFound if the key doesn't exist.
-		// See https://github.com/aws/aws-sdk-go/issues/1208 and https://github.com/aws/aws-sdk-go/pull/1213.
-		log.Debugf("Checking for code=%s", notFoundCode)
-		if aerr.Code() == notFoundCode {
-			log.Debug("Object doesn't exist - got not found")
-			return false, nil
+			// The code will be NotFound if the key doesn't exist.
+			// See https://github.com/aws/aws-sdk-go/issues/1208 and https://github.com/aws/aws-sdk-go/pull/1213.
+			log.Debugf("Checking for code=%s", notFoundCode)
+			if aerr.Code() == notFoundCode {
+				log.Debug("Object doesn't exist - got not found")
+				return false, nil
+			}
 		}
+		return false, errors.WithStack(err)
 	}
 
-	return false, errors.WithStack(err)
+	log.Debug("Object exists")
+	return true, nil
 }
 
 func (o *ObjectStore) GetObject(bucket, key string) (io.ReadCloser, error) {

pkg/cloudprovider/aws/object_store_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 the Heptio Ark contributors.
+Copyright 2018, 2019 the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/cloudprovider/azure/object_store_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 the Heptio Ark contributors.
+Copyright 2018, 2019 the Velero contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/cloudprovider/gcp/object_store.go

@@ -125,14 +125,14 @@ func (o *ObjectStore) PutObject(bucket, key string, body io.Reader) error {
 func (o *ObjectStore) ObjectExists(bucket, key string) (bool, error) {
 	_, err := o.bucketWriter.getAttrs(bucket, key)
 
-	switch err {
-	case nil:
-		return true, nil
-	case storage.ErrObjectNotExist:
-		return false, nil
+	if err != nil {
+		if err == storage.ErrObjectNotExist {
+			return false, nil
+		}
+		return false, errors.WithStack(err)
 	}
 
-	return false, errors.WithStack(err)
+	return true, nil
 }
 
 func (o *ObjectStore) GetObject(bucket, key string) (io.ReadCloser, error) {

pkg/cloudprovider/in_memory_object_store.go

@@ -76,8 +76,7 @@ func (o *InMemoryObjectStore) ObjectExists(bucket, key string) (bool, error) {
 		return false, errors.New("bucket not found")
 	}
 
-	_, ok = bucketData[key]
-	if !ok {
+	if _, ok = bucketData[key]; !ok {
 		return false, errors.New("key not found")
 	}
 

pkg/controller/backup_controller.go

@@ -172,8 +172,6 @@ func (c *backupController) processBackup(key string) error {
 		return errors.Wrap(err, "error getting backup")
 	}
 
-	fmt.Println("original name is.....---- ", original.Name)
-
 	// Double-check we have the correct phase. In the unlikely event that multiple controller
 	// instances are running, it's possible for controller A to succeed in changing the phase to
 	// InProgress, while controller B's attempt to patch the phase fails. When controller B
@@ -199,9 +197,6 @@ func (c *backupController) processBackup(key string) error {
 		request.Status.StartTimestamp.Time = c.clock.Now()
 	}
 
-	fmt.Println("request.Backup.Name name is.....---- ", request.Backup.Name)
-	fmt.Println("original name is.....---- ", original.Name)
-
 	// update status
 	updatedBackup, err := patchBackup(original, request.Backup, c.client)
 	if err != nil {
@@ -417,8 +412,6 @@ func (c *backupController) validateAndGetSnapshotLocations(backup *velerov1api.B
 }
 
 func (c *backupController) runBackup(backup *pkgbackup.Request) error {
-	fmt.Println("runbackup name is.....---- ", backup.Name)
-
 	log := c.logger.WithField("backup", kubeutil.NamespaceAndName(backup))
 	log.Info("Starting backup")
 
@@ -459,15 +452,18 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 		return err
 	}
 
-	var errs []error
-	errs = append(errs, validateUniqueness(backupStore, backup.StorageLocation.Spec.StorageType.ObjectStorage.Bucket, backup.Name)...)
-	if len(errs) > 0 {
+	exists, err := backupStore.BackupExists(backup.StorageLocation.Spec.StorageType.ObjectStorage.Bucket, backup.Name)
+	if exists || err != nil {
 		backup.Status.Phase = velerov1api.BackupPhaseFailed
 		backup.Status.CompletionTimestamp.Time = c.clock.Now()
-		return kerrors.NewAggregate(errs)
+		if err != nil {
+			return errors.Errorf("Error checking if backup already exists in object storage: %v", err)
+		}
+		return errors.Errorf("Backup already exists in object storage")
 	}
 
 	// Do the actual backup
+	var errs []error
 	if err := c.backupper.Backup(log, backup, backupFile, actions, pluginManager); err != nil {
 		errs = append(errs, err)
 		backup.Status.Phase = velerov1api.BackupPhaseFailed
@@ -498,18 +494,6 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 	return kerrors.NewAggregate(errs)
 }
 
-func validateUniqueness(backupStore persistence.BackupStore, bucket, name string) []error {
-	var errs []error
-	exists, err := backupStore.BackupExists(bucket, name)
-	if err != nil {
-		errs = append(errs, errors.Errorf("Error checking if backup already exists in object storage: %v", err))
-	}
-	if exists {
-		errs = append(errs, errors.Errorf("Backup already exists in object storage"))
-	}
-	return errs
-}
-
 func recordBackupMetrics(backup *velerov1api.Backup, backupFile *os.File, serverMetrics *metrics.ServerMetrics) error {
 	backupScheduleName := backup.GetLabels()[velerov1api.ScheduleNameLabel]
 

pkg/controller/backup_controller_test.go

@@ -56,14 +56,12 @@ func (b *fakeBackupper) Backup(logger logrus.FieldLogger, backup *pkgbackup.Requ
 	return args.Error(0)
 }
 
-func TestProcessBackupProcessing(t *testing.T) {
+func TestProcessBackupNonProcessedItems(t *testing.T) {
 	tests := []struct {
-		name        string
-		key         string
-		backup      *v1.Backup
-		expectedErr string
+		name   string
+		key    string
+		backup *v1.Backup
 	}{
-		// processed successfully
 		{
 			name: "bad key does not return error",
 			key:  "bad/key/here",
@@ -72,8 +70,6 @@ func TestProcessBackupProcessing(t *testing.T) {
 			name: "backup not found in lister does not return error",
 			key:  "nonexistent/backup",
 		},
-
-		// skipped
 		{
 			name:   "FailedValidation backup is not processed",
 			key:    "velero/backup-1",
@@ -113,12 +109,7 @@ func TestProcessBackupProcessing(t *testing.T) {
 			}
 
 			err := c.processBackup(test.key)
-			if test.expectedErr != "" {
-				require.Error(t, err)
-				assert.Equal(t, test.expectedErr, err.Error())
-			} else {
-				assert.Nil(t, err)
-			}
+			assert.Nil(t, err)
 
 			// Any backup that would actually proceed to validation will cause a segfault because this
 			// test hasn't set up the necessary controller dependencies for validation/etc. So the lack
@@ -345,7 +336,7 @@ func TestProcessBackupCompletions(t *testing.T) {
 			},
 		},
 		{
-			name:           "backup with existing backup will fail",
+			name:           "backup without an existing backup will succeed",
 			backupExists:   false,
 			backup:         velerotest.NewTestBackup().WithName("backup-1").Backup,
 			backupLocation: defaultBackupLocation,
@@ -488,8 +479,6 @@ func TestProcessBackupCompletions(t *testing.T) {
 			res, err := clientset.VeleroV1().Backups(test.backup.Namespace).Get(test.backup.Name, metav1.GetOptions{})
 			require.NoError(t, err)
 
-			// failed tests for failed backup should have a phase of failed
-
 			assert.Equal(t, test.expectedResult, res)
 		})
 	}

pkg/plugin/framework/object_store_server.go

@@ -143,7 +143,7 @@ func (s *ObjectStoreGRPCServer) ObjectExists(ctx context.Context, req *proto.Obj
 
 	exists, err := impl.ObjectExists(req.Bucket, req.Key)
 	if err != nil {
-		return nil, err
+		return nil, newGRPCError(err)
 	}
 
 	return &proto.ObjectExistsResponse{Exists: exists}, nil