Separate backup (#87)

* Separate backup

* separates out the backup logic from the cobra command
* adds a basic test to the backup command

Note: `backup` updates the cluster spec, which updates the
job pod, so this command is best tested with an e2e test
with a running PGO.

Issue: PGO-663

Author: benjaminjb

internal/cmd/backup.go

@@ -23,6 +23,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/dynamic"
 
 	"github.com/crunchydata/postgres-operator-client/internal"
 	"github.com/crunchydata/postgres-operator-client/internal/apis/postgres-operator.crunchydata.com/v1beta1"
@@ -47,6 +48,8 @@ the --force-conflicts flag.
     postgresclusters.postgres-operator.crunchydata.com  [get patch]
 
 ### Usage`,
+		// Limit the number of args, that is, only one cluster name
+		Args: cobra.ExactArgs(1),
 	}
 
 	cmdBackup.Example = internal.FormatExample(`# Trigger a backup on the 'hippo' postgrescluster using the current spec options
@@ -63,13 +66,10 @@ pgo backup hippo --force-conflicts
 ### Example output
 postgresclusters/hippo backup initiated`)
 
-	// Limit the number of args, that is, only one cluster name
-	cmdBackup.Args = cobra.ExactArgs(1)
-
 	// `backup` command accepts `repoName`, `force-conflicts` and `options` flags;
 	// multiple options flags can be used, with each becoming a new line
 	// in the options array on the spec
-	backup := pgBackRestBackup{}
+	backup := pgBackRestBackupArgs{}
 	cmdBackup.Flags().BoolVar(&backup.ForceConflicts, "force-conflicts", false, "take ownership and overwrite the backup settings")
 	cmdBackup.Flags().StringVar(&backup.RepoName, "repoName", "", "repoName to backup to")
 	cmdBackup.Flags().StringArrayVar(&backup.Options, "options", []string{},
@@ -79,88 +79,47 @@ postgresclusters/hippo backup initiated`)
 	cmdBackup.RunE = func(cmd *cobra.Command, args []string) error {
 
 		// configure client
-		ctx := context.Background()
 		mapping, client, err := v1beta1.NewPostgresClusterClient(config)
 		if err != nil {
 			return err
 		}
 
-		// Get the namespace. This will either be from the Kubernetes configuration
-		// or from the --namespace (-n) flag.
-		configNamespace, err := config.Namespace()
-		if err != nil {
-			return err
-		}
-
-		cluster, err := client.Namespace(configNamespace).Get(ctx,
-			args[0], // the name of the cluster object, limited to one name through `ExactArgs(1)`
-			metav1.GetOptions{},
-		)
-		if err != nil {
-			return err
-		}
-
-		intent := new(unstructured.Unstructured)
-		if err := internal.ExtractFieldsInto(cluster, intent, config.Patch.FieldManager); err != nil {
-			return err
-		}
-		if err := backup.modifyIntent(intent, time.Now()); err != nil {
-			return err
-		}
-
-		patch, err := intent.MarshalJSON()
-		if err != nil {
-			cmd.Printf("\nError packaging payload: %s\n", err)
-			return err
-		}
+		// Pass args[0] as the name of the cluster object, limited to one through `ExactArgs(1)`
+		backup.ClusterName = args[0]
 
-		// Update the spec/annotate
-		// TODO(benjaminjb): Would we want to allow a dry-run option here?
-		patchOptions := metav1.PatchOptions{}
-		if backup.ForceConflicts {
-			b := true
-			patchOptions.Force = &b
+		msg, err := backup.Run(client, config)
+		if msg != "" {
+			cmd.Println(msg)
 		}
-		_, err = client.Namespace(configNamespace).Patch(ctx,
-			args[0], // the name of the cluster object, limited to one name through `ExactArgs(1)`
-			types.ApplyPatchType,
-			patch,
-			config.Patch.PatchOptions(patchOptions),
-		)
-
-		if err != nil {
-			if apierrors.IsConflict(err) {
-				cmd.Printf("SUGGESTION: The --force-conflicts flag may help in performing this operation.")
-			}
-			cmd.Printf("\nError requesting update: %s\n", err)
-			return err
+		if err == nil {
+			// Our `backup` command initiates a job, but does not signal to the user
+			// that a backup has finished; consider a `--wait` flag to wait until the
+			// backup is done.
+			cmd.Printf("%s/%s backup initiated\n", mapping.Resource.Resource, backup.ClusterName)
 		}
 
-		// Print the output received.
-		// TODO(benjaminjb): consider a more informative output
-		cmd.Printf("%s/%s backup initiated\n", mapping.Resource.Resource, args[0])
-
-		return nil
+		return err
 	}
 
 	return cmdBackup
 }
 
-type pgBackRestBackup struct {
+type pgBackRestBackupArgs struct {
+	ClusterName    string
+	ForceConflicts bool
 	Options        []string
 	RepoName       string
-	ForceConflicts bool
 }
 
-func (config pgBackRestBackup) modifyIntent(
+func (backup pgBackRestBackupArgs) modifyIntent(
 	intent *unstructured.Unstructured, now time.Time,
 ) error {
 	intent.SetAnnotations(internal.MergeStringMaps(
 		intent.GetAnnotations(), map[string]string{
 			"postgres-operator.crunchydata.com/pgbackrest-backup": now.UTC().Format(time.RFC3339),
 		}))
 
-	if value, path := config.Options, []string{
+	if value, path := backup.Options, []string{
 		"spec", "backups", "pgbackrest", "manual", "options",
 	}; len(value) == 0 {
 		unstructured.RemoveNestedField(intent.Object, path...)
@@ -170,7 +129,7 @@ func (config pgBackRestBackup) modifyIntent(
 		return err
 	}
 
-	if value, path := config.RepoName, []string{
+	if value, path := backup.RepoName, []string{
 		"spec", "backups", "pgbackrest", "manual", "repoName",
 	}; len(value) == 0 {
 		unstructured.RemoveNestedField(intent.Object, path...)
@@ -185,3 +144,63 @@ func (config pgBackRestBackup) modifyIntent(
 
 	return nil
 }
+
+func (backup pgBackRestBackupArgs) Run(client dynamic.NamespaceableResourceInterface,
+	config *internal.Config) (string, error) {
+
+	var (
+		cluster   *unstructured.Unstructured
+		err       error
+		namespace string
+		patch     []byte
+	)
+
+	ctx := context.Background()
+
+	// Get the namespace. This will either be from the Kubernetes configuration
+	// or from the --namespace (-n) flag.
+	if namespace, err = config.Namespace(); err != nil {
+		return "", err
+	}
+
+	if cluster, err = client.Namespace(namespace).Get(ctx,
+		backup.ClusterName,
+		metav1.GetOptions{},
+	); err != nil {
+		return "", err
+	}
+
+	intent := new(unstructured.Unstructured)
+	if err = internal.ExtractFieldsInto(
+		cluster, intent, config.Patch.FieldManager); err != nil {
+		return "", err
+	}
+	if err = backup.modifyIntent(intent, time.Now()); err != nil {
+		return "", err
+	}
+
+	if patch, err = intent.MarshalJSON(); err != nil {
+		return "Error packaging payload", err
+	}
+
+	// Update the spec/annotate
+	// TODO(benjaminjb): Would we want to allow a dry-run option here?
+	patchOptions := metav1.PatchOptions{}
+	if backup.ForceConflicts {
+		b := true
+		patchOptions.Force = &b
+	}
+	if _, err = client.Namespace(namespace).Patch(ctx,
+		backup.ClusterName,
+		types.ApplyPatchType,
+		patch,
+		config.Patch.PatchOptions(patchOptions),
+	); err != nil {
+		if apierrors.IsConflict(err) {
+			return "SUGGESTION: The --force-conflicts flag may help in performing this operation.", err
+		}
+		return "Error requesting update", err
+	}
+
+	return "", err
+}

internal/cmd/backup_test.go

@@ -15,23 +15,33 @@
 package cmd
 
 import (
+	"fmt"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
 	"gotest.tools/v3/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/cli-runtime/pkg/genericclioptions"
+	"k8s.io/client-go/dynamic/fake"
+	k8stesting "k8s.io/client-go/testing"
 	"sigs.k8s.io/yaml"
 
+	"github.com/crunchydata/postgres-operator-client/internal"
+	"github.com/crunchydata/postgres-operator-client/internal/apis/postgres-operator.crunchydata.com/v1beta1"
 	"github.com/crunchydata/postgres-operator-client/internal/testing/cmp"
 )
 
-func TestPGBackRestBackupModifyIntent(t *testing.T) {
+func TestPGBackRestBackupArgsModifyIntent(t *testing.T) {
 	now := time.Date(2020, 4, 5, 6, 7, 8, 99, time.FixedZone("ZONE", -11))
 
 	for _, tt := range []struct {
 		Name, Before, After string
-		Backup              pgBackRestBackup
+		Backup              pgBackRestBackupArgs
 	}{
 		{
 			Name: "Zero",
@@ -43,7 +53,7 @@ metadata:
 		},
 		{
 			Name: "Options",
-			Backup: pgBackRestBackup{
+			Backup: pgBackRestBackupArgs{
 				Options: []string{"--quoth=raven --midnight=dreary", "--ever=never"},
 			},
 			After: strings.TrimSpace(`
@@ -61,7 +71,7 @@ spec:
 		},
 		{
 			Name:   "RepoName",
-			Backup: pgBackRestBackup{RepoName: "testRepo"},
+			Backup: pgBackRestBackupArgs{RepoName: "testRepo"},
 			After: strings.TrimSpace(`
 metadata:
   annotations:
@@ -94,7 +104,7 @@ metadata:
 		},
 		{
 			Name:   "NewRepoButOptions",
-			Backup: pgBackRestBackup{RepoName: "testRepo"},
+			Backup: pgBackRestBackupArgs{RepoName: "testRepo"},
 			Before: strings.TrimSpace(`
 metadata:
   annotations:
@@ -119,7 +129,7 @@ spec:
 		},
 		{
 			Name:   "NewOptionsButRepo",
-			Backup: pgBackRestBackup{Options: []string{"a", "b c"}},
+			Backup: pgBackRestBackupArgs{Options: []string{"a", "b c"}},
 			Before: strings.TrimSpace(`
 metadata:
   annotations:
@@ -160,12 +170,51 @@ spec:
 			[]byte(`{ spec: { backups: 1234 } }`), &intent.Object,
 		))
 
-		err := pgBackRestBackup{Options: []string{"a"}}.modifyIntent(&intent, now)
+		err := pgBackRestBackupArgs{Options: []string{"a"}}.modifyIntent(&intent, now)
 		assert.ErrorContains(t, err, ".spec.backups")
 		assert.ErrorContains(t, err, "is not a map")
 
-		err = pgBackRestBackup{RepoName: "b"}.modifyIntent(&intent, now)
+		err = pgBackRestBackupArgs{RepoName: "b"}.modifyIntent(&intent, now)
 		assert.ErrorContains(t, err, ".spec.backups")
 		assert.ErrorContains(t, err, "is not a map")
 	})
 }
+
+func TestBackupRun(t *testing.T) {
+	cf := genericclioptions.NewConfigFlags(true)
+	nsd := "test"
+	cf.Namespace = &nsd
+	config := &internal.Config{
+		ConfigFlags: cf,
+		IOStreams: genericclioptions.IOStreams{
+			In:     os.Stdin,
+			Out:    os.Stdout,
+			ErrOut: os.Stderr},
+		Patch: internal.PatchConfig{FieldManager: filepath.Base(os.Args[0])},
+	}
+
+	scheme := runtime.NewScheme()
+	client := fake.NewSimpleDynamicClient(scheme)
+	// Set up dynamicResourceClient with `fake` client
+	gvk := v1beta1.GroupVersion.WithKind("PostgresCluster")
+	gvr := schema.GroupVersionResource{Group: gvk.Group, Version: gvk.Version, Resource: "postgresclusters"}
+	drc := client.Resource(gvr)
+
+	t.Run("PassesThroughError", func(t *testing.T) {
+		// Have the client return an error on get
+		client.PrependReactor("get",
+			"postgresclusters",
+			func(action k8stesting.Action) (bool, runtime.Object, error) {
+				return true, nil, fmt.Errorf("whoops")
+			})
+
+		backup := pgBackRestBackupArgs{
+			ClusterName: "name",
+		}
+
+		msg, err := backup.Run(drc, config)
+		assert.Equal(t, "", msg) // No special message is passed through on get fails
+		assert.Error(t, err, "whoops", "Error from PGO API should be passed through")
+	})
+
+}

internal/cmd/restore_test.go

@@ -174,11 +174,11 @@ spec:
 			[]byte(`{ spec: { backups: 1234 } }`), &intent.Object,
 		))
 
-		err := pgBackRestBackup{Options: []string{"a"}}.modifyIntent(&intent, now)
+		err := pgBackRestBackupArgs{Options: []string{"a"}}.modifyIntent(&intent, now)
 		assert.ErrorContains(t, err, ".spec.backups")
 		assert.ErrorContains(t, err, "is not a map")
 
-		err = pgBackRestBackup{RepoName: "b"}.modifyIntent(&intent, now)
+		err = pgBackRestBackupArgs{RepoName: "b"}.modifyIntent(&intent, now)
 		assert.ErrorContains(t, err, ".spec.backups")
 		assert.ErrorContains(t, err, "is not a map")
 	})

testing/kuttl/e2e/backup/README.md

@@ -21,6 +21,7 @@
 * Check the annotation on the cluster
 * No backup occurs
 
-(6) 10-11
+(6) 10-12
 * Update the spec through KUTTL, changing the ownership of that field
 * Call the backup CLI with different flags, and see a conflict
+* Call the backup CLI with different flags and the `--force-conflicts` flag, see no conflict