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

Extend Operator to PSQL-HA #199

Open
wants to merge 72 commits into
base: main
Choose a base branch
from

Conversation

justinorringer
Copy link

What this PR does / why we need it:
Provision and clone postgres-ha databases. Continues work from another semester, reading in k8s resources to create that database (and proxy).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

How Has This Been Tested?:
Unit tests run correctly to ensure no breaking changes.
We are working on the end-to-end tests.

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Adds provisioning of postgres-HA instances.

Copy link
Contributor

@mazin-s mazin-s left a comment

Choose a reason for hiding this comment

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

Left some comments.

Overall, make sure the logic you are adding in provisioning and cloning are consistent. There's a few helper functions you add in db_helpers.go and clone_helpers.go. Try to anyalze what's needed in both helpers and consolidate them into some sort of syntatic sugar paradigm (as seen with the requestAppenders) workflow. Also, you'll need to move some of that logic and add to a new file in utils/common as indicated in one of my comments so that you can use that function in webhooks. Look at how the defaulting and validation of additional arguments is done both in webhooks and in the regular code flow and it should give you an idea. Really great work stuff. I'll be available most of this weekend for questions. let me know if you want to block a few hours so we can work on this together!

api/v1alpha1/webhook_suite_test.go Show resolved Hide resolved
// +optional
Nodes []*Node `json:"nodes,omitempty"`
}
type Node struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to validate this in web hooks. e.g validate vmNames being unique, properties correctly defined, etc.

api/v1alpha1/database_types.go Show resolved Hide resolved
common/util/additionalArguments.go Outdated Show resolved Hide resolved
controller_adapters/database.go Show resolved Hide resolved
ndb_api/db_helpers.go Show resolved Hide resolved
ndb_api/clone_helpers.go Outdated Show resolved Hide resolved
return nil
}

func createDefaultNodes(database DatabaseInterface) []*v1alpha1.Node {
Copy link
Contributor

@mazin-s mazin-s Apr 19, 2024

Choose a reason for hiding this comment

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

Recall that we want to include this in webhooks. In that package we use things from the common package we don't want to use things from this package. So move this logic tocommon/util/haHelpers.go (maybe that can be the file name) where you specify the DEFAULTING logic so it can be used here and in web-hooks. Also think about what the defaulting logic would look like in case of cloning and provisioning.

Choose a reason for hiding this comment

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

created the haHelpers and moved this method to that

ndb_api/db_helpers.go Outdated Show resolved Hide resolved
ndb_api/db_helpers.go Show resolved Hide resolved
@justinorringer
Copy link
Author

@mazin-s Cody messaged you, but since the database_types and webhooks are coupled in the same package, refactoring these methods into utils/common causes a circular dependency.

justinorringer and others added 30 commits April 22, 2024 13:40
Updated required nodes to 3 database nodes
end to end test with custom test case
Revert "end to end test with custom test case"
* bumped to gcr.io/kubebuilder/kube-rbac-proxy:v0.16.0 from v0.15.0 to fix security vulnerability

* bumped to v0.5.2

* bumped up to 0.5.1
end to end test with custom test case
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.

5 participants