Fix spot fleets after the multi-part userdata improvement #732
Conversation
This is a POC of the possible fix for kubernetes-retired#727 with ignition, probably simplified/replaced/enhanced with a more simpler solution without ignition
k8s-ci-robot
added
the
cncf-cla: yes
|
|
||
| head, err := json.Marshal(`{ | ||
| "ignition": { | ||
| "version": "2.0.0" |
There was a problem hiding this comment.
In the future, to allow customizing the ignition config on user-side i.e. retain the flexibility of having all configs in a text form, we can add ignition.config.append.source which points to the user-customizable ignition config json uploaded to s3.
There was a problem hiding this comment.
Then, we can also utilize the user-customizable ignition config to gradually migrate from our current cloud-configs to ignition.
| // once it supports sourcing from a s3 object | ||
| // https://coreos.com/ignition/docs/0.14.0/configuration-v2_0.html | ||
|
|
||
| head, err := json.Marshal(`{ |
There was a problem hiding this comment.
@redbaron I've thrown away the quoteForCfn func 😉
|
Verified that this does fix #727 😉 |
Codecov Report
@@ Coverage Diff @@
## master #732 +/- ##
==========================================
- Coverage 34.33% 33.63% -0.71%
==========================================
Files 55 55
Lines 3073 3137 +64
==========================================
Hits 1055 1055
- Misses 1859 1923 +64
Partials 159 159
Continue to review full report at Codecov.
|
… userdata improvement Our original fix incorporated ignition, which introduces an another moving part which is not strictly necessary for now
| @@ -101,7 +101,7 @@ | |||
| {{end}} | |||
| ], | |||
| "SubnetId": {{$workerSubnet.Ref}}, | |||
| "UserData": {{ $.UserDataWorker.Parts.instance.Base64 true | checkSizeLessThan 16384 | quote }} | |||
| "UserData": {{ $.UserDataWorker.Parts.instance.Base64UserDataRef }} | |||
There was a problem hiding this comment.
I've introduced Base64UserDataRef to inject {"Ref":"AWS::StackName"} into nodes by modifying the user-provided cloud-config(bash script)
| "Name=key,Values=aws:cloudformation:stack-name" \ | ||
| --output json \ | ||
| | jq -r ".Tags[].Value") >> {{.StackNameEnvFileName}}' | ||
| echo "{{.StackNameEnvVarName}}=$(cat /kube-aws-stack-name)" >> {{.StackNameEnvFileName}} |
There was a problem hiding this comment.
The instance part of userdata now reads the stack name from a text file which is written by a bash snippet automatically injected by kube-aws
| "files": [ | ||
| { | ||
| "filesystem": "root", | ||
| "path": "/kube-aws-stack-name", |
There was a problem hiding this comment.
The path should be somewhere else like /var/run/coreos/kube-aws-stack-name. However, /var/run is a tmpfs other than the root filesystem, which requires me to provide ignition an another filesystem name other than "root".
The problem doing so is that I'm not sure that names of filesystems other than "root" is stable i.e. safe to be hard-coded.
So, I'm rather wanting to leave this undecided for now.
|
|
||
| func (self UserDataPart) Base64UserDataRef() (string, error) { | ||
| // Change this to Base64IgnitionConfigRef to test with ignition | ||
| return self.Base64BashScriptRef() |
There was a problem hiding this comment.
Ignition is not used by default for now. However, I like to leave the ignition support able to be enabled in code so that we can start testing with it toward #728 🤓
| splits = append(splits, jsonStrings[:1]...) | ||
| splits = append(splits, `"echo \""`) | ||
| splits = append(splits, `{"Ref":"AWS::StackName"}`) | ||
| splits = append(splits, `"\" >> /kube-aws-stack-name\n"`) |
There was a problem hiding this comment.
This func is used to inject {"Ref":"AWS::StackName"} into nodes by modifying the "instance" parts of userdata.
|
Implemented, enabled, and verified the simpler fix without ignition. @redbaron Could you review this? This solution with Also, please let me sync with you that I'd like to address #727 before releasing v0.9.7 final 🙇 |
mumoshu
changed the title
|
Closing this in favor of #735 |
This is currently a POC of the possible fix for #727 with ignition, probably simplified/replaced/enhanced with a more simpler solution without ignition.