Merge pull request #512 from carlpett/additional-backup-error

Handle errors in additionalItemBackupper

pkg/backup/item_backupper.go

@@ -206,7 +206,9 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 					return err
 				}
 
-				ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource())
+				if err = ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()); err != nil {
+					return err
+				}
 			}
 		} else {
 			// We want this to show up in the log file at the place where the error occurs. When we return

pkg/backup/item_backupper_test.go

@@ -139,6 +139,8 @@ func TestBackupItemNoSkips(t *testing.T) {
 		customActionAdditionalItems           []runtime.Unstructured
 		groupResource                         string
 		snapshottableVolumes                  map[string]api.VolumeBackupInfo
+		snapshotError                         error
+		additionalItemError                   error
 	}{
 		{
 			name: "explicit namespace include",
@@ -221,6 +223,33 @@ func TestBackupItemNoSkips(t *testing.T) {
 				unstructuredOrDie(`{"apiVersion":"g2/v1","kind":"r1","metadata":{"namespace":"ns2","name":"n2"}}`),
 			},
 		},
+		{
+			name: "action invoked - additional items - error",
+			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
+			item:                  `{"metadata":{"namespace": "myns", "name":"bar"}}`,
+			expectError:           true,
+			expectExcluded:        false,
+			expectedTarHeaderName: "resources/resource.group/namespaces/myns/bar.json",
+			customAction:          true,
+			expectedActionID:      "myns/bar",
+			customActionAdditionalItemIdentifiers: []ResourceIdentifier{
+				{
+					GroupResource: schema.GroupResource{Group: "g1", Resource: "r1"},
+					Namespace:     "ns1",
+					Name:          "n1",
+				},
+				{
+					GroupResource: schema.GroupResource{Group: "g2", Resource: "r2"},
+					Namespace:     "ns2",
+					Name:          "n2",
+				},
+			},
+			customActionAdditionalItems: []runtime.Unstructured{
+				unstructuredOrDie(`{"apiVersion":"g1/v1","kind":"r1","metadata":{"namespace":"ns1","name":"n1"}}`),
+				unstructuredOrDie(`{"apiVersion":"g2/v1","kind":"r1","metadata":{"namespace":"ns2","name":"n2"}}`),
+			},
+			additionalItemError: errors.New("foo"),
+		},
 		{
 			name: "takePVSnapshot is not invoked for PVs when snapshotService == nil",
 			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
@@ -242,6 +271,17 @@ func TestBackupItemNoSkips(t *testing.T) {
 				"vol-abc123": {SnapshotID: "snapshot-1", AvailabilityZone: "us-east-1c"},
 			},
 		},
+		{
+			name: "backup fails when takePVSnapshot fails",
+			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
+			item:          `{"apiVersion": "v1", "kind": "PersistentVolume", "metadata": {"name": "mypv", "labels": {"failure-domain.beta.kubernetes.io/zone": "us-east-1c"}}, "spec": {"awsElasticBlockStore": {"volumeID": "aws://us-east-1c/vol-abc123"}}}`,
+			expectError:   true,
+			groupResource: "persistentvolumes",
+			snapshottableVolumes: map[string]api.VolumeBackupInfo{
+				"vol-abc123": {SnapshotID: "snapshot-1", AvailabilityZone: "us-east-1c"},
+			},
+			snapshotError: fmt.Errorf("failure"),
+		},
 	}
 
 	for _, test := range tests {
@@ -320,6 +360,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 				snapshotService = &arktest.FakeSnapshotService{
 					SnapshottableVolumes: test.snapshottableVolumes,
 					VolumeID:             "vol-abc123",
+					Error:                test.snapshotError,
 				}
 				b.snapshotService = snapshotService
 			}
@@ -337,9 +378,15 @@ func TestBackupItemNoSkips(t *testing.T) {
 
 			obj := &unstructured.Unstructured{Object: item}
 			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePre).Return(nil)
-			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePost).Return(nil)
+			if test.snapshotError == nil && test.additionalItemError == nil {
+				// TODO: Remove if-clause when #511 is resolved.
+				itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, resourceHooks, hookPhasePost).Return(nil)
+			}
 
 			for i, item := range test.customActionAdditionalItemIdentifiers {
+				if test.additionalItemError != nil && i > 0 {
+					break
+				}
 				itemClient := &arktest.FakeDynamicClient{}
 				defer itemClient.AssertExpectations(t)
 
@@ -347,7 +394,7 @@ func TestBackupItemNoSkips(t *testing.T) {
 
 				itemClient.On("Get", item.Name, metav1.GetOptions{}).Return(test.customActionAdditionalItems[i], nil)
 
-				additionalItemBackupper.On("backupItem", mock.AnythingOfType("*logrus.Entry"), test.customActionAdditionalItems[i], item.GroupResource).Return(nil)
+				additionalItemBackupper.On("backupItem", mock.AnythingOfType("*logrus.Entry"), test.customActionAdditionalItems[i], item.GroupResource).Return(test.additionalItemError)
 			}
 
 			err = b.backupItem(arktest.NewLogger(), obj, groupResource)

pkg/util/test/fake_snapshot_service.go

@@ -37,9 +37,15 @@ type FakeSnapshotService struct {
 
 	VolumeID    string
 	VolumeIDSet string
+
+	Error error
 }
 
 func (s *FakeSnapshotService) CreateSnapshot(volumeID, volumeAZ string, tags map[string]string) (string, error) {
+	if s.Error != nil {
+		return "", s.Error
+	}
+
 	if _, exists := s.SnapshottableVolumes[volumeID]; !exists {
 		return "", errors.New("snapshottable volume not found")
 	}
@@ -53,6 +59,10 @@ func (s *FakeSnapshotService) CreateSnapshot(volumeID, volumeAZ string, tags map
 }
 
 func (s *FakeSnapshotService) CreateVolumeFromSnapshot(snapshotID, volumeType, volumeAZ string, iops *int64) (string, error) {
+	if s.Error != nil {
+		return "", s.Error
+	}
+
 	key := api.VolumeBackupInfo{
 		SnapshotID:       snapshotID,
 		Type:             volumeType,
@@ -64,6 +74,10 @@ func (s *FakeSnapshotService) CreateVolumeFromSnapshot(snapshotID, volumeType, v
 }
 
 func (s *FakeSnapshotService) DeleteSnapshot(snapshotID string) error {
+	if s.Error != nil {
+		return s.Error
+	}
+
 	if !s.SnapshotsTaken.Has(snapshotID) {
 		return errors.New("snapshot not found")
 	}
@@ -74,6 +88,10 @@ func (s *FakeSnapshotService) DeleteSnapshot(snapshotID string) error {
 }
 
 func (s *FakeSnapshotService) GetVolumeInfo(volumeID, volumeAZ string) (string, *int64, error) {
+	if s.Error != nil {
+		return "", nil, s.Error
+	}
+
 	if volumeInfo, exists := s.SnapshottableVolumes[volumeID]; !exists {
 		return "", nil, errors.New("VolumeID not found")
 	} else {
@@ -82,10 +100,14 @@ func (s *FakeSnapshotService) GetVolumeInfo(volumeID, volumeAZ string) (string,
 }
 
 func (s *FakeSnapshotService) GetVolumeID(pv runtime.Unstructured) (string, error) {
+	if s.Error != nil {
+		return "", s.Error
+	}
+
 	return s.VolumeID, nil
 }
 
 func (s *FakeSnapshotService) SetVolumeID(pv runtime.Unstructured, volumeID string) (runtime.Unstructured, error) {
 	s.VolumeIDSet = volumeID
-	return pv, nil
+	return pv, s.Error
 }