search

teamhephy / postgres

MIT License
2 stars 6 forks source link

WIP: Feat s3 endpoint #6

Open kingdonb opened 5 years ago

kingdonb commented 5 years ago

Includes changes from teamhephy/workflow#71 and teamhephy/workflow#74, #3, #5

This seems to be working well for me with DigitalOcean Spaces!

(There are some commits on the end that need to be squashed and made to conform to the style guide, marking this PR as wip...)

Also requires the changes in teamhephy/workflow#75, with a Values.global.s3.use_sse setting

duanhongyi commented 5 years ago

@kingdonb

docker build fails

docker build -t /deis/postgres:git-01e7242 .

Modify the comment, and then I close my RP.

duanhongyi commented 5 years ago

Why use the global use_sse variable?

yebyen commented 5 years ago

I think that only global variables are passed into depended charts... Try it, I got an error message from helm without the global

See https://github.com/kingdonb/postgres/pull/1#event-1985100615

duanhongyi commented 5 years ago

Hello everyone! I reorganized the submission and replaced the global variable with a local use_sse #7

kingdonb commented 5 years ago

Thank you, I see this makes sense. Will test and review.

Cryptophobia commented 5 years ago

Thank you, I see this makes sense. Will test and review.

Thanks for testing @kingdonb . My plan is to merge these S3 support changes by @rwos in next release of Hephy v2.21 or to merge them into v2.20 as long as they are non-breaking changes.

kingdonb commented 5 years ago

@Cryptophobia That sounds good, I'd like to verify that the switch of base images to postgres:11 (removing ca-certificates) hasn't broken any behaviors in AWS S3, GCS, or Azure Blob, the currently supported storage APIs. I'm not sure if anyone else is testing database/postgres WAL backups.

Either that, or a hotfix to the current beta release adding "ca-certificates" – in the event that it is broken, I think the remainder of the S3_ENDPOINT things are still needing some changes and not ready to merge yet. But since I'm not usually testing on AWS anymore, I can't say for sure that it's broken in master right now either. (I have a suspicion but don't know how certificate verification works in Boto, so it's possible that everything is fine. My testing is very caveman-like, I'm not able to say for sure that any needed PRs to complete this are actively missing, only that I have not tested them all together and wound up with a perfectly functioning cluster.) Edit: It looks like I should have just looked in master, the latest image built for v2.7.0 postgres does actually include ca-certificates

I'll repeat the testing I've done on a supported cloud with v2.20-beta to be sure it's ready for release, I think that's the right way forward. We should be able to guarantee the S3_ENDPOINT changes are non-breaking for v2.20.1, in fact one better, we should guarantee that upgrading + reusing v2.20.0 values.yaml that omits the endpoint config entirely doesn't change any behavior or ruin any part of the configuration in the upgrade.

Cryptophobia commented 5 years ago

Please see comment in the postgres issue https://github.com/teamhephy/postgres/issues/8#issuecomment-441745954. I think ca-certificates is still being updated on the image due to step #4 of the Dockerfile.

Cryptophobia commented 5 years ago

I think the remainder of the S3_ENDPOINT things are still needing some changes and not ready to merge yet.

Agreed. We need to test the S3_ENDPOINT changes more.

in fact one better, we should guarantee that upgrading + reusing v2.20.0 values.yaml that omits the endpoint config entirely doesn't change any behavior or ruin any part of the configuration in the upgrade.

Totally agree with this.

duanhongyi commented 5 years ago

@kingdonb hi, can you merge RP (#7)? It contains all commit of this; fixes bugs in build and uses related local variables instead of global variables.

Or instead of merge the RP, use it directly (#7) @Cryptophobia

kingdonb commented 5 years ago

We are considering them for v2.21 @duanhongyi – the alpine change also might be able to get merged for 2.20 because it can help to save so much space in terms of docker images.

But the S3_ENDPOINT changes are still under consideration. #7 includes all of those, so if you can submit a new PR with only Alpine and not including S3_ENDPOINT, it might become the next release.

kingdonb commented 5 years ago

I am testing this evening... v2.20-beta with Azure blob storage and AKS

duanhongyi commented 5 years ago

@kingdonb That's RP (#5).