From b818cc2769979db02ca39fa3ad9b2fae02e327ae Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Fri, 26 Oct 2018 09:32:46 -0600 Subject: [PATCH] don't require a default provider VSL if there's only 1 Signed-off-by: Steve Kriss --- pkg/cmd/server/server.go | 47 +------- pkg/cmd/server/server_test.go | 57 --------- pkg/controller/backup_controller.go | 56 +++++++-- pkg/controller/backup_controller_test.go | 143 ++++++++++++----------- 4 files changed, 124 insertions(+), 179 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index b78d4f373..c5e7d5709 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -273,60 +273,17 @@ func (s *server) run() error { Warnf("Default backup storage location %q not found; backups must explicitly specify a location", s.config.defaultBackupLocation) } - defaultVolumeSnapshotLocations, err := getDefaultVolumeSnapshotLocations(s.arkClient, s.namespace, s.config.defaultVolumeSnapshotLocations) - if err != nil { - return err - } - if err := s.initRestic(); err != nil { return err } - if err := s.runControllers(defaultVolumeSnapshotLocations); err != nil { + if err := s.runControllers(s.config.defaultVolumeSnapshotLocations); err != nil { return err } return nil } -func getDefaultVolumeSnapshotLocations(arkClient clientset.Interface, namespace string, defaultVolumeSnapshotLocations map[string]string) (map[string]*api.VolumeSnapshotLocation, error) { - providerDefaults := make(map[string]*api.VolumeSnapshotLocation) - if len(defaultVolumeSnapshotLocations) == 0 { - return providerDefaults, nil - } - - volumeSnapshotLocations, err := arkClient.ArkV1().VolumeSnapshotLocations(namespace).List(metav1.ListOptions{}) - if err != nil { - return providerDefaults, errors.WithStack(err) - } - - providerLocations := make(map[string][]*api.VolumeSnapshotLocation) - for i, vsl := range volumeSnapshotLocations.Items { - locations := providerLocations[vsl.Spec.Provider] - providerLocations[vsl.Spec.Provider] = append(locations, &volumeSnapshotLocations.Items[i]) - } - - for provider, locations := range providerLocations { - defaultLocation, ok := defaultVolumeSnapshotLocations[provider] - if !ok { - return providerDefaults, errors.Errorf("missing provider %s. When using default volume snapshot locations, one must exist for every known provider.", provider) - } - - for _, location := range locations { - if location.ObjectMeta.Name == defaultLocation { - providerDefaults[provider] = location - break - } - } - - if _, ok := providerDefaults[provider]; !ok { - return providerDefaults, errors.Errorf("%s is not a valid volume snapshot location for %s", defaultLocation, provider) - } - } - - return providerDefaults, nil -} - // namespaceExists returns nil if namespace can be successfully // gotten from the kubernetes API, or an error otherwise. func (s *server) namespaceExists(namespace string) error { @@ -510,7 +467,7 @@ func (s *server) initRestic() error { return nil } -func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]*api.VolumeSnapshotLocation) error { +func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string) error { s.logger.Info("Starting controllers") ctx := s.ctx diff --git a/pkg/cmd/server/server_test.go b/pkg/cmd/server/server_test.go index 791b91a0e..372aff8a0 100644 --- a/pkg/cmd/server/server_test.go +++ b/pkg/cmd/server/server_test.go @@ -23,7 +23,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/heptio/ark/pkg/apis/ark/v1" - fakeclientset "github.com/heptio/ark/pkg/generated/clientset/versioned/fake" arktest "github.com/heptio/ark/pkg/util/test" ) @@ -70,59 +69,3 @@ func TestArkResourcesExist(t *testing.T) { arkAPIResourceList.APIResources = arkAPIResourceList.APIResources[:3] assert.Error(t, server.arkResourcesExist()) } - -func TestDefaultVolumeSnapshotLocations(t *testing.T) { - namespace := "heptio-ark" - arkClient := fakeclientset.NewSimpleClientset() - - location := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider1"}} - arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location) - - defaultVolumeSnapshotLocations := make(map[string]string) - - // No defaults - volumeSnapshotLocations, err := getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Equal(t, 0, len(volumeSnapshotLocations)) - assert.NoError(t, err) - - // Bad location - defaultVolumeSnapshotLocations["provider1"] = "badlocation" - volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Equal(t, 0, len(volumeSnapshotLocations)) - assert.Error(t, err) - - // Bad provider - defaultVolumeSnapshotLocations["provider2"] = "badlocation" - volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Equal(t, 0, len(volumeSnapshotLocations)) - assert.Error(t, err) - - // Good provider, good location - delete(defaultVolumeSnapshotLocations, "provider2") - defaultVolumeSnapshotLocations["provider1"] = "location1" - volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Equal(t, 1, len(volumeSnapshotLocations)) - assert.NoError(t, err) - - location2 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location2"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider2"}} - arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location2) - - // Mutliple Provider/Location 1 good, 1 bad - defaultVolumeSnapshotLocations["provider2"] = "badlocation" - volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Error(t, err) - - location21 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location2-1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider2"}} - arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location21) - - location11 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location1-1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider1"}} - arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location11) - - // Mutliple Provider/Location all good - defaultVolumeSnapshotLocations["provider2"] = "location2" - volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations) - assert.Equal(t, 2, len(volumeSnapshotLocations)) - assert.NoError(t, err) - assert.Equal(t, volumeSnapshotLocations["provider1"].ObjectMeta.Name, "location1") - assert.Equal(t, volumeSnapshotLocations["provider2"].ObjectMeta.Name, "location2") -} diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 990c5915c..542127516 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -30,6 +30,7 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/clock" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -65,7 +66,7 @@ type backupController struct { backupLocationLister listers.BackupStorageLocationLister defaultBackupLocation string snapshotLocationLister listers.VolumeSnapshotLocationLister - defaultSnapshotLocations map[string]*api.VolumeSnapshotLocation + defaultSnapshotLocations map[string]string metrics *metrics.ServerMetrics newBackupStore func(*api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) } @@ -81,7 +82,7 @@ func NewBackupController( backupLocationInformer informers.BackupStorageLocationInformer, defaultBackupLocation string, volumeSnapshotLocationInformer informers.VolumeSnapshotLocationInformer, - defaultSnapshotLocations map[string]*api.VolumeSnapshotLocation, + defaultSnapshotLocations map[string]string, metrics *metrics.ServerMetrics, ) Interface { c := &backupController{ @@ -298,7 +299,8 @@ func (c *backupController) prepareBackupRequest(backup *api.Backup) *pkgbackup.R // - each location name in .spec.volumeSnapshotLocations exists as a location // - exactly 1 location per provider // - a given provider's default location name is added to .spec.volumeSnapshotLocations if one -// is not explicitly specified for the provider +// is not explicitly specified for the provider (if there's only one location for the provider, +// it will automatically be used) func (c *backupController) validateAndGetSnapshotLocations(backup *api.Backup) (map[string]*api.VolumeSnapshotLocation, []string) { errors := []string{} providerLocations := make(map[string]*api.VolumeSnapshotLocation) @@ -328,11 +330,51 @@ func (c *backupController) validateAndGetSnapshotLocations(backup *api.Backup) ( return nil, errors } - for provider, defaultLocation := range c.defaultSnapshotLocations { - // if a location name for a given provider does not already exist, add the provider's default - if _, ok := providerLocations[provider]; !ok { - providerLocations[provider] = defaultLocation + allLocations, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).List(labels.Everything()) + if err != nil { + errors = append(errors, fmt.Sprintf("error listing volume snapshot locations: %v", err)) + return nil, errors + } + + // build a map of provider->list of all locations for the provider + allProviderLocations := make(map[string][]*api.VolumeSnapshotLocation) + for i := range allLocations { + loc := allLocations[i] + allProviderLocations[loc.Spec.Provider] = append(allProviderLocations[loc.Spec.Provider], loc) + } + + // go through each provider and make sure we have/can get a VSL + // for it + for provider, locations := range allProviderLocations { + if _, ok := providerLocations[provider]; ok { + // backup's spec had a location named for this provider + continue } + + if len(locations) > 1 { + // more than one possible location for the provider: check + // the defaults + defaultLocation := c.defaultSnapshotLocations[provider] + if defaultLocation == "" { + errors = append(errors, fmt.Sprintf("provider %s has more than one possible volume snapshot location, and none were specified explicitly or as a default", provider)) + continue + } + location, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).Get(defaultLocation) + if err != nil { + errors = append(errors, fmt.Sprintf("error getting volume snapshot location named %s: %v", defaultLocation, err)) + continue + } + + providerLocations[provider] = location + continue + } + + // exactly one location for the provider: use it + providerLocations[provider] = locations[0] + } + + if len(errors) > 0 { + return nil, errors } return providerLocations, nil diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 537d53748..4d9c20c36 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -163,11 +163,12 @@ func TestProcessBackupValidationFailures(t *testing.T) { ) c := &backupController{ - genericController: newGenericController("backup-test", logger), - client: clientset.ArkV1(), - lister: sharedInformers.Ark().V1().Backups().Lister(), - backupLocationLister: sharedInformers.Ark().V1().BackupStorageLocations().Lister(), - defaultBackupLocation: defaultBackupLocation.Name, + genericController: newGenericController("backup-test", logger), + client: clientset.ArkV1(), + lister: sharedInformers.Ark().V1().Backups().Lister(), + backupLocationLister: sharedInformers.Ark().V1().BackupStorageLocations().Lister(), + snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(), + defaultBackupLocation: defaultBackupLocation.Name, } require.NotNil(t, test.backup) @@ -294,15 +295,16 @@ func TestProcessBackupCompletions(t *testing.T) { ) c := &backupController{ - genericController: newGenericController("backup-test", logger), - client: clientset.ArkV1(), - lister: sharedInformers.Ark().V1().Backups().Lister(), - backupLocationLister: sharedInformers.Ark().V1().BackupStorageLocations().Lister(), - defaultBackupLocation: defaultBackupLocation.Name, - backupTracker: NewBackupTracker(), - metrics: metrics.NewServerMetrics(), - clock: clock.NewFakeClock(now), - newPluginManager: func(logrus.FieldLogger) plugin.Manager { return pluginManager }, + genericController: newGenericController("backup-test", logger), + client: clientset.ArkV1(), + lister: sharedInformers.Ark().V1().Backups().Lister(), + backupLocationLister: sharedInformers.Ark().V1().BackupStorageLocations().Lister(), + snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(), + defaultBackupLocation: defaultBackupLocation.Name, + backupTracker: NewBackupTracker(), + metrics: metrics.NewServerMetrics(), + clock: clock.NewFakeClock(now), + newPluginManager: func(logrus.FieldLogger) plugin.Manager { return pluginManager }, newBackupStore: func(*v1.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { return backupStore, nil }, @@ -351,76 +353,74 @@ func TestProcessBackupCompletions(t *testing.T) { } func TestValidateAndGetSnapshotLocations(t *testing.T) { - defaultLocationsAWS := map[string]*v1.VolumeSnapshotLocation{ - "aws": arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-east-2").VolumeSnapshotLocation, - } - defaultLocationsFake := map[string]*v1.VolumeSnapshotLocation{ - "fake-provider": arktest.NewTestVolumeSnapshotLocation().WithName("some-name").VolumeSnapshotLocation, - } - - multipleLocationNames := []string{"aws-us-west-1", "aws-us-east-1"} - - multipleLocation1 := arktest.LocationInfo{ - Name: multipleLocationNames[0], - Provider: "aws", - Config: map[string]string{"region": "us-west-1"}, - } - multipleLocation2 := arktest.LocationInfo{ - Name: multipleLocationNames[1], - Provider: "aws", - Config: map[string]string{"region": "us-west-1"}, - } - - multipleLocationList := []arktest.LocationInfo{multipleLocation1, multipleLocation2} - - dupLocationNames := []string{"aws-us-west-1", "aws-us-west-1"} - dupLocation1 := arktest.LocationInfo{ - Name: dupLocationNames[0], - Provider: "aws", - Config: map[string]string{"region": "us-west-1"}, - } - dupLocation2 := arktest.LocationInfo{ - Name: dupLocationNames[0], - Provider: "aws", - Config: map[string]string{"region": "us-west-1"}, - } - dupLocationList := []arktest.LocationInfo{dupLocation1, dupLocation2} - tests := []struct { name string backup *arktest.TestBackup locations []*arktest.TestVolumeSnapshotLocation - defaultLocations map[string]*v1.VolumeSnapshotLocation + defaultLocations map[string]string expectedVolumeSnapshotLocationNames []string // adding these in the expected order will allow to test with better msgs in case of a test failure expectedErrors string expectedSuccess bool }{ { - name: "location name does not correspond to any existing location", - backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("random-name"), - locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), + name: "location name does not correspond to any existing location", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("random-name"), + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"), + }, expectedErrors: "error getting volume snapshot location named random-name: volumesnapshotlocation.ark.heptio.com \"random-name\" not found", expectedSuccess: false, }, { - name: "duplicate locationName per provider: should filter out dups", - backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames...), - locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), - expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0]}, + name: "duplicate locationName per provider: should filter out dups", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1"), + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"), + }, + expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1"}, expectedSuccess: true, }, { - name: "multiple location names per provider", - backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(multipleLocationNames...), - locations: arktest.NewTestVolumeSnapshotLocation().WithName(multipleLocationNames[0]).WithProviderConfig(multipleLocationList), - expectedErrors: "more than one VolumeSnapshotLocation name specified for provider aws: aws-us-east-1; unexpected name was aws-us-west-1", + name: "multiple non-dupe location names per provider should error", + backup: arktest.NewTestBackup().WithName("backup1").WithVolumeSnapshotLocations("aws-us-east-1", "aws-us-west-1"), + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"), + }, + expectedErrors: "more than one VolumeSnapshotLocation name specified for provider aws: aws-us-west-1; unexpected name was aws-us-east-1", expectedSuccess: false, }, { - name: "no location name for the provider exists: the provider's default should be added", - backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), - defaultLocations: defaultLocationsAWS, - expectedVolumeSnapshotLocationNames: []string{defaultLocationsAWS["aws"].Name}, + name: "no location name for the provider exists, only one VSL for the provider: use it", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"), + }, + expectedVolumeSnapshotLocationNames: []string{"aws-us-east-1"}, + expectedSuccess: true, + }, + { + name: "no location name for the provider exists, no default, more than one VSL for the provider: error", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"), + }, + expectedErrors: "provider aws has more than one possible volume snapshot location, and none were specified explicitly or as a default", + }, + { + name: "no location name for the provider exists, more than one VSL for the provider: the provider's default should be added", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), + defaultLocations: map[string]string{"aws": "aws-us-east-1"}, + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-east-1").WithProvider("aws"), + arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-west-1").WithProvider("aws"), + }, + expectedVolumeSnapshotLocationNames: []string{"aws-us-east-1"}, expectedSuccess: true, }, { @@ -429,11 +429,14 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) { expectedSuccess: true, }, { - name: "multiple location names for a provider, default location name for another provider", - backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames...), - locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), - defaultLocations: defaultLocationsFake, - expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0], defaultLocationsFake["fake-provider"].Name}, + name: "multiple location names for a provider, default location name for another provider", + backup: arktest.NewTestBackup().WithName("backup1").WithVolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1"), + defaultLocations: map[string]string{"fake-provider": "some-name"}, + locations: []*arktest.TestVolumeSnapshotLocation{ + arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"), + arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"), + }, + expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1", "some-name"}, expectedSuccess: true, }, }