From 6513e8f30e8314f2188159b3472fd627a9bd2619 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Wed, 12 Jun 2019 12:45:04 -0600 Subject: [PATCH] migrate more backup action tests, remove obsolete test code (#1564) * migrate more backup action tests, remove obsolete test code Signed-off-by: Steve Kriss --- pkg/backup/backup_new_test.go | 76 +++++++++++++++++++++ pkg/backup/backup_test.go | 54 --------------- pkg/backup/item_backupper_test.go | 110 ++++-------------------------- 3 files changed, 89 insertions(+), 151 deletions(-) diff --git a/pkg/backup/backup_new_test.go b/pkg/backup/backup_new_test.go index 656be30a9..4550f5c69 100644 --- a/pkg/backup/backup_new_test.go +++ b/pkg/backup/backup_new_test.go @@ -34,6 +34,7 @@ import ( "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -42,6 +43,7 @@ import ( dynamicfake "k8s.io/client-go/dynamic/fake" kubefake "k8s.io/client-go/kubernetes/fake" + v1 "github.com/heptio/velero/pkg/apis/velero/v1" velerov1 "github.com/heptio/velero/pkg/apis/velero/v1" "github.com/heptio/velero/pkg/client" "github.com/heptio/velero/pkg/discovery" @@ -49,6 +51,7 @@ import ( "github.com/heptio/velero/pkg/kuberesource" "github.com/heptio/velero/pkg/plugin/velero" "github.com/heptio/velero/pkg/test" + kubeutil "github.com/heptio/velero/pkg/util/kube" ) // TestBackupResourceFiltering runs backups with different combinations @@ -618,6 +621,51 @@ func TestBackupResourceOrdering(t *testing.T) { } } +// recordResourcesAction is a backup item action that can be configured +// to run for specific resources/namespaces and simply records the items +// that it is executed for. +type recordResourcesAction struct { + selector velero.ResourceSelector + ids []string + backups []v1.Backup + additionalItems []velero.ResourceIdentifier +} + +func (a *recordResourcesAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) { + metadata, err := meta.Accessor(item) + if err != nil { + return item, a.additionalItems, err + } + a.ids = append(a.ids, kubeutil.NamespaceAndName(metadata)) + a.backups = append(a.backups, *backup) + + return item, a.additionalItems, nil +} + +func (a *recordResourcesAction) AppliesTo() (velero.ResourceSelector, error) { + return a.selector, nil +} + +func (a *recordResourcesAction) ForResource(resource string) *recordResourcesAction { + a.selector.IncludedResources = append(a.selector.IncludedResources, resource) + return a +} + +func (a *recordResourcesAction) ForNamespace(namespace string) *recordResourcesAction { + a.selector.IncludedNamespaces = append(a.selector.IncludedNamespaces, namespace) + return a +} + +func (a *recordResourcesAction) ForLabelSelector(selector string) *recordResourcesAction { + a.selector.LabelSelector = selector + return a +} + +func (a *recordResourcesAction) WithAdditionalItems(items []velero.ResourceIdentifier) *recordResourcesAction { + a.additionalItems = items + return a +} + // TestBackupActionsRunsForCorrectItems runs backups with backup item actions, and // verifies that each backup item action is run for the correct set of resources based on its // AppliesTo() resource selector. Verification is done by using the recordResourcesAction struct, @@ -1156,6 +1204,34 @@ func TestBackupActionAdditionalItems(t *testing.T) { "resources/pods/namespaces/ns-2/pod-2.json", }, }, + { + name: "if there's an error backing up additional items, the item the action was run for isn't backed up", + backup: defaultBackup().Backup(), + apiResources: []*apiResource{ + pods( + newPod("ns-1", "pod-1"), + newPod("ns-2", "pod-2"), + newPod("ns-3", "pod-3"), + ), + }, + actions: []velero.BackupItemAction{ + &pluggableAction{ + selector: velero.ResourceSelector{IncludedNamespaces: []string{"ns-1"}}, + executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) { + additionalItems := []velero.ResourceIdentifier{ + {GroupResource: kuberesource.Pods, Namespace: "ns-4", Name: "pod-4"}, + {GroupResource: kuberesource.Pods, Namespace: "ns-5", Name: "pod-5"}, + } + + return item, additionalItems, nil + }, + }, + }, + want: []string{ + "resources/pods/namespaces/ns-2/pod-2.json", + "resources/pods/namespaces/ns-3/pod-3.json", + }, + }, } for _, tc := range tests { diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 3c10cb4d7..f5964d531 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -25,7 +25,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" @@ -39,63 +38,10 @@ import ( "github.com/heptio/velero/pkg/podexec" "github.com/heptio/velero/pkg/restic" "github.com/heptio/velero/pkg/util/collections" - kubeutil "github.com/heptio/velero/pkg/util/kube" "github.com/heptio/velero/pkg/util/logging" velerotest "github.com/heptio/velero/pkg/util/test" ) -var ( - trueVal = true - falseVal = false - truePointer = &trueVal - falsePointer = &falseVal -) - -// recordResourcesAction is a backup item action that can be configured -// to run for specific resources/namespaces and simply records the items -// that it is executed for. -type recordResourcesAction struct { - selector velero.ResourceSelector - ids []string - backups []v1.Backup - additionalItems []velero.ResourceIdentifier -} - -func (a *recordResourcesAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) { - metadata, err := meta.Accessor(item) - if err != nil { - return item, a.additionalItems, err - } - a.ids = append(a.ids, kubeutil.NamespaceAndName(metadata)) - a.backups = append(a.backups, *backup) - - return item, a.additionalItems, nil -} - -func (a *recordResourcesAction) AppliesTo() (velero.ResourceSelector, error) { - return a.selector, nil -} - -func (a *recordResourcesAction) ForResource(resource string) *recordResourcesAction { - a.selector.IncludedResources = append(a.selector.IncludedResources, resource) - return a -} - -func (a *recordResourcesAction) ForNamespace(namespace string) *recordResourcesAction { - a.selector.IncludedNamespaces = append(a.selector.IncludedNamespaces, namespace) - return a -} - -func (a *recordResourcesAction) ForLabelSelector(selector string) *recordResourcesAction { - a.selector.LabelSelector = selector - return a -} - -func (a *recordResourcesAction) WithAdditionalItems(items []velero.ResourceIdentifier) *recordResourcesAction { - a.additionalItems = items - return a -} - var ( v1Group = &metav1.APIResourceList{ GroupVersion: "v1", diff --git a/pkg/backup/item_backupper_test.go b/pkg/backup/item_backupper_test.go index 88f349ccd..20d0b42a2 100644 --- a/pkg/backup/item_backupper_test.go +++ b/pkg/backup/item_backupper_test.go @@ -25,7 +25,6 @@ import ( "time" "github.com/pkg/errors" - "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -46,24 +45,19 @@ import ( func TestBackupItemNoSkips(t *testing.T) { tests := []struct { - name string - item string - namespaceIncludesExcludes *collections.IncludesExcludes - expectError bool - expectExcluded bool - expectedTarHeaderName string - tarWriteError bool - tarHeaderWriteError bool - customAction bool - expectedActionID string - customActionAdditionalItemIdentifiers []velero.ResourceIdentifier - customActionAdditionalItems []runtime.Unstructured - groupResource string - snapshottableVolumes map[string]velerotest.VolumeBackupInfo - snapshotError error - additionalItemError error - trackedPVCs sets.String - expectedTrackedPVCs sets.String + name string + item string + namespaceIncludesExcludes *collections.IncludesExcludes + expectError bool + expectExcluded bool + expectedTarHeaderName string + tarWriteError bool + tarHeaderWriteError bool + groupResource string + snapshottableVolumes map[string]velerotest.VolumeBackupInfo + snapshotError error + trackedPVCs sets.String + expectedTrackedPVCs sets.String }{ { name: "tar header write error", @@ -77,33 +71,6 @@ func TestBackupItemNoSkips(t *testing.T) { expectError: true, tarWriteError: true, }, - { - 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: []velero.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{ - velerotest.UnstructuredOrDie(`{"apiVersion":"g1/v1","kind":"r1","metadata":{"namespace":"ns1","name":"n1"}}`), - velerotest.UnstructuredOrDie(`{"apiVersion":"g2/v1","kind":"r1","metadata":{"namespace":"ns2","name":"n2"}}`), - }, - additionalItemError: errors.New("foo"), - }, { name: "takePVSnapshot is not invoked for PVs when volumeSnapshotter == nil", namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"), @@ -177,7 +144,6 @@ func TestBackupItemNoSkips(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { var ( - action *recordResourcesAction backup = new(Request) groupResource = schema.ParseGroupResource("resource.group") backedUpItems = make(map[itemKey]struct{}) @@ -212,18 +178,6 @@ func TestBackupItemNoSkips(t *testing.T) { w.writeError = errors.New("error") } - if test.customAction { - action = new(recordResourcesAction).WithAdditionalItems(test.customActionAdditionalItemIdentifiers) - backup.ResolvedActions = []resolvedAction{ - { - BackupItemAction: action, - namespaceIncludesExcludes: collections.NewIncludesExcludes(), - resourceIncludesExcludes: collections.NewIncludesExcludes().Includes(groupResource.String()), - selector: labels.Everything(), - }, - } - } - podCommandExecutor := &velerotest.MockPodCommandExecutor{} defer podCommandExecutor.AssertExpectations(t) @@ -268,28 +222,10 @@ func TestBackupItemNoSkips(t *testing.T) { defer itemHookHandler.AssertExpectations(t) b.itemHookHandler = itemHookHandler - additionalItemBackupper := &mockItemBackupper{} - defer additionalItemBackupper.AssertExpectations(t) - b.additionalItemBackupper = additionalItemBackupper - obj := &unstructured.Unstructured{Object: item} itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, backup.ResourceHooks, hookPhasePre).Return(nil) itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, backup.ResourceHooks, hookPhasePost).Return(nil) - for i, item := range test.customActionAdditionalItemIdentifiers { - if test.additionalItemError != nil && i > 0 { - break - } - itemClient := &velerotest.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(test.additionalItemError) - } - err = b.backupItem(velerotest.NewLogger(), obj, groupResource) gotError := err != nil if e, a := test.expectError, gotError; e != a { @@ -330,17 +266,6 @@ func TestBackupItemNoSkips(t *testing.T) { t.Errorf("data: expected %s, got %s", e, a) } - if test.customAction { - if len(action.ids) != 1 { - t.Errorf("unexpected custom action ids: %v", action.ids) - } else if e, a := test.expectedActionID, action.ids[0]; e != a { - t.Errorf("action.ids[0]: expected %s, got %s", e, a) - } - - require.Equal(t, 1, len(action.backups), "unexpected custom action backups: %#v", action.backups) - assert.Equal(t, backup.Backup, &(action.backups[0]), "backup") - } - if test.snapshottableVolumes != nil { require.Equal(t, len(test.snapshottableVolumes), len(volumeSnapshotter.SnapshotsTaken)) } @@ -624,12 +549,3 @@ func (w *fakeTarWriter) WriteHeader(header *tar.Header) error { w.headers = append(w.headers, header) return w.writeHeaderError } - -type mockItemBackupper struct { - mock.Mock -} - -func (ib *mockItemBackupper) backupItem(logger logrus.FieldLogger, obj runtime.Unstructured, groupResource schema.GroupResource) error { - args := ib.Called(logger, obj, groupResource) - return args.Error(0) -}