Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
21 changes: 15 additions & 6 deletions internal/controller/postgrescluster/pgbackrest.go
Original file line number Diff line number Diff line change
Expand Up @@ -1463,6 +1463,14 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context,
return result, nil
}

// K8SPG-698
sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster)
if err != nil {
log.Error(err, "unable to create replica creation backup")
result.Requeue = true
return result, nil
}

var repoHost *appsv1.StatefulSet
var repoHostName string
// reconcile the pgbackrest repository host
Expand Down Expand Up @@ -1512,12 +1520,13 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context,
}

// reconcile the RBAC required to run pgBackRest Jobs (e.g. for backups)
sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster)
if err != nil {
log.Error(err, "unable to create replica creation backup")
result.Requeue = true
return result, nil
}
// K8SPG-698: it should happen earlier
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we removed comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//sa, err := r.reconcilePGBackRestRBAC(ctx, postgresCluster)
//if err != nil {
// log.Error(err, "unable to create replica creation backup")
// result.Requeue = true
// return result, nil
//}

// reconcile the pgBackRest stanza for all configuration pgBackRest repos
configHashMismatch, err := r.reconcileStanzaCreate(ctx, postgresCluster, instances, configHash)
Expand Down
115 changes: 115 additions & 0 deletions percona/controller/pgcluster/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1622,3 +1622,118 @@ var _ = Describe("Validate TLS", Ordered, func() {
checkSecretProjectionWithCA(cr, cr.Spec.Secrets.CustomReplicationClientTLSSecret, secretName)
})
})

type saTestClient struct {
client.Client

crName string
ns string
}

func (sc *saTestClient) checkObject(ctx context.Context, obj client.Object) error {
sts, ok := obj.(*appsv1.StatefulSet)
if !ok {
return nil
}
serviceAccountName := sts.Spec.Template.Spec.ServiceAccountName
if serviceAccountName == "" {
panic("it's not expected to have empty service account name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since checkObject(...) returns an error and when used we do this

	if err := sc.checkObject(ctx, obj); err != nil {
		panic(err) // should panic because reconciler can ignore the error
	}

I think this check can return an error and let the caller panic it. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

if err := sc.Client.Get(ctx, types.NamespacedName{
Name: serviceAccountName,
Namespace: sts.Namespace,
}, new(corev1.ServiceAccount)); err != nil {
if k8serrors.IsNotFound(err) {
return errors.Wrap(err, "test error: service account should be created before the statefulset")
}
return err
}

return nil
}

func (sc *saTestClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
if err := sc.checkObject(ctx, obj); err != nil {
panic(err) // should panic because reconciler can ignore the error
}
return sc.Client.Update(ctx, obj, opts...)
}

func (sc *saTestClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
if err := sc.checkObject(ctx, obj); err != nil {
panic(err) // should panic because reconciler can ignore the error
}
return sc.Client.Patch(ctx, obj, patch, opts...)
}
Copy link
Contributor

@gkech gkech Apr 14, 2025

Choose a reason for hiding this comment

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

Do we need these? Seems that they are not used in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Crunchy code uses a patch request to create the ServiceAccount instead of a create request:

func (r *Reconciler) apply(ctx context.Context, object client.Object) error {
// Generate an apply-patch by comparing the object to its zero value.
zero := reflect.New(reflect.TypeOf(object).Elem()).Interface()
data, err := client.MergeFrom(zero.(client.Object)).Data(object)
apply := client.RawPatch(client.Apply.Type(), data)
// Keep a copy of the object before any API calls.
intent := object.DeepCopyObject()
patch := kubeapi.NewJSONPatch()
// Send the apply-patch with force=true.
if err == nil {
err = r.patch(ctx, object, apply, client.ForceOwnership)
}
// Some fields cannot be server-side applied correctly. When their outcome
// does not match the intent, send a json-patch to get really specific.
switch actual := object.(type) {
case *corev1.Service:
applyServiceSpec(patch, actual.Spec, intent.(*corev1.Service).Spec, "spec")
}
// Send the json-patch when necessary.
if err == nil && !patch.IsEmpty() {
err = r.patch(ctx, object, patch)
}
I will remove the update part

Copy link
Contributor Author

Choose a reason for hiding this comment

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


func (sc *saTestClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
if err := sc.checkObject(ctx, obj); err != nil {
panic(err) // should panic because reconciler can ignore the error
}
return sc.Client.Create(ctx, obj, opts...)
}

// This test ensures that the ServiceAccount associated with a StatefulSet is created
// before the StatefulSet itself. (K8SPG-698)
// The saTestClient verifies the existence of the ServiceAccount during create, patch,
// or update operations on the StatefulSet.
var _ = Describe("ServiceAccount early creation", Ordered, func() {
ctx := context.Background()

const crName = "sa-timestamp"
const ns = crName

namespace := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: crName,
Namespace: ns,
},
}
crNamespacedName := types.NamespacedName{Name: crName, Namespace: ns}

cr, err := readDefaultCR(crName, ns)
It("should read default cr.yaml", func() {
Expect(err).NotTo(HaveOccurred())
})

cr.Default()
reconciler := reconciler(cr)
crunchyReconciler := crunchyReconciler()

var cl client.Client

BeforeAll(func() {
cl = &saTestClient{
Client: k8sClient,

crName: crName,
ns: ns,
}
reconciler.Client = cl
crunchyReconciler.Client = cl

By("Creating the Namespace to perform the tests")
err := cl.Create(ctx, namespace)
Expect(err).To(Not(HaveOccurred()))
})

AfterAll(func() {
By("Deleting the Namespace to perform the tests")
_ = cl.Delete(ctx, namespace)
})

It("should create PerconaPGCluster", func() {
status := cr.Status
Expect(cl.Create(ctx, cr)).Should(Succeed())
cr.Status = status
Expect(cl.Status().Update(ctx, cr)).Should(Succeed())
})

It("Should reconcile", func() {
_, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: crNamespacedName})
Expect(err).NotTo(HaveOccurred())
_, err = crunchyReconciler.Reconcile(ctx, ctrl.Request{NamespacedName: crNamespacedName})
Expect(err).NotTo(HaveOccurred())
})
})
Loading