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

[storage] Add an SQL storage driver #5371

Merged
merged 4 commits into from May 9, 2019

Conversation

Projects
None yet
7 participants
@elafarge
Copy link
Contributor

commented Feb 27, 2019

What this PR does / why we need it:

This commits adds the possibility to back Tiller (or the future
Tiller-less Helm CLI) with any SQL database (only postgres has been
tested so far) to store release information.

The main motivation for this commit was to use a storage backend that
would allow releases larger that 1MB in size (ConfigMap or Secret
drivers don't, because of limits on value size in the underlying etcd
key-value store).

Signed-off-by: Étienne Lafarge etienne.lafarge@gmail.com

Co-authored-by: Elliot Maincourt e.maincourt@gmail.com (@emaincourt)
Co-authored-by: Paul Borensztein hi@0x01.fr (@commit-master)

Special notes for the reviewer(s):

  • It goes without saying that we'll be glad to maintain and update this part of the codebase - in particular - to comply with the new Tiller-less Helm 3 ;-)
  • Also, we'll be glad to write an any backend -> postgres migration script if this backend gets promoted to GA at some point.
  • Only postgresql has been tested so far. However, we've tried to make our code SQL dialect-agnostic (using sqlx and avoiding postgres specific SQL statements). We'll be happy to test other databases if required by the community.
  • We're currently testing this feature on our pre-production environment (to provide our developers with a "staging-on-demand" feature) and will switch our production infrastructure on it if this PR gets merged

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility --> not applicable for now, we'll be glad to write a migration script and add it to the helm CLI if needed

closes #1413
closes #1996
closes #4215

@helm-bot helm-bot added the size/XXL label Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from be9e49f to fb7d99b Feb 27, 2019

@helm-bot helm-bot added size/XL and removed size/XXL labels Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from fb7d99b to 531aaaa Feb 27, 2019

@helm-bot helm-bot added size/XL and removed size/XL labels Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 531aaaa to 1a00388 Feb 27, 2019

@helm-bot helm-bot added size/XL and removed size/XL labels Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 1a00388 to 33a8fbc Feb 27, 2019

@helm-bot helm-bot added size/XXL and removed size/XL labels Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 33a8fbc to 759bc8b Feb 27, 2019

@helm-bot helm-bot added size/XXL and removed size/XXL labels Feb 27, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 759bc8b to 6feda1e Feb 27, 2019

@helm-bot helm-bot added size/XXL and removed size/XXL labels Feb 27, 2019

Show resolved Hide resolved docs/install.md Outdated
Show resolved Hide resolved docs/install.md Outdated
Show resolved Hide resolved docs/sql-storage.md Outdated
Show resolved Hide resolved docs/sql-storage.md Outdated
Show resolved Hide resolved docs/sql-storage.md Outdated
Show resolved Hide resolved docs/sql-storage.md Outdated
Show resolved Hide resolved docs/sql-storage.md Outdated
Show resolved Hide resolved pkg/storage/driver/sql.go Outdated

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 6feda1e to a4f2d95 Mar 2, 2019

@helm-bot helm-bot added size/XL and removed size/XXL size/XL labels Mar 2, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 193ed00 to c5e5a93 Mar 2, 2019

@helm-bot helm-bot added size/XL and removed size/XL labels Mar 2, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from c5e5a93 to e6ef139 Mar 2, 2019

@elafarge

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@technosophos @bacongobbler we just rebased our branch against master and fixed some conflicts in the glide.lock file. Let us know if there's anything else you'd like us to change :)

@callicles

This comment has been minimized.

Copy link

commented Apr 5, 2019

@technosophos Would be great if that could make it to the next release!

@bacongobbler bacongobbler added this to the 2.14.0 milestone Apr 5, 2019

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 77ecebe to 1e522c9 Apr 8, 2019

@elafarge

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

(just rebased against the master branch, fixed a conflict in tiller.go and force-pushed).

@tg339

This comment has been minimized.

Copy link

commented Apr 16, 2019

@technosophos any update on this one :) We're excited to see this come through to help us with getting over a hurdle that's preventing us from using helm in production

@bacongobbler

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

He's currently on vacation for the next week. I'd try asking in #helm-dev on Slack and ask if another maintainer would be willing to review this.

elafarge and others added some commits Feb 27, 2019

[storage] Add an SQL storage driver
This commits adds the possibility to back Tiller (or the future
Tiller-less Helm CLI) with any SQL database (only postgres has been
tested so far) to store release information.

The main motivation for this commit was to use a storage backend that
would allow releases larger that 1MB in size (ConfigMap or Secret
drivers don't, because of limits on value size in the underlying etcd
key-value store).

Signed-off-by: Étienne Lafarge <etienne.lafarge@gmail.com>

Co-authored-by: Elliot Maincourt <e.maincourt@gmail.com> (@emaincourt)
Co-authored-by: Paul Borensztein <hi@0x01.fr> (@commit-master)
[pr-review] Lighten docs & validate SQL dialect
Signed-off-by: Étienne Lafarge <etienne.lafarge@gmail.com>
Clarify our SQL Release binding struct naming and purpose
Signed-off-by: Elliot Maincourt <e.maincourt@gmail.com>
Fix formatting issue
Signed-off-by: Elliot Maincourt <e.maincourt@gmail.com>

@elafarge elafarge force-pushed the PayFit:feat/add-sql-storage-driver branch from 1e522c9 to f405282 Apr 22, 2019

@elafarge

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

(FYI, I just rebased the branch against master, fixed conflicts, and force-pushed)

@elafarge

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Hi @technosophos @bacongobbler ,

Could you take a look at this PR when you get the chance. All required updates have been made. We'll be glad to change/add/remove/discuss anything you feel is relevant in a timely manner so that this can make it to 2.14.0 ;-)

Have a great day !

@technosophos
Copy link
Member

left a comment

LGTM. I'll leave it to @bacongobbler to merge when it fits the release schedule.

@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I completely forgot that we made the call to bring this into the 2.14 release. I'll cut rc.2 with this PR. Thanks @elafarge!

@bacongobbler bacongobbler merged commit a93ebe1 into helm:master May 9, 2019

2 checks passed

DCO DCO
Details
ci/circleci: build Your tests passed on CircleCI!
Details
@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

cherry-picked into 2.14.

@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Just a heads up, but the SQL driver tests failed after merging this into master: https://circleci.com/gh/helm/helm/8204

@elafarge would you mind looking into this to fix up the breaking tests?

@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Looks like it's due to the underlying gRPC layer being populated. See attached.

image

@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I've pulled it out of the 2.14 release branch for the time being so we can continue to test the release candidate. @elafarge, when you have time to look into this, please reach out to me and we can see what we can do about getting this in for 2.14. Thanks!

@bacongobbler

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Found the fix; see #5706.

@elafarge

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Hi @bacongobbler . Sorry, for the late reply, I was off for the week. Glad to hear you found the fix.

Thanks a lot for the merge !! 🎉

@elafarge elafarge deleted the PayFit:feat/add-sql-storage-driver branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.