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

Add Support for Pre bootstrap commands in Windows Node Groups #2004

Merged
merged 1 commit into from
May 11, 2020
Merged

Add Support for Pre bootstrap commands in Windows Node Groups #2004

merged 1 commit into from
May 11, 2020

Conversation

niranjan94
Copy link
Contributor

@niranjan94 niranjan94 commented Apr 3, 2020

Description

Allows the specification of preBootstrapCommands in the node group spec for windows. Users can run custom powershell commands on instance start before the instance is bootstrapped for EKS.

Resolves #2003

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the site/content directory)
  • Manually tested
  • Added labels for change area (e.g. area/nodegroup), target version (e.g. version/0.12.0) and kind (e.g. kind/improvement)
  • Make sure the title of the PR is a good description that can go into the release notes

@niranjan94 niranjan94 changed the title Added Support for Pre bootstrap commands in Windows Node Groups Add Support for Pre bootstrap commands in Windows Node Groups Apr 5, 2020
@niranjan94
Copy link
Contributor Author

@sayboras @martina-if @cPu1 please review when possible.


The tests started failing after rebasing over the latest master. Seems to be failing in the codegen stage. Unable to find CloudFormationAPI in any go files under this path. But can find it defined correctly under pkg/eks/mocks/CloudFormationAPI.go. The tests also pass fine locally. I'm not sure how to fix this. Would be really helpful if you could point me in the right direction. Thanks 😄

The error log from Circle CI:

Generating deepcopy funcs
>> Removing /tmp/tmp.BaNKKO
time go run ./cmd/schema/generate.go userdocs/src/usage/schema.md
real	0m 51.86s
user	1m 34.23s
sys	0m 6.27s
mkdir -p vendor/github.com/aws/
ln -sfn "/go/pkg/mod/github.com/weaveworks/aws-sdk-go@v1.25.14-0.20191218135223-757eeed07291" vendor/github.com/aws/aws-sdk-go
time env GOBIN=/go/bin go generate ./pkg/eks/mocks
Unable to find CloudFormationAPI in any go files under this path
pkg/eks/mocks/mocks.go:16: running "/go/bin/mockery": exit status 1
Command exited with non-zero status 1

@sayboras
Copy link
Contributor

sayboras commented Apr 27, 2020

@niranjan94 thanks for your pointing this out. I got this fixed in #2021, but this PR might take some time to land in.

So I have created another one to fix this issue #2097. If this is blocking you, kindly help to apply the change here as well.

Sorry for any inconvenience caused.

@sayboras
Copy link
Contributor

@niranjan94 the fix got merged into master, so simple rebase should fix the build failure. Thanks

@niranjan94
Copy link
Contributor Author

@sayboras the build passes now. Thank you ! :)

@sayboras
Copy link
Contributor

@sayboras the build passes now. Thank you ! :)

You might need to rebase one more time to fix the error with link-checker, now link-check only runs if there is changes on related files.

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented May 9, 2020

@niranjan94 do you want to run these commands once at instance launch or every time the instance starts? At the moment (with your branch) they are executed only once, at launch.

@niranjan94
Copy link
Contributor Author

@michaelbeaumont good point. I had overlooked this.

Yes, currently this PR runs it only once at launch. But I see that linux nodes use runcmd cloudconfig directive for pre-bootstrap commands which run at each boot.

I will make changes to this PR to ensure this runs on every boot too to keep the behaviour consistent with linux nodes (Will take care to ensure the kubernetes windows bootstrap command runs only once though)

Allows the specification of preBootstrapCommands in the node group spec for windows. Users can run custom Powershell commands on instance start before the instance is bootstrapped for EKS.

Issue #2003
@niranjan94
Copy link
Contributor Author

@michaelbeaumont I have made the changes to ensure the pre-bootstrap commands run on every boot, consistent with the behavior of linux nodes. Please review when possible 😄

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented May 11, 2020

@niranjan94 I think the precmd commands are only run once at launch for linux as well, which functionality do you need? (sorry for the confusion)
https://github.com/weaveworks/eksctl/blob/aaeab00f8173eded3388ae51b5b6465de1ae5230/pkg/cloudconfig/cloudconfig.go#L22

@niranjan94
Copy link
Contributor Author

@michaelbeaumont you're right again 😄 . I completely misunderstood the runcmd documentation and wrongly assumed it runs at every launch. 🤦

My previous implementation was correct. Will revert the recent commit.

  • New: Windows: Uses <powershell></powershell> user data directive. Runs once at first boot.
  • Existing: Linux: Uses runcmd cloud-config directive. Runs once at first boot.

Windows & linux implementations are inline with each other on the behaviour of pre-bootstrap commands.

@michaelbeaumont
Copy link
Contributor

OK, sounds good. I only asked initially because I wanted to be sure that's the functionality you intended it to have. thanks!

@michaelbeaumont michaelbeaumont merged commit df2b4d5 into eksctl-io:master May 11, 2020
@niranjan94 niranjan94 deleted the windows-pre-bootstrap-commands branch May 11, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Pre bootstrap commands in Unmanaged Windows Node Groups
3 participants