Skip to content

Separate postgrescluster.Reconciler client concerns #4256

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Separate postgrescluster.Reconciler client concerns #4256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 1 addition & 18 deletions cmd/postgres-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (
"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/logging"
"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/internal/registration"
"github.com/crunchydata/postgres-operator/internal/tracing"
"github.com/crunchydata/postgres-operator/internal/upgradecheck"
Expand Down Expand Up @@ -256,8 +255,8 @@ func main() {
}

// add all PostgreSQL Operator controllers to the runtime manager
addControllersToManager(manager, log, registrar)
must(pgupgrade.ManagedReconciler(manager, registrar))
must(postgrescluster.ManagedReconciler(manager, registrar))
must(standalone_pgadmin.ManagedReconciler(manager))
must(crunchybridgecluster.ManagedReconciler(manager, func() bridge.ClientInterface {
return bridgeClient()
Expand Down Expand Up @@ -306,19 +305,3 @@ func main() {
log.Info("shutdown complete")
}
}

// addControllersToManager adds all PostgreSQL Operator controllers to the provided controller
// runtime manager.
func addControllersToManager(mgr runtime.Manager, log logging.Logger, reg registration.Registration) {
pgReconciler := &postgrescluster.Reconciler{
Client: mgr.GetClient(),
Owner: naming.ControllerPostgresCluster,
Recorder: mgr.GetEventRecorderFor(naming.ControllerPostgresCluster),
Registration: reg,
}

if err := pgReconciler.SetupWithManager(mgr); err != nil {
log.Error(err, "unable to create PostgresCluster controller")
os.Exit(1)
}
}
8 changes: 4 additions & 4 deletions internal/controller/postgrescluster/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import (
)

// apply sends an apply patch to object's endpoint in the Kubernetes API and
// updates object with any returned content. The fieldManager is set to
// r.Owner and the force parameter is true.
// updates object with any returned content. The fieldManager is set by
// r.Writer and the force parameter is true.
// - https://docs.k8s.io/reference/using-api/server-side-apply/#managers
// - https://docs.k8s.io/reference/using-api/server-side-apply/#conflicts
func (r *Reconciler) apply(ctx context.Context, object client.Object) error {
Expand All @@ -32,7 +32,7 @@ func (r *Reconciler) apply(ctx context.Context, object client.Object) error {

// Send the apply-patch with force=true.
if err == nil {
err = r.patch(ctx, object, apply, client.ForceOwnership)
err = r.Writer.Patch(ctx, object, apply, client.ForceOwnership)
}

// Some fields cannot be server-side applied correctly. When their outcome
Expand All @@ -44,7 +44,7 @@ func (r *Reconciler) apply(ctx context.Context, object client.Object) error {

// Send the json-patch when necessary.
if err == nil && !patch.IsEmpty() {
err = r.patch(ctx, object, patch)
err = r.Writer.Patch(ctx, object, patch)
}
return err
}
Expand Down
33 changes: 19 additions & 14 deletions internal/controller/postgrescluster/apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ func TestServerSideApply(t *testing.T) {
assert.NilError(t, err)

t.Run("ObjectMeta", func(t *testing.T) {
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
cc := client.WithFieldOwner(cc, t.Name())
reconciler := Reconciler{Writer: cc}
constructor := func() *corev1.ConfigMap {
var cm corev1.ConfigMap
cm.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))
Expand All @@ -55,15 +56,15 @@ func TestServerSideApply(t *testing.T) {

// Create the object.
before := constructor()
assert.NilError(t, cc.Patch(ctx, before, client.Apply, reconciler.Owner))
assert.NilError(t, cc.Patch(ctx, before, client.Apply))
assert.Assert(t, before.GetResourceVersion() != "")

// Allow the Kubernetes API clock to advance.
time.Sleep(time.Second)

// client.Apply changes the ResourceVersion inadvertently.
after := constructor()
assert.NilError(t, cc.Patch(ctx, after, client.Apply, reconciler.Owner))
assert.NilError(t, cc.Patch(ctx, after, client.Apply))
assert.Assert(t, after.GetResourceVersion() != "")

switch {
Expand All @@ -87,7 +88,8 @@ func TestServerSideApply(t *testing.T) {
})

t.Run("ControllerReference", func(t *testing.T) {
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
cc := client.WithFieldOwner(cc, t.Name())
reconciler := Reconciler{Writer: cc}

// Setup two possible controllers.
controller1 := new(corev1.ConfigMap)
Expand Down Expand Up @@ -115,7 +117,7 @@ func TestServerSideApply(t *testing.T) {
assert.NilError(t,
controllerutil.SetControllerReference(controller2, applied, cc.Scheme()))

err1 := cc.Patch(ctx, applied, client.Apply, client.ForceOwnership, reconciler.Owner)
err1 := cc.Patch(ctx, applied, client.Apply, client.ForceOwnership)

// Patch not accepted; the ownerReferences field is invalid.
assert.Assert(t, apierrors.IsInvalid(err1), "got %#v", err1)
Expand Down Expand Up @@ -154,20 +156,21 @@ func TestServerSideApply(t *testing.T) {
return &sts
}

reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
cc := client.WithFieldOwner(cc, t.Name())
reconciler := Reconciler{Writer: cc}
upstream := constructor("status-upstream")

// The structs defined in "k8s.io/api/apps/v1" marshal empty status fields.
switch {
case serverVersion.LessThan(version.MustParseGeneric("1.22")):
assert.ErrorContains(t,
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership, reconciler.Owner),
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership),
"field not declared in schema",
"expected https://issue.k8s.io/109210")

default:
assert.NilError(t,
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership, reconciler.Owner))
cc.Patch(ctx, upstream, client.Apply, client.ForceOwnership))
}

// Our apply method generates the correct apply-patch.
Expand All @@ -187,15 +190,16 @@ func TestServerSideApply(t *testing.T) {
}

t.Run("wrong-keys", func(t *testing.T) {
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
cc := client.WithFieldOwner(cc, t.Name())
reconciler := Reconciler{Writer: cc}

intent := constructor("some-selector")
intent.Spec.Selector = map[string]string{"k1": "v1"}

// Create the Service.
before := intent.DeepCopy()
assert.NilError(t,
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))

// Something external mucks it up.
assert.NilError(t,
Expand All @@ -206,7 +210,7 @@ func TestServerSideApply(t *testing.T) {
// client.Apply cannot correct it in old versions of Kubernetes.
after := intent.DeepCopy()
assert.NilError(t,
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))

switch {
case serverVersion.LessThan(version.MustParseGeneric("1.22")):
Expand Down Expand Up @@ -248,15 +252,16 @@ func TestServerSideApply(t *testing.T) {
{"empty", make(map[string]string)},
} {
t.Run(tt.name, func(t *testing.T) {
reconciler := Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
cc := client.WithFieldOwner(cc, t.Name())
reconciler := Reconciler{Writer: cc}

intent := constructor(tt.name + "-selector")
intent.Spec.Selector = tt.selector

// Create the Service.
before := intent.DeepCopy()
assert.NilError(t,
cc.Patch(ctx, before, client.Apply, client.ForceOwnership, reconciler.Owner))
cc.Patch(ctx, before, client.Apply, client.ForceOwnership))

// Something external mucks it up.
assert.NilError(t,
Expand All @@ -267,7 +272,7 @@ func TestServerSideApply(t *testing.T) {
// client.Apply cannot correct it.
after := intent.DeepCopy()
assert.NilError(t,
cc.Patch(ctx, after, client.Apply, client.ForceOwnership, reconciler.Owner))
cc.Patch(ctx, after, client.Apply, client.ForceOwnership))

assert.Assert(t, len(after.Spec.Selector) != len(intent.Spec.Selector),
"got %v", after.Spec.Selector)
Expand Down
50 changes: 23 additions & 27 deletions internal/controller/postgrescluster/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,19 +82,20 @@ func TestCustomLabels(t *testing.T) {
require.ParallelCapacity(t, 2)

reconciler := &Reconciler{
Client: cc,
Owner: client.FieldOwner(t.Name()),
Recorder: new(record.FakeRecorder),
Reader: cc,
Recorder: new(record.FakeRecorder),
StatusWriter: client.WithFieldOwner(cc, t.Name()).Status(),
Writer: client.WithFieldOwner(cc, t.Name()),
}

ns := setupNamespace(t, cc)

reconcileTestCluster := func(cluster *v1beta1.PostgresCluster) {
assert.NilError(t, reconciler.Client.Create(ctx, cluster))
assert.NilError(t, cc.Create(ctx, cluster))
t.Cleanup(func() {
// Remove finalizers, if any, so the namespace can terminate.
assert.Check(t, client.IgnoreNotFound(
reconciler.Client.Patch(ctx, cluster, client.RawPatch(
cc.Patch(ctx, cluster, client.RawPatch(
client.Merge.Type(), []byte(`{"metadata":{"finalizers":[]}}`)))))
})

Expand Down Expand Up @@ -168,7 +169,7 @@ func TestCustomLabels(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -216,7 +217,7 @@ func TestCustomLabels(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -263,7 +264,7 @@ func TestCustomLabels(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -298,7 +299,7 @@ func TestCustomLabels(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand All @@ -320,19 +321,20 @@ func TestCustomAnnotations(t *testing.T) {
require.ParallelCapacity(t, 2)

reconciler := &Reconciler{
Client: cc,
Owner: client.FieldOwner(t.Name()),
Recorder: new(record.FakeRecorder),
Reader: cc,
Recorder: new(record.FakeRecorder),
StatusWriter: client.WithFieldOwner(cc, t.Name()).Status(),
Writer: client.WithFieldOwner(cc, t.Name()),
}

ns := setupNamespace(t, cc)

reconcileTestCluster := func(cluster *v1beta1.PostgresCluster) {
assert.NilError(t, reconciler.Client.Create(ctx, cluster))
assert.NilError(t, cc.Create(ctx, cluster))
t.Cleanup(func() {
// Remove finalizers, if any, so the namespace can terminate.
assert.Check(t, client.IgnoreNotFound(
reconciler.Client.Patch(ctx, cluster, client.RawPatch(
cc.Patch(ctx, cluster, client.RawPatch(
client.Merge.Type(), []byte(`{"metadata":{"finalizers":[]}}`)))))
})

Expand Down Expand Up @@ -407,7 +409,7 @@ func TestCustomAnnotations(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -455,7 +457,7 @@ func TestCustomAnnotations(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -502,7 +504,7 @@ func TestCustomAnnotations(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand Down Expand Up @@ -537,7 +539,7 @@ func TestCustomAnnotations(t *testing.T) {
for _, gvk := range gvks {
uList := &unstructured.UnstructuredList{}
uList.SetGroupVersionKind(gvk)
assert.NilError(t, reconciler.Client.List(ctx, uList,
assert.NilError(t, cc.List(ctx, uList,
client.InNamespace(cluster.Namespace),
client.MatchingLabelsSelector{Selector: selector}))

Expand All @@ -554,10 +556,7 @@ func TestCustomAnnotations(t *testing.T) {
}

func TestGenerateClusterPrimaryService(t *testing.T) {
_, cc := setupKubernetes(t)
require.ParallelCapacity(t, 0)

reconciler := &Reconciler{Client: cc}
reconciler := &Reconciler{}

cluster := &v1beta1.PostgresCluster{}
cluster.Namespace = "ns2"
Expand Down Expand Up @@ -658,7 +657,7 @@ func TestReconcileClusterPrimaryService(t *testing.T) {
_, cc := setupKubernetes(t)
require.ParallelCapacity(t, 1)

reconciler := &Reconciler{Client: cc, Owner: client.FieldOwner(t.Name())}
reconciler := &Reconciler{Writer: client.WithFieldOwner(cc, t.Name())}

cluster := testCluster()
cluster.Namespace = setupNamespace(t, cc).Name
Expand All @@ -676,10 +675,7 @@ func TestReconcileClusterPrimaryService(t *testing.T) {
}

func TestGenerateClusterReplicaServiceIntent(t *testing.T) {
_, cc := setupKubernetes(t)
require.ParallelCapacity(t, 0)

reconciler := &Reconciler{Client: cc}
reconciler := &Reconciler{}

cluster := &v1beta1.PostgresCluster{}
cluster.Namespace = "ns1"
Expand Down
Loading
Loading