Add file assets to node user data scripts, fingerprint fileAssets and hooks content. #3323

KashifSaadat · 2017-08-31T18:18:09Z

Changes made:

Include FileAssets in the bootstrapscript (user-data for all nodes), selectively dependent on the roles specified for each asset.
Fingerprint the sections of the FileAssets (Content) and Hooks (Manifests, ExecContainer Commands) Specs within the bootstrap script to reduce size (otherwise this can very quickly hit the 16KB user data limit with AWS).

KashifSaadat · 2017-08-31T18:19:19Z

PR raised based on what we discussed earlier, let me know what you think! :) I don't believe there's any associated issue raised for this yet.

gambol99

Just left a few comments ...

gambol99 · 2017-08-31T19:26:48Z

pkg/model/bootstrapscript.go

-				spec["hooks"] = hooks
+				if hooksFingerprint, err := b.fingerprintSpecList(hooks); err != nil {
+					return "", err
+				} else {


i think the else is redundant here given the return;

hooksFingerprint, err := b.fingerprintSpecList(hooks) if err != nil { return "", err } spec["hooksFingerprint"] = hooksFingerprint

gambol99 · 2017-08-31T19:27:36Z

pkg/model/bootstrapscript.go

+
+			fileAssets := b.getRelevantFileAssets(cs.FileAssets, ig.Spec.Role)
+			if len(fileAssets) > 0 {
+				if fileAssetsFingerprint, err := b.fingerprintSpecList(fileAssets); err != nil {


gambol99 · 2017-08-31T19:34:02Z

pkg/model/bootstrapscript.go

@@ -124,7 +139,19 @@ func (b *BootstrapScript) ResourceNodeUp(ig *kops.InstanceGroup, cs *kops.Cluste
 			spec["taints"] = ig.Spec.Taints
 			hooks := b.getRelevantHooks(ig.Spec.Hooks, ig.Spec.Role)
 			if len(hooks) > 0 {
-				spec["hooks"] = hooks
+				if hooksFingerprint, err := b.fingerprintSpecList(hooks); err != nil {


hmmm ... thinking out loud, perhaps it's overkill; I know we lose the diff (i.e. content) but it might be useful to keep some context, rather then a large hash on the full spec, maybe a hash per item. At least then you have some clue as to what changed. @KashifSaadat @justinsb

I like that idea, will give it a go!

gambol99 · 2017-08-31T19:34:21Z

pkg/model/bootstrapscript.go

+					spec["hooksFingerprint"] = hooksFingerprint
+				}
+			}
+			fileAssets := b.getRelevantFileAssets(ig.Spec.FileAssets, ig.Spec.Role)


same thoughts as above ... though happy to go with the majority :-)

hooks and fileAssets specs.

KashifSaadat · 2017-09-01T14:45:00Z

Following @gambol99's suggestions I've done the following:

Fingerprint the ExecContainer.Command & Manifest within each Hook (these could both potentially be quite large)
Fingerprint the Content within each File Asset
Drop the Roles from the processed Hooks & File Assets

I've dropped the roles (set to nil) to avoid potentially causing an instance group to have a rolling upgrade when it's not necessary. E.g. You could have a File Asset deploying to Masters & Nodes, then drop Masters from the Roles list. This would then trigger a UserData update for Nodes, causing them to require a rebuild with no functional changes.

chrislovecnm · 2017-09-15T00:53:54Z

@gambol99 lgtm??

justinsb · 2017-09-15T14:11:33Z

/lgtm

justinsb · 2017-09-15T13:57:09Z

pkg/model/bootstrapscript.go

-func (b *BootstrapScript) getRelevantHooks(hooks []kops.HookSpec, role kops.InstanceGroupRole) []kops.HookSpec {
+// getRelevantHooks returns a list of hooks to be applied to the instance group,
+// with the Manifest and ExecContainer Commands fingerprinted to reduce size
+func (b *BootstrapScript) getRelevantHooks(allHooks []kops.HookSpec, role kops.InstanceGroupRole) ([]kops.HookSpec, error) {


Not sure if we can reuse this selector logic in nodeup (probably by moving it to a new package). I think we have the same logic duplicated?

Do you know where it's duplicated, or where's best to move it to? We could extract from here (taking out the fingerprinting logic so it doesn't make the spec content invalid).

justinsb · 2017-09-15T14:10:09Z

pkg/model/bootstrapscript.go

+				if err != nil {
+					return nil, err
+				}
+				fileAsset.Content = contentFingerprint + " (fingerprint)"


Long term I feel like it would be safer to use a hash in a new field (ContentHash), but this is the right way to start!

Sure that would be better. Would you suggest updating the spec to include ContentHash (which may get confusing, as we won't expect users to be specifying this in their spec), or just add it as a new field here (might be a bit complicated as it's not a valid field on the struct)?

justinsb · 2017-09-15T14:10:57Z

pkg/model/bootstrapscript.go

+		return "", fmt.Errorf("error computing fingerprint hash: %v", err)
+	}
+
+	return base64.StdEncoding.EncodeToString(hasher.Sum(nil)), nil


I like this fingerprint function - it is unique enough that we could use it to retrieve the Content using only the ContentHash (within the "safe space" of a cluster!)

I think you wrote this one in an earlier PR, came in quite handy :D

k8s-github-robot · 2017-09-15T14:12:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-15T15:32:51Z

/test all [submit-queue is verifying that this PR is safe to merge]

KashifSaadat · 2017-09-15T16:35:44Z

/test pull-kops-e2e-kubernetes-aws

k8s-github-robot · 2017-09-15T17:08:29Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-15T17:44:16Z

Automatic merge from submit-queue

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2017

k8s-ci-robot assigned justinsb Aug 31, 2017

gambol99 reviewed Aug 31, 2017

View reviewed changes

Add file assets to node user data scripts, fingerprint sections of the

e4919d0

hooks and fileAssets specs.

KashifSaadat force-pushed the bootstrap-include-file-assets branch from 4693731 to e4919d0 Compare September 1, 2017 14:25

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2017

justinsb reviewed Sep 15, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017

k8s-github-robot merged commit 70007f8 into kubernetes:master Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file assets to node user data scripts, fingerprint fileAssets and hooks content. #3323

Add file assets to node user data scripts, fingerprint fileAssets and hooks content. #3323

KashifSaadat commented Aug 31, 2017 •

edited

KashifSaadat commented Aug 31, 2017

gambol99 left a comment

gambol99 Aug 31, 2017

KashifSaadat Sep 1, 2017

gambol99 Aug 31, 2017

KashifSaadat Sep 1, 2017

gambol99 Aug 31, 2017 •

edited

KashifSaadat Aug 31, 2017

gambol99 Aug 31, 2017

KashifSaadat commented Sep 1, 2017

chrislovecnm commented Sep 15, 2017

justinsb commented Sep 15, 2017

justinsb Sep 15, 2017

KashifSaadat Sep 15, 2017

justinsb Sep 15, 2017

KashifSaadat Sep 15, 2017

justinsb Sep 15, 2017

KashifSaadat Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

KashifSaadat commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

Add file assets to node user data scripts, fingerprint fileAssets and hooks content. #3323

Add file assets to node user data scripts, fingerprint fileAssets and hooks content. #3323

Conversation

KashifSaadat commented Aug 31, 2017 • edited

KashifSaadat commented Aug 31, 2017

gambol99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gambol99 Aug 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KashifSaadat commented Sep 1, 2017

chrislovecnm commented Sep 15, 2017

justinsb commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

KashifSaadat commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

KashifSaadat commented Aug 31, 2017 •

edited

gambol99 Aug 31, 2017 •

edited