This repository was archived by the owner on Jan 14, 2020. It is now read-only.
Simpler architecture, better logging, better tests (v1.0.0) #34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Golang uses the name testdata for directories containing test fixtures, so we should use it too.
Create an integration test suite for the pgbouncer package that exercises the PgBouncer struct and several important flows against a real PgBouncer.
Remove the functional tests that are much better to cover in our new integration tests, then cover what we can without an actual PgBouncer instance being booted.
Ditch the concept of a subscriber and replace it with a stream. This is a move toward a much simpler architecture that focuses on producing channels of mvccpb.KeyValue structs, allowing sharing of general concepts (like filtering streams, retrying handlers) between both the etcd and pacemaker codebase.
Change the proxy command to make use of the streams package with the etcd.Stream functionality. We also add integration tests that confirm the PgBouncer reloading behaviour in response to our host key being updated in etcd.
Refactor the concept of a subscriber into a simple stream functionality which plays well with the streams library. This ends up being a considerable simplification, which is great.
etcd doesn't enjoy having this variable set, and can break the integration tests.
Ensure we present the config hash in the correct type for go-kit to log the value. Fixup our config parsing to ensure our bytes are interpreted as toml. More significantly, we were using the uname attribute from pacemaker cib, which we can't then pass to the resolve method to transform into an IP address. This properly breaks things- this commit fixes them!
Don't send any old change down the etcd stream, ensuring we only get the keys we're actually watching, despite the prefix watch.
This represents an outdated model of 'integration' testing, that should be turned into a first-class acceptance test concept. Throw them out, we don't need them no more!
Ensure it's possible to render an InvalidNodeIDError, as before we were infinitely recursing on our own value.
Restructure our old integration tests into a first-class acceptance test concept. This is a binary called pgcm-acceptance that when run will spin-up docker containers to verify the behaviour of the pgcm codebase.
Requiring generate means we need protoc on our build system, which isn't ideal. Let's rely on our developers behaving correctly wrt running generate whenever they change proto based behaviour.
Change the integration harnesses to not return errors, but rely on gomega matchers to bail out of tests if things go wrong. We then need to ensure we're within a ginkgo context if we're going to panic with a matcher error, so start and cleanup the processes within a suite tear up/down hook.
Ensure circleci runs our unit tests as a non-root user, as any pgbouncer processes spawned by root will refuse to boot. Also ensure the postgres user in circle has a password configured, and that the pgbouncer test harness provides a mechanism to provision that password into the userlist, as it will use this to auth against the target postgres.
Switching to use the GinkgoWriter global allows Ginkgo to handle outputting logs at the right time whenever tests fail.
Refactor the failover pipeline process to improve error handling, logging, standardise the way each method is structured and provide a Pipeline concept that makes it easy to understand the steps required to perform a failover.
Use a grpc interceptor to log all-the-things, allowing us to remove the logging that used to exist at every point and rely on a generic logger. This along with several other small improvements neatens the file quite considerably, while preserving tests.
Bring the README up-to-date with the new pgcm command and all its uses. Ensure that the diagrams are correct and the examples use new binaries.
The refactor requires quite different images that break old builds. Tag the new images to reflect the breaking change.
We don't need any permissions here, the docker images aren't private.
13 tasks
Sometimes an etcd value might contain sensitive information, perhaps if we've misconfigured the streamer. This change ensures we only log this info in debug level.
Contributor
Author
|
We're unlikely to be making any changes to pgsql-cluster-manager soon, but will want to reference this repo in another projects dep file. Going to get this merged so we can do so more naturally. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was originally a PR on the master fork, but given the uncertain future of the project I've moved it out into my own fork. I think this PR contains some great examples of how we might architect go projects in future so if we do get some time/abandon our current setup then I think it's worth merging.
Otherwise it can exist in the separate fork as a default branch, making for easy viewing of the branched code.