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.

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

Tablespace Implementation #3575

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

benjaminjb
Copy link
Contributor

@benjaminjb benjaminjb commented Feb 14, 2023

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature
  • Bug fix
  • Documentation
  • Testing enhancement
  • Other

What is the current behavior (link to any open issues here)?

Tablespaces were not implemented by the operator

What is the new behavior (if this is a feature change)?

  • Breaking change (fix or feature that would cause existing functionality to change)

Implements and documents a tablespaces feature, which is behind a feature flag.

Other Information:
Issue: [sc-17759]

@benjaminjb benjaminjb changed the title Tablespace initial Tablespace Implementation (alpha) Feb 14, 2023
@benjaminjb benjaminjb force-pushed the tablespace-initial branch 3 times, most recently from 30e7a63 to fb0876e Compare February 14, 2023 23:05
@benjaminjb benjaminjb marked this pull request as ready for review February 14, 2023 23:05
@@ -1086,11 +1087,14 @@ func (r *Reconciler) reconcileInstance(
if err == nil {
postgresWALVolume, err = r.reconcilePostgresWALVolume(ctx, cluster, spec, instance, observed, clusterVolumes)
}
if err == nil {
tablespaceVolumes, err = r.reconcileTablespaceVolumes(ctx, cluster, spec, instance, clusterVolumes)
}
if err == nil {
postgres.InstancePod(
Copy link
Contributor

@tony-landreth tony-landreth Feb 15, 2023

Choose a reason for hiding this comment

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

❓ Should this function start taking a parameter struct to bring the number of arguments down? Same question for reconcileRestoreJob in internal/controller/postgrescluster/pgbackrest.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not feeling that as a necessity at this moment, but would definitely consider it in the future

@benjaminjb benjaminjb force-pushed the tablespace-initial branch 6 times, most recently from d283faf to 499d3c4 Compare February 17, 2023 18:27
internal/postgres/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tony-landreth tony-landreth left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

internal/controller/postgrescluster/pgbackrest.go Outdated Show resolved Hide resolved
internal/controller/postgrescluster/pgbackrest.go Outdated Show resolved Hide resolved
@@ -561,6 +561,78 @@ func (r *Reconciler) reconcilePostgresDataVolume(
return pvc, err
}

// +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=create;patch
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ I prefer this syntax because it aligns with how our hack script works. There's no functional difference.

Suggested change
// +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=create;patch
// +kubebuilder:rbac:groups="",resources="persistentvolumeclaims",verbs={create,patch}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth changing these in the few places it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we have a bunch of annotations that follow the first syntax, maybe it's worth a whole separate PR to bring it into line?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. No need to touch lots of stuff in this PR.

internal/controller/postgrescluster/pgbackrest.go Outdated Show resolved Hide resolved
internal/naming/names.go Outdated Show resolved Hide resolved
internal/pgbackrest/reconcile.go Outdated Show resolved Hide resolved
internal/postgres/reconcile.go Outdated Show resolved Hide resolved
@benjaminjb benjaminjb force-pushed the tablespace-initial branch 3 times, most recently from 9f9d635 to bf4ca05 Compare February 23, 2023 17:47
* Adds the tablespaceVolumes field to the CRD;
* Adds basic tablespace functionality: mounts the volumes and
preps them with correct permissions;
* Adds option for restoring with tablespaces;
* Adds docs/content/guides/tablespaces
* Adds a basic KUTTL test for creating a cluster with tablespaces;
* Updates the github test to add the feature gate

Issue: [sc-17759]
@benjaminjb benjaminjb merged commit a266886 into CrunchyData:master Feb 23, 2023
@andrewlecuyer andrewlecuyer changed the title Tablespace Implementation (alpha) Tablespace Implementation Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants