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.

Sign up for GitHub

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

Jump to bottom

ship a better storage solution #1157

Merged
merged 5 commits into from
Dec 11, 2019
Merged

ship a better storage solution #1157

merged 5 commits into from
Dec 11, 2019

Conversation

BenTheElder
Copy link
Member

@BenTheElder BenTheElder commented Dec 11, 2019 •
edited
Loading

  • install a customized rancher.io/local-host-path driver config at image build time (with preloaded images)
  • when installing the default storage class, attempt to install from the node instead of from the one in the kind binary first

TODO:

  • handle kubernetes < 1.12
    • DONE: for less than 1.12 we will ship the old manifest.
  • Push an image

I'll handle those later tonight. I just want to surface progress on this.

Fixes #1151 #118

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 11, 2019
@BenTheElder
Copy link
Member Author

note that this does unfortunately increase the node image size slightly, but having somewhat saner PVs is worth it.

@BenTheElder BenTheElder mentioned this pull request Dec 11, 2019
Closed
aojea
aojea reviewed Dec 11, 2019
View reviewed changes
pkg/build/node/node.go Outdated Show resolved Hide resolved
aojea
aojea reviewed Dec 11, 2019
View reviewed changes
pkg/cluster/internal/create/actions/installstorage/storage.go
@@ -53,7 +55,7 @@ func (a *action) Execute(ctx *actions.ActionContext) error {
node := controlPlanes[0] // kind expects at least one always

// add the default storage class
if err := addDefaultStorageClass(node); err != nil {
if err := addDefaultStorage(ctx.Logger, node); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be able to opt-out and not install it as we did with the CNI?
or opt-in and keep the legacy by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary initially.
You can easily replace the storage class with your own and delete the deployment, unlike CNI.

Let me mark this PR WIP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point in opt-in. We ship hermetic images, the images etc. will already be there and just running the deployment is low cost.

And the legacy one is know to have major limitations.

@BenTheElder BenTheElder changed the title ship a better storage solution WIP: ship a better storage solution Dec 11, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2019
yashbhutwala
yashbhutwala approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@yashbhutwala yashbhutwala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 😃

@k8s-ci-robot
Copy link
Contributor

@yashbhutwala: changing LGTM is restricted to collaborators

In response to this:

Nice! 😃

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, yashbhutwala

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BenTheElder
Copy link
Member Author

handled v1.11 node image builds by shipping the old manifest in that case (and not bundling the images)

that's pretty low effort but will let us continue to best effort support v1.11 minimally without much code

@BenTheElder BenTheElder changed the title WIP: ship a better storage solution ship a better storage solution Dec 11, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2019
@BenTheElder
Copy link
Member Author

rebased to upstream and pushed the node image

@aojea
Copy link
Contributor

aojea commented Dec 11, 2019

/lgtm
I couldn't test it though, will be nice to have feedback from users

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2019
@k8s-ci-robot k8s-ci-robot merged commit 09b20ed into kubernetes-sigs:master Dec 11, 2019
@tamalsaha
Copy link
Contributor

tamalsaha commented Dec 11, 2019 •
edited
Loading

This is great! Looking forward to the next release of Kind. Thank you!

@BenTheElder
Copy link
Member Author

I did a LOT of testing with these. It should work great with the tweaks we've applied for kind :-)

@BenTheElder BenTheElder deleted the storage branch December 11, 2019 21:04
@jmlrt jmlrt mentioned this pull request Jan 8, 2020
Closed
@monotek
Copy link
Member

monotek commented Jan 13, 2020

Is there a timeline / roadmap / due date for the next kind release?

Very interested in this fix as i hit this with the Elasticsearch (elastic/helm-charts#429) and likely also the Prometheus chart (eclipse/packages#13 (comment)).

@monotek monotek mentioned this pull request Jan 13, 2020
Closed
@BenTheElder
Copy link
Member Author

BenTheElder commented Jan 14, 2020 via email

@BenTheElder
Copy link
Member Author

BenTheElder commented Jan 14, 2020 via email

@monotek
Copy link
Member

monotek commented Jan 14, 2020

Thanks a lot! :-)

@monotek monotek mentioned this pull request Jan 15, 2020
Merged
@jglick jglick mentioned this pull request Jan 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@yashbhutwala yashbhutwala yashbhutwala approved these changes

@amwat amwat Awaiting requested review from amwat

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ship a working storageclass
6 participants
@BenTheElder @k8s-ci-robot @aojea @tamalsaha @monotek @yashbhutwala