Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle errors in additionalItemBackupper #512

Merged
merged 2 commits into from
May 23, 2018
Merged

Handle errors in additionalItemBackupper #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24dfef6
Handle errors in additionalItemBackupper
carlpett May 22, 2018
b92d086
Add additional test
carlpett May 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/backup/item_backupper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 49 additions & 2 deletions pkg/backup/item_backupper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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("*"),
Expand All @@ -242,6 +271,17 @@ func TestBackupItemNoSkips(t *testing.T) {
"vol-abc123": {SnapshotID: "snapshot-1", AvailabilityZone: "us-east-1c"},
},
},
{
name: "backup fails when takePVSnapshot fails",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test your change to item_backupper.go, we need a test case that has this mocked behavior return a non-nil error: additionalItemBackupper.On("backupItem", mock.AnythingOfType("*logrus.Entry"), test.customActionAdditionalItems[i], item.GroupResource).Return(nil).

The test case you've added here is valuable too, so we should keep it, but it doesn't cover your fix: namely, that when we call ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()), it checks the returned error and returns it if it's non-nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this would cover your change: https://gist.github.com/ncdc/12c3b6b6f4db0e5c60f4ecf77e8d85c2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated with this, will ask some questions on Slack too :)

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 {
Expand Down Expand Up @@ -320,6 +360,7 @@ func TestBackupItemNoSkips(t *testing.T) {
snapshotService = &arktest.FakeSnapshotService{
SnapshottableVolumes: test.snapshottableVolumes,
VolumeID: "vol-abc123",
Error: test.snapshotError,
}
b.snapshotService = snapshotService
}
Expand All @@ -337,17 +378,23 @@ 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)

dynamicFactory.On("ClientForGroupVersionResource", item.GroupResource.WithVersion("").GroupVersion(), metav1.APIResource{Name: item.Resource}, item.Namespace).Return(itemClient, nil)

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)
Expand Down
24 changes: 23 additions & 1 deletion pkg/util/test/fake_snapshot_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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,
Expand All @@ -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")
}
Expand All @@ -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 {
Expand All @@ -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
}