PR Feedback

Issue: [sc-17759]

Author: benjaminjb

Makefile

@@ -138,6 +138,7 @@ deploy-dev: createnamespaces
 	hack/create-kubeconfig.sh postgres-operator pgo
 	env \
 		CRUNCHY_DEBUG=true \
+		PGO_FEATURE_GATES="TablespaceVolumes=true" \
 		CHECK_FOR_UPGRADES='$(if $(CHECK_FOR_UPGRADES),$(CHECK_FOR_UPGRADES),false)' \
 		KUBECONFIG=hack/.kube/postgres-operator/pgo \
 		PGO_NAMESPACE='postgres-operator' \

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

@@ -9645,12 +9645,17 @@ spec:
                             description: The name for the tablespace, used as the
                               path name for the volume. Must be unique in the instance
                               set since they become the directory names.
+                            minLength: 1
+                            pattern: ^[a-z][a-z0-9]*$
                             type: string
                         required:
                         - dataVolumeClaimSpec
                         - name
                         type: object
                       type: array
+                      x-kubernetes-list-map-keys:
+                      - name
+                      x-kubernetes-list-type: map
                     tolerations:
                       description: 'Tolerations of a PostgreSQL pod. Changing this
                         value causes PostgreSQL to restart. More info: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration'

docs/content/guides/tablespaces.md

@@ -29,7 +29,7 @@ distributed system, there are several considerations to ensure proper operations
 
 - Each tablespace must have its own volume; this means that every tablespace for
 every replica in a system must have its own volume;
-- The filesystem map must be consistent across the cluster;
+- The available filesystem paths must be consistent on each Postgres pod in a Postgres cluster;
 - The backup & disaster recovery management system must be able to safely backup
 and restore data to tablespaces.
 
@@ -51,7 +51,7 @@ PGO feature gates are enabled by setting the `PGO_FEATURE_GATES` environment
 variable on the PGO Deployment. To enable tablespaces, you would want to set
 
 ```
-PGO_FEATURE_GATES="TablespaveVolumes=true"
+PGO_FEATURE_GATES="TablespaceVolumes=true"
 ```
 
 Please note that it is possible to enable more than one feature at a time as
@@ -62,7 +62,7 @@ you would set `PGO_FEATURE_GATES` like so:
 PGO_FEATURE_GATES="FeatureName=true,FeatureName2=true,FeatureName3=true..."
 ```
 
-## Using TablespaceVolumes in PGO v5
+## Adding TablespaceVolumes to a postgrescluster in PGO v5
 
 Once you have enabled `TablespaceVolumes` on your PGO deployment, you can add volumes to
 a new or existing cluster by adding volumes to the `spec.instances.tablespaceVolumes` field.
@@ -122,8 +122,12 @@ spec:
           volumeName: pvc-3fea1531-617a-4fff-9032-6487206ce644 # A preexisting volume you want to use for this tablespace
 ```
 
-Note: the `name` of the `tablespaceVolume` needs to be unique in the instance since
-that name becomes part of the mount path for that volume.
+Note: the `name` of the `tablespaceVolume` needs to be
+
+* unique in the instance since that name becomes part of the mount path for that volume;
+* valid as part of a path name, label, and part of a volume name.
+
+There is validation on the CRD for these requirements.
 
 Once you request those `tablespaceVolumes`, PGO takes care of creating (or reusing) those volumes,
 including mounting them to the pod at a known path (`/tablespaces/NAME`) and adding them to the

internal/controller/postgrescluster/pgbackrest.go

@@ -50,7 +50,6 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/pgbackrest"
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
-	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -1118,21 +1117,19 @@ func (r *Reconciler) reconcileRestoreJob(ctx context.Context,
 		volumeMounts = append(volumeMounts, walVolumeMount)
 	}
 
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) {
-		for _, pgtablespaceVolume := range pgtablespaceVolumes {
-			tablespaceVolumeMount := postgres.TablespaceVolumeMount(
-				pgtablespaceVolume.Labels["postgres-operator.crunchydata.com/data"])
-			tablespaceVolume := corev1.Volume{
-				Name: tablespaceVolumeMount.Name,
-				VolumeSource: corev1.VolumeSource{
-					PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
-						ClaimName: pgtablespaceVolume.GetName(),
-					},
+	for _, pgtablespaceVolume := range pgtablespaceVolumes {
+		tablespaceVolumeMount := postgres.TablespaceVolumeMount(
+			pgtablespaceVolume.Labels[naming.LabelData])
+		tablespaceVolume := corev1.Volume{
+			Name: tablespaceVolumeMount.Name,
+			VolumeSource: corev1.VolumeSource{
+				PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+					ClaimName: pgtablespaceVolume.GetName(),
 				},
-			}
-			volumes = append(volumes, tablespaceVolume)
-			volumeMounts = append(volumeMounts, tablespaceVolumeMount)
+			},
 		}
+		volumes = append(volumes, tablespaceVolume)
+		volumeMounts = append(volumeMounts, tablespaceVolumeMount)
 	}
 
 	restoreJob := &batchv1.Job{}
@@ -1528,12 +1525,9 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context,
 		return errors.WithStack(err)
 	}
 
-	pgtablespaces := []*corev1.PersistentVolumeClaim{}
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) {
-		pgtablespaces, err = r.reconcileTablespaceVolumes(ctx, cluster, instanceSet, fakeSTS, clusterVolumes)
-		if err != nil {
-			return errors.WithStack(err)
-		}
+	pgtablespaces, err := r.reconcileTablespaceVolumes(ctx, cluster, instanceSet, fakeSTS, clusterVolumes)
+	if err != nil {
+		return errors.WithStack(err)
 	}
 
 	// reconcile the pgBackRest restore Job to populate the cluster's data directory
@@ -1626,14 +1620,12 @@ func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context,
 		return errors.WithStack(err)
 	}
 
-	// TODO(benjaminjb): do we really need this?
-	pgtablespaces := []*corev1.PersistentVolumeClaim{}
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) {
-		pgtablespaces, err = r.reconcileTablespaceVolumes(ctx, cluster, instanceSet, fakeSTS, clusterVolumes)
-		if err != nil {
-			return errors.WithStack(err)
-		}
+	// TODO(benjaminjb): do we really need this for cloud-based datasources?
+	pgtablespaces, err := r.reconcileTablespaceVolumes(ctx, cluster, instanceSet, fakeSTS, clusterVolumes)
+	if err != nil {
+		return errors.WithStack(err)
 	}
+
 	// The `reconcileRestoreJob` was originally designed to take a PostgresClusterDataSource
 	// and rather than reconfigure that func's signature, we translate the PGBackRestDataSource
 	tmpDataSource := &v1beta1.PostgresClusterDataSource{

internal/naming/names.go

@@ -325,8 +325,6 @@ func InstancePostgresDataVolume(instance *appsv1.StatefulSet) metav1.ObjectMeta
 	}
 }
 
-// TODO(benjaminjb): We need to consider shortening the PVC name
-// or at least need to document so that people can avoid too-long names
 // InstanceTablespaceDataVolume returns the ObjectMeta for the tablespace data
 // volume for instance.
 func InstanceTablespaceDataVolume(instance *appsv1.StatefulSet, tablespaceName string) metav1.ObjectMeta {

internal/pgbackrest/config.go

@@ -27,7 +27,6 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
-	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -207,12 +206,10 @@ func RestoreCommand(pgdata string, tablespaceVolumes []*corev1.PersistentVolumeC
 	// are no timeouts when starting or stopping Postgres.
 
 	tablespaceCmd := ""
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) {
-		for _, tablespaceVolume := range tablespaceVolumes {
-			tablespaceCmd = tablespaceCmd + fmt.Sprintf(
-				"\ninstall --directory --mode=0700 '/tablespaces/%s/data'",
-				tablespaceVolume.Labels["postgres-operator.crunchydata.com/data"])
-		}
+	for _, tablespaceVolume := range tablespaceVolumes {
+		tablespaceCmd = tablespaceCmd + fmt.Sprintf(
+			"\ninstall --directory --mode=0700 '/tablespaces/%s/data'",
+			tablespaceVolume.Labels[naming.LabelData])
 	}
 
 	restoreScript := `declare -r pgdata="$1" opts="$2"

internal/pgbackrest/config_test.go

@@ -32,7 +32,6 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
-	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -294,7 +293,6 @@ func TestReloadCommandPrettyYAML(t *testing.T) {
 func TestRestoreCommand(t *testing.T) {
 	shellcheck := require.ShellCheck(t)
 
-	assert.NilError(t, util.AddAndSetFeatureGates(string(util.TablespaceVolumes+"=false")))
 	pgdata := "/pgdata/pg13"
 	opts := []string{
 		"--stanza=" + DefaultStanzaName, "--pg1-path=" + pgdata,
@@ -314,7 +312,6 @@ func TestRestoreCommand(t *testing.T) {
 }
 
 func TestRestoreCommandPrettyYAML(t *testing.T) {
-	assert.NilError(t, util.AddAndSetFeatureGates(string(util.TablespaceVolumes+"=false")))
 	b, err := yaml.Marshal(RestoreCommand("/dir", nil, "--options"))
 	assert.NilError(t, err)
 	assert.Assert(t, strings.Contains(string(b), "\n- |"),

internal/pgbackrest/reconcile.go

@@ -307,7 +307,7 @@ func addServerContainerAndVolume(
 		for _, instance := range cluster.Spec.InstanceSets {
 			for _, vol := range instance.TablespaceVolumes {
 				tablespaceVolumeMount := postgres.TablespaceVolumeMount(vol.Name)
-				postgresMounts[postgres.TablespaceVolumeMount(vol.Name).Name] = tablespaceVolumeMount
+				postgresMounts[tablespaceVolumeMount.Name] = tablespaceVolumeMount
 			}
 		}
 	}

internal/pgbackrest/reconcile_test.go

@@ -663,7 +663,7 @@ func TestAddServerToInstancePod(t *testing.T) {
 		}
 
 		out := pod.DeepCopy()
-		out.Volumes = append(out.Volumes, corev1.Volume{Name: "trial"}, corev1.Volume{Name: "castle"})
+		out.Volumes = append(out.Volumes, corev1.Volume{Name: "tablespace-trial"}, corev1.Volume{Name: "tablespace-castle"})
 		AddServerToInstancePod(clusterWithTablespaces, out, "instance-secret-name")
 
 		// Only Containers and Volumes fields have changed.
@@ -700,9 +700,9 @@ func TestAddServerToInstancePod(t *testing.T) {
   - mountPath: /pgwal
     name: postgres-wal
   - mountPath: /tablespaces/trial
-    name: trial
+    name: tablespace-trial
   - mountPath: /tablespaces/castle
-    name: castle
+    name: tablespace-castle
 - command:
   - bash
   - -ceu

internal/postgres/config.go

@@ -208,26 +208,27 @@ func startupCommand(
 	// If the user requests tablespaces, we want to make sure the directories exist with the
 	// correct owner and permissions.
 	tablespaceCmd := ""
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) && instance.TablespaceVolumes != nil {
+	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) {
 		// This command checks if a dir exists and if not, creates it;
 		// if the dir does exist, then we `recreate` it to make sure the owner is correct;
-		// but if the dir exists with the wrong owner and is not writeable, we error.
+		// if the dir exists with the wrong owner and is not writeable, we error.
 		// This is the same behavior we use for the main PGDATA directory.
 		// Note: Postgres requires the tablespace directory to be "an existing, empty directory
 		// that is owned by the PostgreSQL operating system user."
 		// - https://www.postgresql.org/docs/current/manage-ag-tablespaces.html
 		// However, unlike the PGDATA directory, Postgres will _not_ error out
 		// if the permissions are wrong on the tablespace directory.
-		// Instead, when a tablespace is created in Postgres, Postgres will
-		// chmod the tablespace directory to match permissions on the PGDATA directory,
-		// i.e., 700 or 750
-		// Postgres setting the directory permissions:
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/tablespace.c;h=5411638696b4d1436a56f62e9cde7ec61797377b;hb=REL_14_0#l604
-		// Postgres setting directory permissions:
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/common/file_perm.c;h=ab85765a0bfb5108265819a4c7e9ff87dc6add10;hb=REL_14_0#l34
-		// Note: This permission change seems to only happen when the tablespace is created,
-		// i.e., if a user `chmod`'ed the directory after the creation of the tablespace,
-		// Postgres would not revert that
+		// Instead, when a tablespace is created in Postgres, Postgres will `chmod` the
+		// tablespace directory to match permissions on the PGDATA directory (either 700 or 750).
+		// Postgres setting the tablespace directory permissions:
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/commands/tablespace.c;hb=REL_14_0#l600
+		// Postgres choosing directory permissions:
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/common/file_perm.c;hb=REL_14_0#l27
+		// Note: This permission change seems to happen only when the tablespace is created in Postgres.
+		// If the user manually `chmod`'ed the directory after the creation of the tablespace, Postgres
+		// would not attempt to change the directory permissions.
+		// Note: as noted below, we mount the tablespace directory to the mountpoint `/tablespaces/NAME`,
+		// and so we add the subdirectory `data` in order to set the permissions.
 		checkInstallRecreateCmd := strings.Join([]string{
 			`if [[ ! -e "${tablespace_dir}" || -O "${tablespace_dir}" ]]; then`,
 			`install --directory --mode=0700 "${tablespace_dir}"`,

internal/postgres/reconcile.go

@@ -40,7 +40,7 @@ func DataVolumeMount() corev1.VolumeMount {
 
 // TablespaceVolumeMount returns the name and mount path of the PostgreSQL tablespace data volume.
 func TablespaceVolumeMount(tablespaceName string) corev1.VolumeMount {
-	return corev1.VolumeMount{Name: tablespaceName, MountPath: tablespaceMountPath + "/" + tablespaceName}
+	return corev1.VolumeMount{Name: "tablespace-" + tablespaceName, MountPath: tablespaceMountPath + "/" + tablespaceName}
 }
 
 // WALVolumeMount returns the name and mount path of the PostgreSQL WAL volume.
@@ -224,25 +224,23 @@ func InstancePod(ctx context.Context,
 		downwardAPIVolume,
 	}
 
-	// If `TablespaceVolumes` FeatureGate is enabled, add any tablespace volumes to the pod, and
+	// If `TablespaceVolumes` FeatureGate is enabled, `inTablespaceVolumes` may not be nil.
+	// In that case, add any tablespace volumes to the pod, and
 	// add volumeMounts to the database and startup containers
-	if util.DefaultMutableFeatureGate.Enabled(util.TablespaceVolumes) &&
-		inTablespaceVolumes != nil {
-		for _, vol := range inTablespaceVolumes {
-			tablespaceVolumeMount := TablespaceVolumeMount(vol.Labels["postgres-operator.crunchydata.com/data"])
-			tablespaceVolume := corev1.Volume{
-				Name: tablespaceVolumeMount.Name,
-				VolumeSource: corev1.VolumeSource{
-					PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
-						ClaimName: vol.Name,
-						ReadOnly:  false,
-					},
+	for _, vol := range inTablespaceVolumes {
+		tablespaceVolumeMount := TablespaceVolumeMount(vol.Labels[naming.LabelData])
+		tablespaceVolume := corev1.Volume{
+			Name: tablespaceVolumeMount.Name,
+			VolumeSource: corev1.VolumeSource{
+				PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
+					ClaimName: vol.Name,
+					ReadOnly:  false,
 				},
-			}
-			outInstancePod.Volumes = append(outInstancePod.Volumes, tablespaceVolume)
-			container.VolumeMounts = append(container.VolumeMounts, tablespaceVolumeMount)
-			startup.VolumeMounts = append(startup.VolumeMounts, tablespaceVolumeMount)
+			},
 		}
+		outInstancePod.Volumes = append(outInstancePod.Volumes, tablespaceVolume)
+		container.VolumeMounts = append(container.VolumeMounts, tablespaceVolumeMount)
+		startup.VolumeMounts = append(startup.VolumeMounts, tablespaceVolumeMount)
 	}
 
 	if len(inCluster.Spec.Config.Files) != 0 {

internal/postgres/reconcile_test.go

@@ -63,7 +63,7 @@ func TestTablespaceVolumeMount(t *testing.T) {
 	mount := TablespaceVolumeMount("trial")
 
 	assert.DeepEqual(t, mount, corev1.VolumeMount{
-		Name:      "trial",
+		Name:      "tablespace-trial",
 		MountPath: "/tablespaces/trial",
 		ReadOnly:  false,
 	})
@@ -546,7 +546,6 @@ volumes:
 		}
 		tablespaceVolumes := []*corev1.PersistentVolumeClaim{tablespaceVolume1, tablespaceVolume2}
 
-		assert.NilError(t, util.AddAndSetFeatureGates(string(util.TablespaceVolumes+"=true")))
 		InstancePod(ctx, cluster, instance,
 			serverSecretProjection, clientSecretProjection, dataVolume, nil, tablespaceVolumes, pod)
 
@@ -560,9 +559,9 @@ volumes:
   name: database-containerinfo
   readOnly: true
 - mountPath: /tablespaces/castle
-  name: castle
+  name: tablespace-castle
 - mountPath: /tablespaces/trial
-  name: trial`), "expected tablespace mount(s) in %q container", pod.Containers[0].Name)
+  name: tablespace-trial`), "expected tablespace mount(s) in %q container", pod.Containers[0].Name)
 
 		// InitContainer has all mountPaths, except downwardAPI and additionalConfig
 		assert.Assert(t, marshalMatches(pod.InitContainers[0].VolumeMounts, `
@@ -572,9 +571,9 @@ volumes:
 - mountPath: /pgdata
   name: postgres-data
 - mountPath: /tablespaces/castle
-  name: castle
+  name: tablespace-castle
 - mountPath: /tablespaces/trial
-  name: trial`), "expected tablespace mount(s) in %q container", pod.InitContainers[0].Name)
+  name: tablespace-trial`), "expected tablespace mount(s) in %q container", pod.InitContainers[0].Name)
 	})
 
 	t.Run("WithWALVolumeWithWALVolumeSpec", func(t *testing.T) {

pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go

@@ -481,14 +481,26 @@ type PostgresInstanceSetSpec struct {
 
 	// The list of tablespaces volumes to mount for this postgrescluster
 	// This function is currently in alpha as of PGO v5.4
+	// +listType=map
+	// +listMapKey=name
 	// +optional
 	TablespaceVolumes []TablespaceVolume `json:"tablespaceVolumes,omitempty"`
 }
 
 type TablespaceVolume struct {
+	// This value goes into
+	// a. the name of a corev1.PersistentVolumeClaim,
+	// b. a label value, and
+	// c. a path name.
+	// So it must match both IsDNS1123Subdomain and IsValidLabelValue;
+	// and be valid as a file path.
+
 	// The name for the tablespace, used as the path name for the volume.
 	// Must be unique in the instance set since they become the directory names.
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:Pattern=`^[a-z][a-z0-9]*$`
+	// +kubebuilder:validation:Type=string
 	Name string `json:"name"`
 
 	// Defines a PersistentVolumeClaim for a tablespace.

testing/kuttl/e2e/tablespace-enabled/01--psql-connect.yaml

@@ -41,3 +41,5 @@ spec:
               CREATE SCHEMA "tablespace-enabled";
               CREATE TABLE important (data) TABLESPACE trial AS VALUES ('treasure');
               CREATE TABLE also_important (data) TABLESPACE castle AS VALUES ('treasure');
+              CREATE TABLE moving_important (data) AS VALUES ('treasure');
+              ALTER TABLE moving_important SET TABLESPACE trial;