Add hooks to bootstrapscript output #3195
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @KashifSaadat. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/assign @justinsb |
| func (b *BootstrapScript) getRelevantHooks(hooks []kops.HookSpec, role kops.InstanceGroupRole) []kops.HookSpec { | ||
| relevantHooks := []kops.HookSpec{} | ||
| for _, hook := range hooks { | ||
| if len(hook.Roles) == 0 { |
There was a problem hiding this comment.
you might as well hit this early and check at the beginning no?
There was a problem hiding this comment.
We pass the full list of hook specs and roles can be defined differently for each hook, so will need to check on each one.
There was a problem hiding this comment.
Debug logging would probably be helpful if a user messes this up.
There was a problem hiding this comment.
What happens with an image where docker or the proper docker is not installed? For instance, RHEL does not have docker installed.
KashifSaadat
force-pushed
the
hooks-in-bootstrap-script
branch
from
1a71794
to
fb3e830
Compare
| export https_proxy=${http_proxy} | ||
| export no_proxy= | ||
| export NO_PROXY=${no_proxy} | ||
| echo "export http_proxy=${http_proxy}" >> /etc/default/docker |
There was a problem hiding this comment.
Just a crazy side not we have functionality for this now in kops. i.e. egressProxy will setup http proxy.
| @@ -57,20 +58,53 @@ func TestBootstrapUserData(t *testing.T) { | |||
| cs := []struct { | |||
| Role kops.InstanceGroupRole | |||
| ExpectedFilePath string | |||
| HookSpecRoles []kops.InstanceGroupRole | |||
| }{ | |||
| { | |||
| Role: "Master", | |||
| ExpectedFilePath: "tests/data/bootstrapscript_0.txt", | |||
There was a problem hiding this comment.
Total nit pick question. Why do we need 5 test files? A quick eye ball was not showing me any differences.
There was a problem hiding this comment.
No worries, I was feeling it might be a bit excessive. :D Each of the test cases have some subtle differences in the instance group type / configuration, which can cause a change in output of the bootstrap script. I covered each where I felt the logic could go a bit wrong, such as multiple hook specs applying to specific / all instance groups. The test file contents all vary towards the bottom, in the generation of the cluster_spec.yaml and ig-spec.yaml files.
| func (b *BootstrapScript) getRelevantHooks(hooks []kops.HookSpec, role kops.InstanceGroupRole) []kops.HookSpec { | ||
| relevantHooks := []kops.HookSpec{} | ||
| for _, hook := range hooks { | ||
| if len(hook.Roles) == 0 { |
There was a problem hiding this comment.
Debug logging would probably be helpful if a user messes this up.
| func (b *BootstrapScript) getRelevantHooks(hooks []kops.HookSpec, role kops.InstanceGroupRole) []kops.HookSpec { | ||
| relevantHooks := []kops.HookSpec{} | ||
| for _, hook := range hooks { | ||
| if len(hook.Roles) == 0 { |
There was a problem hiding this comment.
What happens with an image where docker or the proper docker is not installed? For instance, RHEL does not have docker installed.
|
@KashifSaadat do we have an issue that is tied to this PR? Do we need to update documentation? Thanks for the PR! |
|
Hey @chrislovecnm, thanks for the review :) The PR is related to issue #3076, where certain spec changes do not trigger an update being required for a node. I raised PR #3120 to cover these cases, by including the spec detail within the bootstrap script it causes a diff in the node configuration and will mark it for update. This comes off the back of PR #3063 which made some changes to hooks. I can include some documentation if you like, is there any related section which needs updating? |
|
Now I understand, so it does not matter if we have docker or anything else. Since this is not new functionality, we probably just need release notes on this. This will only help with hooks and not say other component config values? |
|
Yep that's right, this PR only adds hooks into the user-data script. The PR #3120 covered some other component config cases, but possibly not all. If there's any you think I've missed, let me know and I can add to this (or raise a separate PR for them). |
|
/ok-to-test We are good @KashifSaadat - pending e2e lets get this in |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KashifSaadat, chrislovecnm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
Ping @gambol99
Related to:
Cluster Hooks are now added in their entirety to the nodeup user data scripts, dependant on whether the instance group roles match. This could increase the size of the user-data files significantly (getting closer to the 16KB mark) depending on users' hooks content. I considered hashing and fingerprinting the cluster hooks, but following discussions in #3120 thought we should keep the full output instead.