From 7d28f825406cf8f466b80dbbdccb0da585b884cf Mon Sep 17 00:00:00 2001 From: Carlisia Date: Wed, 24 Apr 2019 10:54:43 -0700 Subject: [PATCH] Address code review Signed-off-by: Carlisia --- examples/azure/05-backupstoragelocation.yaml | 6 +-- examples/azure/06-volumesnapshotlocation.yaml | 2 +- pkg/cloudprovider/aws/object_store.go | 39 +++++++++---------- pkg/cloudprovider/aws/object_store_test.go | 2 +- pkg/cloudprovider/azure/object_store_test.go | 2 +- pkg/cloudprovider/gcp/object_store.go | 12 +++--- pkg/cloudprovider/in_memory_object_store.go | 3 +- pkg/controller/backup_controller.go | 30 ++++---------- pkg/controller/backup_controller_test.go | 23 +++-------- pkg/plugin/framework/object_store_server.go | 2 +- 10 files changed, 46 insertions(+), 75 deletions(-) diff --git a/examples/azure/05-backupstoragelocation.yaml b/examples/azure/05-backupstoragelocation.yaml index 0dbff0a2b..e2656dd62 100644 --- a/examples/azure/05-backupstoragelocation.yaml +++ b/examples/azure/05-backupstoragelocation.yaml @@ -21,7 +21,7 @@ metadata: spec: provider: azure objectStorage: - bucket: + bucket: velero config: - resourceGroup: - storageAccount: + resourceGroup: VeleroDev + storageAccount: velero89312100bd0c diff --git a/examples/azure/06-volumesnapshotlocation.yaml b/examples/azure/06-volumesnapshotlocation.yaml index f32524124..40b5c16b2 100644 --- a/examples/azure/06-volumesnapshotlocation.yaml +++ b/examples/azure/06-volumesnapshotlocation.yaml @@ -21,4 +21,4 @@ metadata: spec: provider: azure config: - apiTimeout: + apiTimeout: 2m0s diff --git a/pkg/cloudprovider/aws/object_store.go b/pkg/cloudprovider/aws/object_store.go index 09d4ca4ec..825c3849b 100644 --- a/pkg/cloudprovider/aws/object_store.go +++ b/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) { diff --git a/pkg/cloudprovider/aws/object_store_test.go b/pkg/cloudprovider/aws/object_store_test.go index 21f053b45..0dbc0b5be 100644 --- a/pkg/cloudprovider/aws/object_store_test.go +++ b/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. diff --git a/pkg/cloudprovider/azure/object_store_test.go b/pkg/cloudprovider/azure/object_store_test.go index f572b9b4f..7d0ab77be 100644 --- a/pkg/cloudprovider/azure/object_store_test.go +++ b/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. diff --git a/pkg/cloudprovider/gcp/object_store.go b/pkg/cloudprovider/gcp/object_store.go index c9cad80aa..a3a682433 100644 --- a/pkg/cloudprovider/gcp/object_store.go +++ b/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) { diff --git a/pkg/cloudprovider/in_memory_object_store.go b/pkg/cloudprovider/in_memory_object_store.go index 26e2c66c7..66be661aa 100644 --- a/pkg/cloudprovider/in_memory_object_store.go +++ b/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") } diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 1246601ac..ada1dc5f9 100644 --- a/pkg/controller/backup_controller.go +++ b/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] diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index bc0c30192..ec54c2df3 100644 --- a/pkg/controller/backup_controller_test.go +++ b/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) }) } diff --git a/pkg/plugin/framework/object_store_server.go b/pkg/plugin/framework/object_store_server.go index b17fa31f2..3784f523a 100644 --- a/pkg/plugin/framework/object_store_server.go +++ b/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