-
Notifications
You must be signed in to change notification settings - Fork 0
feat: enable replicas to use replication slots #47
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
base: release-v1.16.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Piotr Kopec <[email protected]>
…ication slots Signed-off-by: Piotr Kopec <[email protected]>
|
||
type Repository interface { | ||
CreateSlot(ctx context.Context, name string) (replicationslot.ReplicationSlot, error) | ||
FindSlotByName(ctx context.Context, name string) (replicationslot.ReplicationSlot, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit:
FindSlotByName(ctx context.Context, name string) (replicationslot.ReplicationSlot, error) | |
GetSlot(ctx context.Context, name string) (replicationslot.ReplicationSlot, error) |
I think name
is the ID of the slot, so we can make it easier to understand this API.
If we add some fins slots by other filters, then we can add Find*
methods.
Signed-off-by: Piotr Kopec <[email protected]>
79232df
to
e2265f2
Compare
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergicastro annotated code, let me know if something is not clear.
replicationSlotsCreateDeleter: noopReplicationSlotsCreateDeleter{}, | ||
} | ||
|
||
// TODO(piotrkpc): should this be here ? not testable code really |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also does not looks great as the direct sql/postgres dependencies are here because of this. Maybe we can figure out better way to setup db connections.
} | ||
|
||
// TODO(piotrkpc): @Sergi, this is likely to be changed to something more sophisticated that would enable ssl enabled connections | ||
func newConnectorConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to figure out how to create and manage connection with SSL configuration.
@@ -159,7 +369,13 @@ func (r *ReplicaDbCountSpecEnforcer) getDeployedReplicas() []statefulset.Statefu | |||
} | |||
|
|||
func (r *ReplicaDbCountSpecEnforcer) getNbreDeployedReplicas() int32 { | |||
return r.resourcesStates.StatefulSets.Replicas.NbreDeployed | |||
deployedReplicas := r.resourcesStates.StatefulSets.Replicas.NbreDeployed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is required in cases when we enable/disable replication slots so we can deploy replicas with proper configuration before cleaning up replicas that have old config (enabled/disable replication slots).
"reactive-tech.io/kubegres/controllers/states" | ||
) | ||
|
||
func TestCreateReplicaDbCountSpecEnforcer(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is empty now as I'm not sure we want to test it here - it will execute a lot faster but the setup requires creating a lot of fake object to make it work and I'm also not sure if there are some hidden dependencies on other controllers such as statefulset controllers.
@@ -211,8 +211,16 @@ data: | |||
then | |||
chown -R postgres:postgres $PGDATA; | |||
fi | |||
|
|||
|
|||
grep "primary_slot_name" $PGDATA/postgresql.auto.conf > /dev/null && sed -i '/primary_slot_name/d' $PGDATA/postgresql.auto.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will be also required in TSB since we use custom config.
github.com/onsi/ginkgo/v2 v2.21.0 | ||
github.com/onsi/gomega v1.34.2 | ||
github.com/stretchr/testify v1.10.0 | ||
github.com/testcontainers/testcontainers-go v0.38.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me know what do you think about using testcontainers - used to the repo object.
test/spec_replica_test.go
Outdated
}) | ||
|
||
// TODO(piotrkpc): missing tests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is missing, it might work though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the test, does work as expected.
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
# Conflicts: # go.mod # go.sum
Signed-off-by: Piotr Kopec <[email protected]>
…p after test case Signed-off-by: Piotr Kopec <[email protected]>
} | ||
} else { | ||
log.Println("No PVC found for kubegres resource '" + resourceToDelete.Name + "'") | ||
continue | ||
} | ||
} | ||
|
||
log.Println("Deleted all resources created during tests. Waiting for 30 seconds...") | ||
time.Sleep(30 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
watching for delete events rather than doing this sleep shave ~4min from each test group and hopefully helps with flakiness
…nce (#53) I need to finish testing of the TLS PR #51 before making this connection management compatible with TLS-secured connections. The [`func (r *ServicesCountSpecEnforcer) canConnectToPrimaryDb() bool`](https://github.com/tetrateio/kubegres/pull/53/files#diff-2069405fe99f8bae54d23273d6e4708cb8b4e1ec715bbe3f276c63b3157c5ee2R136) method is the current one checking all of this works. It will need to be removed once the target PR starts using these changes. --------- Signed-off-by: Sergi Castro <[email protected]> Co-authored-by: Copilot <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
Signed-off-by: Piotr Kopec <[email protected]>
test/util/TestResourceCreator.go
Outdated
@@ -261,6 +262,28 @@ func (r *TestResourceCreator) DeleteResource(resourceToDelete client.Object, res | |||
return true | |||
} | |||
|
|||
func (r *TestResourceCreator) DeleteResourceWithWatch(resourceToDelete client.Object, resourceName string, listObject client.ObjectList) (<-chan watch.Event, func()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not used here but i'd keep iet here so we can speedup tests in followup PR.
Signed-off-by: Piotr Kopec <[email protected]>
Part of: https://github.com/tetrateio/tetrate/issues/26058
This PR introduces the following options to enable use of replicaiton slots:
Here are default values:
Replication slots are created and deleted by the operator and the replication slot name is passed down to scripts as a env var in a similar way as
$PRIMARY_HOST_NAME