K8SPG-698: create serviceaccount before statefulset #1125

pooknull · 2025-04-09T21:07:57Z

https://perconadev.atlassian.net/browse/K8SPG-698

DESCRIPTION

Problem:
The cluster1-repo-host StatefulSet is created before its corresponding ServiceAccount cluster1-pgbackrest exists. This can cause failures in certain environments.

Solution:
Create ServiceAccount before the StatefulSet.

CHECKLIST

Jira

Is the Jira ticket created and referenced properly?
Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

Is an E2E test/test case added for the new feature/change?
Are unit tests added where appropriate?

Config/Logging/Testability

Are all needed new/changed options added to default YAML files?
Are all needed new/changed options added to the Helm Chart?
Did we add proper logging messages for operator actions?
Did we ensure compatibility with the previous version or cluster upgrade process?
Does the change support oldest and newest supported PG version?
Does the change support oldest and newest supported Kubernetes version?

https://perconadev.atlassian.net/browse/K8SPG-698

hors · 2025-04-10T16:10:49Z

internal/controller/postgrescluster/pgbackrest.go

-		result.Requeue = true
-		return result, nil
-	}
+	// K8SPG-698: it should happen earlier


can we removed comments

gkech · 2025-04-14T08:43:46Z

percona/controller/pgcluster/controller_test.go

+	}
+	serviceAccountName := sts.Spec.Template.Spec.ServiceAccountName
+	if serviceAccountName == "" {
+		panic("it's not expected to have empty service account name")


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?

gkech · 2025-04-14T09:03:13Z

percona/controller/pgcluster/controller_test.go

+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...)
+}


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

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

percona-postgresql-operator/internal/controller/postgrescluster/apply.go

Lines 23 to 48 in 5dcd0e0

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

gkech · 2025-04-15T08:17:34Z

seems ok, let's check the golangci and the envtests why they are failing and then we can approve.

JNKPercona · 2025-04-15T09:59:53Z

Test name	Status
backup-enable-disable	passed
custom-extensions	passed
custom-tls	passed
demand-backup	passed
finalizers	passed
init-deploy	passed
monitoring	passed
monitoring-pmm3	passed
one-pod	passed
operator-self-healing	passed
pitr	passed
scaling	passed
scheduled-backup	passed
self-healing	passed
sidecars	passed
start-from-backup	passed
tablespaces	passed
telemetry-transfer	passed
upgrade-consistency	passed
upgrade-minor	passed
users	passed
We run 21 out of 21

commit: f24206a
image: perconalab/percona-postgresql-operator:PR-1125-f24206a27

pooknull added 2 commits April 10, 2025 00:07

K8SPG-698: create serviceaccount before statefulset

af4977a

https://perconadev.atlassian.net/browse/K8SPG-698

Merge remote-tracking branch 'origin/main' into K8SPG-698

5dcd0e0

pooknull marked this pull request as ready for review April 9, 2025 21:32

hors requested review from egegunes and gkech April 10, 2025 16:10

hors requested changes Apr 10, 2025

View reviewed changes

gkech reviewed Apr 14, 2025

View reviewed changes

pooknull added 2 commits April 14, 2025 16:09

small fixes

f1f4ce3

Merge remote-tracking branch 'origin/main' into K8SPG-698

3497874

pooknull requested a review from nmarukovich as a code owner April 14, 2025 13:10

pooknull and others added 2 commits April 14, 2025 16:10

remove update

da937df

Merge branch 'main' into K8SPG-698

d58f029

hors added this to the 2.7.0 milestone Apr 14, 2025

Merge branch 'main' into K8SPG-698

5eba903

fix lint

f24206a

pooknull requested review from hors and gkech April 15, 2025 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

K8SPG-698: create serviceaccount before statefulset #1125

K8SPG-698: create serviceaccount before statefulset #1125

pooknull commented Apr 9, 2025 •

edited

Loading

hors Apr 10, 2025

pooknull Apr 14, 2025

gkech Apr 14, 2025

pooknull Apr 14, 2025

gkech Apr 14, 2025 •

edited

Loading

pooknull Apr 14, 2025

pooknull Apr 14, 2025

gkech commented Apr 15, 2025

JNKPercona commented Apr 15, 2025

	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)
	}

K8SPG-698: create serviceaccount before statefulset #1125

Are you sure you want to change the base?

K8SPG-698: create serviceaccount before statefulset #1125

Conversation

pooknull commented Apr 9, 2025 • edited Loading

DESCRIPTION

CHECKLIST

hors Apr 10, 2025

Choose a reason for hiding this comment

pooknull Apr 14, 2025

Choose a reason for hiding this comment

gkech Apr 14, 2025

Choose a reason for hiding this comment

pooknull Apr 14, 2025

Choose a reason for hiding this comment

gkech Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

pooknull Apr 14, 2025

Choose a reason for hiding this comment

pooknull Apr 14, 2025

Choose a reason for hiding this comment

gkech commented Apr 15, 2025

JNKPercona commented Apr 15, 2025

pooknull commented Apr 9, 2025 •

edited

Loading

gkech Apr 14, 2025 •

edited

Loading