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

Give users the option to gzip and base64 encode the heredocs in the nodeup.sh user-data #10357

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

rdrgmnzs
Copy link
Contributor

@rdrgmnzs rdrgmnzs commented Dec 3, 2020

gzip and base64 encode the heredocs in the nodeup.sh user-data, this is to accommodate the fact that there is a 15K limit on user-data in AWS and as our specs keep growing we are more and more likely to hit this limit.

This is an alternative to #10280

/cc @hakman @olemarkus

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rdrgmnzs

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2020
@rifelpet
Copy link
Member

rifelpet commented Dec 3, 2020

Spot checking the integration test outputs it looks like we can expect an approx 20 or 30% savings for nodes and masters respectively. The downside of this approach is the reduced visibility in dryrun diffs - though typically during an upgrade, changes here matter less than changes in nodeup itself which has no visibility in dryrun diffs.

@rdrgmnzs
Copy link
Contributor Author

rdrgmnzs commented Dec 3, 2020

Agreed on losing the visibility, I was actually playing with the idea of only encoding the heredocs if the user-data was over 15K however that got pretty ugly so I dropped it.

@hakman
Copy link
Member

hakman commented Dec 3, 2020

@rdrgmnzs would it be possible to make this optional? I would hate to go back to the dark ages when integration tests changes are less easy to understand. 😄

What do you think about compressing the actual script (or part of it) instead of the specs and env? This would change much less often.

@olemarkus
Copy link
Member

I don't think we can have it both ways. But I wonder if we really need all the content in the heredoc too. At least the duplication ...

I vote merging this in so that we don't mess up too much for 1.19 users with custom userdata. Then look for better solutions for 1.20.

@hakman
Copy link
Member

hakman commented Dec 3, 2020

For me it is important to see what changes in integration tests.

@rdrgmnzs
Copy link
Contributor Author

rdrgmnzs commented Dec 3, 2020

We could potentially set a bool in a struct and pass it all the way down to the nodeup script generation, that if set would then cause the heredocs to be compressed. We'd just pass gzipBase64 to the template as another function to use to gzip when the template is executed and use if blocks (with the previously mentioned var) to pick if we want to use the compressed version or not.

Let me do some digging tomorrow during the day to see how hard it would be to implement the above.

@rdrgmnzs rdrgmnzs changed the title gzip and base64 encode the heredocs in the nodeup.sh user-data Give users the option to gzip and base64 encode the heredocs in the nodeup.sh user-data Dec 4, 2020
@rdrgmnzs rdrgmnzs force-pushed the gzip-nodeup-heredocs branch 2 times, most recently from b0ec704 to c3fe6fa Compare December 4, 2020 07:12
Comment on lines 368 to 369
context := map[string]interface{}{
"compressStructs": b.ig.Spec.CompressUserData,
Copy link
Member

Choose a reason for hiding this comment

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

I see NodeUpSource and NodeUpSourceHash are implemented as functions should we use the same approach?

@mikesplain
Copy link
Contributor

Very nice, I think giving the user the option is a good middle ground on this one.

@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

I agree, nice work 👍🏻 can we add docs and maybe a release note for this?

@hakman
Copy link
Member

hakman commented Dec 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit ec69111 into kubernetes:master Dec 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Dec 4, 2020
k8s-ci-robot added a commit that referenced this pull request Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/documentation 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants