Add PDB #204

wejdross · 2024-07-24T16:26:49Z

Summary

to prevent downtime caused by software error after maintenance we need PDBs for our HA databases

Checklist

Categorize the PR by setting a good title and adding one of the labels:
bug, enhancement, documentation, change, breaking, dependency
as they show up in the changelog
Update tests.
Link this PR to related issues.

keycload.yaml

pkg/comp-functions/functions/common/interfaces.go

pkg/comp-functions/functions/common/pbp.go

Kidswiss · 2024-07-25T08:32:35Z

pkg/comp-functions/functions/common/pbp.go

+		}
+		pdb, ok := obj.(PDB)
+		if !ok {
+			return runtime.NewWarningResult(fmt.Sprintf("Type %s doesn't implement PDB interface", reflect.TypeOf(obj).String()))


I'd also return a fatal here, as that's an error that could only happen if the XRD is wrongly implemented.

pkg/comp-functions/functions/common/pbp.go

Kidswiss · 2024-07-25T08:45:27Z

pkg/comp-functions/functions/common/pbp.go

+		emptyMap := map[string]string{}
+
+		// to add new case, simply print labels, for example `kubectl -n vshn-mariadb-something-123sa get pod mariadb-something-123sa-0 -o json | jq .metadata.labels` and add the label to the map
+		switch obj.(type) {


This won't scale, ideally the comp should know the necessary labels and return them as part of the PDB interface.

Kidswiss · 2024-07-25T08:45:28Z

pkg/comp-functions/functions/common/pbp.go

+		case *v1.VSHNKeycloak:
+			emptyMap["app.kubernetes.io/name"] = obj.GetLabels()["crossplane.io/composite"]
+		case *v1.VSHNPostgreSQL:
+			emptyMap["stackgres.io/cluster-name"] = obj.GetLabels()["crossplane.io/composite"] + "-pg"


For nested services using obj.GetLabels()["crossplane.io/composite"] will be very problematic as it will always point to the composite belonging to the claim, which might be another service like nextcloud and thus have another name. For example, the -pg appendix you have here only applies to PG instances that are nested with Nextcloud or Keycloak. A normal PostgreSQL instance doesn't have that appendix.

Just use obj.GetName() that will return the right value because obj is our composite anyway.

Co-authored-by: Kidswiss <[email protected]>

pkg/comp-functions/functions/common/pbp.go

zugao · 2024-07-26T07:31:13Z

pkg/comp-functions/functions/common/pdb.go

+			return runtime.NewFatalResult(fmt.Errorf("can't get composite: %w", err))
+		}
+		pdb, ok := comp.(PDB)
+		if !ok {


It would be great to catch this problem at build time and not at runtime. I thought about a solution here but nothing comes into my mind. @Kidswiss maybe you have an idea here?

I tried to think of something during my first review, unfortunately I wasn't able to get a working idea.

I'll have another look now that all my other remarks have been implemented.

Huh... okay it's actually rather simple:

Make the PDB interface also implement client.Object:

type PDB interface { GetInstanceNamespace() string GetInstances() int GetPDBLabels() map[string]string client.Object }

Accept PDB in the AddPDBSettings function:

func AddPDBSettings(comp PDB) func(ctx context.Context, svc *runtime.ServiceRuntime) *fnproto.Result {

Remove the type assert and rename all pdb variables to comp. No more type assertion needed and the compiler will scream at you if you forget to implement the interface.

perfect. Awesome. Thanks for this :)

the whole point I wanted to keep it in different interface was that not all services are ready for HA from the beginning, not all services even have a field instances in their claim. Now it's using InfoGetter{} as mentioned in the beginning and I completely got rid of PDB{} interface

The idea of my comment wasn't to get rid of the PDB interface completely, just reduce the amount of duplication.

not all services even have a field instances in their claim.

In the long run we want all our service to have HA as well.

@wejdross you can use the interface Composite here, it will satisfy all necessary functions. Then you can get rid of the type assertion.

pkg/comp-functions/functions/common/pdb.go

wejdross · 2024-07-26T07:53:53Z

@zugao @Kidswiss I just pushed new commit doing a small cleanup with interfaces and enabling it for Minio. By the way, compiler catches if type implement the interface, I could probably delete that check, I rather want to be double sure here

Kidswiss · 2024-07-26T07:55:45Z

apis/vshn/v1/dbaas_vshn_keycloak.go

@@ -324,3 +324,9 @@ func (v *VSHNKeycloak) GetMonitoring() VSHNMonitoring {
 func (v *VSHNKeycloak) GetInstances() int {
 	return v.Spec.Parameters.Instances
 }
+
+func (v *VSHNKeycloak) GetPDBLabels() map[string]string {


Can you please add this to hack/bootstrap/template/api.txt?

Kidswiss · 2024-07-26T07:57:05Z

@zugao @Kidswiss I just pushed new commit doing a small cleanup with interfaces and enabling it for Minio. By the way, compiler catches if type implement the interface, I could probably delete that check, I rather want to be double sure here

I've added a comment how to make it work: #204 (comment)

zugao · 2024-07-26T08:18:46Z

By the way, compiler catches if type implement the interface

Since you get already the PDB object in the function there's no need to check for anything else at runtime. It should work and in worse case scenario (which should not happen) the application will panick

zugao

Now looks great and clean

zugao · 2024-07-26T13:07:42Z

pkg/comp-functions/functions/common/pdb.go

+		}
+		log.Info("HA detected, adding pdb", "service", comp.GetName())
+
+		min := intstr.IntOrString{IntVal: int32(comp.GetInstances()) / 2}


I'm not sure this will work for uneven numbers.

I get this warning in my IDE for variable min:

Inspection info: Reports declarations of variables, arguments or functions that overlap with the built-in or reserved keyword. If you receive this error then your code might not be as explicit as possible and might confuse other users. Example: type byte struct{} type string interface{} Types byte and string collide with the built-in type aliases. Therefore, they will be highlighted. Consider renaming such declarations Maybe the reason why 50% does not work because of the 2 point?

Kidswiss · 2024-07-26T13:08:06Z

pkg/comp-functions/functions/common/pdb.go

+		}
+		log.Info("HA detected, adding pdb", "service", comp.GetName())
+
+		min := intstr.IntOrString{IntVal: int32(comp.GetInstances()) / 2}


If the string doesn't work, can we at least make sure that we always round up here? Then we'll have the same functionality as using 50%

ok, I found the issue with 50%, Type: intval is default one, when setting a strVal: "50%", You must add explicitly Type: strVal

zugao

Needs a bit more rework

zugao

From my side now it's ok. Please resolve all Simon's concerns and make sure you don't go back to casting approach.

[email protected] added 2 commits July 23, 2024 13:06

pdb init

8a40a79

working PDB

45017fd

wejdross self-assigned this Jul 24, 2024

wejdross requested review from a team, Kidswiss, TheBigLee and zugao and removed request for a team July 24, 2024 16:26

wejdross added the enhancement New feature or request label Jul 24, 2024

Kidswiss requested changes Jul 25, 2024

View reviewed changes

wejdross and others added 3 commits July 25, 2024 11:32

Update pkg/comp-functions/functions/common/pbp.go

1ee2453

Co-authored-by: Kidswiss <[email protected]>

Update pkg/comp-functions/functions/common/pbp.go

e008713

Co-authored-by: Kidswiss <[email protected]>

pdb enhanced

0c8c930

wejdross requested a review from Kidswiss July 25, 2024 15:56

tobru reviewed Jul 26, 2024

View reviewed changes

pkg/comp-functions/functions/common/pbp.go Outdated Show resolved Hide resolved

pdb support

128bb51

wejdross force-pushed the pdb branch from 3e14b91 to 128bb51 Compare July 26, 2024 07:27

zugao reviewed Jul 26, 2024

View reviewed changes

pkg/comp-functions/functions/common/pdb.go Outdated Show resolved Hide resolved

wejdross requested a review from zugao July 26, 2024 07:53

Kidswiss reviewed Jul 26, 2024

View reviewed changes

wejdross force-pushed the pdb branch 2 times, most recently from 1f12158 to ab32f1e Compare July 26, 2024 08:31

wejdross requested review from tobru and Kidswiss July 26, 2024 08:32

pdb support

0a045ae

wejdross force-pushed the pdb branch from ab32f1e to 0a045ae Compare July 26, 2024 08:44

zugao approved these changes Jul 26, 2024

View reviewed changes

[email protected] added 2 commits July 26, 2024 12:13

fixing missing svc getter

2cd9b75

fixing Spec in pdb

511d234

wejdross force-pushed the pdb branch from f1a104d to 511d234 Compare July 26, 2024 13:02

zugao reviewed Jul 26, 2024

View reviewed changes

Kidswiss requested changes Jul 26, 2024

View reviewed changes

zugao self-requested a review July 26, 2024 13:11

zugao requested changes Jul 26, 2024

View reviewed changes

set minAvailability to 50%

9b52fef

zugao approved these changes Jul 26, 2024

View reviewed changes

Kidswiss approved these changes Jul 26, 2024

View reviewed changes

wejdross merged commit e31ca9b into master Jul 26, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PDB #204

Add PDB #204

wejdross commented Jul 24, 2024

Kidswiss Jul 25, 2024

Kidswiss Jul 25, 2024

Kidswiss Jul 25, 2024 •

edited

Loading

zugao Jul 26, 2024

Kidswiss Jul 26, 2024

Kidswiss Jul 26, 2024

zugao Jul 26, 2024 •

edited

Loading

wejdross Jul 26, 2024

Kidswiss Jul 26, 2024

Kidswiss Jul 26, 2024

wejdross commented Jul 26, 2024

Kidswiss Jul 26, 2024

Kidswiss commented Jul 26, 2024

zugao commented Jul 26, 2024

zugao left a comment •

edited

Loading

zugao Jul 26, 2024 •

edited

Loading

Kidswiss Jul 26, 2024

wejdross Jul 26, 2024

zugao left a comment

zugao left a comment

Add PDB #204

Add PDB #204

Conversation

wejdross commented Jul 24, 2024

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kidswiss Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zugao Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wejdross commented Jul 26, 2024

Choose a reason for hiding this comment

Kidswiss commented Jul 26, 2024

zugao commented Jul 26, 2024

zugao left a comment • edited Loading

Choose a reason for hiding this comment

zugao Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zugao left a comment

Choose a reason for hiding this comment

zugao left a comment

Choose a reason for hiding this comment

Kidswiss Jul 25, 2024 •

edited

Loading

zugao Jul 26, 2024 •

edited

Loading

zugao left a comment •

edited

Loading

zugao Jul 26, 2024 •

edited

Loading