BCDevOps / platform-services

Collection of platform related tools and configurations
Apache License 2.0
13 stars 29 forks source link

Patroni updates for OCP4 support and multi-version postgres build support #702

Closed jefkel closed 4 years ago

jefkel commented 4 years ago

Updated build/deploy for Patroni:

Builds

Deployments

Documentation

Not Covered

Tested Build/Deploy on OpenShift 4 lab environments successful for builds using postgres 10, 11, 12.

cvarjao commented 4 years ago

@jefkel , does this PR effectively replaces #703 and #691?

jefkel commented 4 years ago

I think it effectively replaces #691 (As I didn't run into the same errors during my build testing), but as I wasn't able to reproduce that specific error, I can't say for sure. I did NOT incorporate any of the changes in that PR into this one, so if the issue is not fixed with the base image update, then this PR will not replace PR #691

As for your latest PR #703, I based my initial work on the branch you used for that PR. There were some additions (specifically the additional dockerfiles for different postgres base image versions).

I think that the biggest difference between your branch and this PR is that I'm leveraging the major version tags of postgres (10,11,12) and you had a specific 10.13 version tag on your builds. If the postgres version difference is not an issue for your PR, then I would suggest that this would replace your PR as well.

cvarjao commented 4 years ago

I guess, a question to the team is when to use LTS/rolling tags (10, 11) vs immutable tags. The problem I have with LTS tags is that the Dockerfile is NOT deterministic. The same source code built today, may produce a different image tomorrow with absolutely no breadcrumb of that change in the source control. IMHO I think that the rolling to a new base image needs to be explicit decision, and the output should always be tested, and a track record in source control of that change.

cvarjao commented 4 years ago

It looks like that other than change on the FROM statement, there is absolutely no difference among the 3 docker files. I think that at least for now, the PG_VERSION parameter in the build.yaml template already takes care of switching the base image while keeping the exact same Dockerfile. I could see that in the future, when/if there is actual difference we could use different Dockerfiles.

jefkel commented 4 years ago

The PG_VERSION parameter in the build.yaml file does NOT change the tag used in the dockerfile, but rather is only used to select the external postgres tag to sync locally. The dockerfile is referencing a specific (local) tagged version of postgres, if you're looking at supporting multiple (concurrent) base image versions in the builds (such as postgres:10, postgres:10.13, postgres:11, postgres:12, ... ), we need to either have separate dockerfile's (with the explicit version tag used for the build), or alternatively add a generic build-tag that hides the base image used (to be controlled by tweaking the pointer build-tag).

I'm not a big fan of using a generic build-tag in a single dockerfile as this means that the base image can be switched simply by re-pointing the generic tag. I'd rather use separate dockerfiles, adding in a new dockerfile for each explicit (potentially immutable) postgres version which would then be tracked in version control.

All that said, there is no requirement in this PR to only use the provided dockerfile's that specify the LTS version. These could be used as samples and a separate immutable version specified in both a dockerfile and PG_VERSION could easily be added to support a more explicitly managed pipeline.

cvarjao commented 4 years ago

@jefkel , the BuildConfig.spec.strategy.dockerStrategy.from DOES replace the FROM statement from the DockerFile as documented:

from is reference to an DockerImage, ImageStreamTag, or ImageStreamImage from which the docker image should be pulled the resulting image will be used in the FROM line of the Dockerfile for this build

if you pass oc process ... -p DOCKER_FILE=Dockerfile-pg12 -p PG_VERSION=10, it will still build using postgres:10

jefkel commented 4 years ago

missed that, I now want to re-test...

cvarjao commented 4 years ago

I am just concerned with having 3 (or more Dockerfiles) to now maintain and probably keep them in sync, when OpenShift/BuildConfig already provides a way to abstract them out in one file since the only thing changing is the FROM instruction.

jefkel commented 4 years ago

Thanks for the over-engineer catch. I've removed the extra dockerfiles (and dockerfile parameter) for the buildconfig, and it's building/deploying as expected.

Documentation to reflect the change has also been updated

jefkel commented 4 years ago

hmm, I can't recall a reason for it not to be REPLICAS vs SS_REPLICAS (I'll switch that)

And yeah. Default image registry is for OCP4 format, README contains a note for adding the IMAGE_REGISTRY parameter for OCP3 deployments (Had to pick one, and most of my testing was on OCP4)