Add support for specifying maxPods as a bootstrap option

[backjo]

This PR builds off of conversation in #184 . This support is initially only added for BottleRocket nodes. In a later PR, additional fields will be added, as well as support for these fields on any/all compatible OS families.

[codecov]

Codecov Report

Merging #216 (2bc2c7a) into master (6565d8a) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   88.04%   88.15%   +0.10%     
==========================================
  Files          17       17              
  Lines        2166     2186      +20     
==========================================
+ Hits         1907     1927      +20     
  Misses        159      159              
  Partials      100      100              

Impacted Files	Coverage Δ
controllers/provisioners/eks/eks.go	85.00% <ø> (ø)
controllers/provisioners/eks/helpers.go	90.78% <100.00%> (+0.31%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6565d8a...379a6e5. Read the comment docs.

[eytan-avisror]

Will this default to 0 if not provided? should we have a better default?

[backjo]

it will default to 0 and be falsey

[eytan-avisror]

I thought the agreement in #184 was only to add structure around bootstrap arguments vs. kubelet arguments which might be difficult to keep track of.

#184 (comment)

Can you add a full design of how this should look like after we support all os families, what options we want to support and how the API will look like?

Since this adds a new API I want to make sure we are in full agreement on how it will look like, and potential use cases along with bootstrapArguments.
Feel free to add it here or in #184

[backjo]

Another option that might be easier is to not exclude kubelet-extra-args and merge the two fields for now (similar to what you suggested), and deprecate bootstrapArguments field in v1alpha2, so that you can submit something like:

configuration:
  bootstrapArguments: "--foo=bar"
  bootstrapOptions:
    kubeletArguments: "--eviction-hard memory.available<300mi"
    maxPods: 20 #jback changed this
    dnsClusterIP: 10.0.0.1 #jback added this
    dockerConfigJson: | 
       {
 	"bridge": "none",
 	"log-driver": "json-file",
 	"log-opts": {
 		"max-file": "5",
 		"max-size": "50"
       }
     }

Would result in:

bootstrap.sh <clustername> --use-max-pods 'false' --docker-config-json "<JSON payload>" --kubelet-extra-args '--max-pods "20" --eviction-hard "memory.available<300mi" --foo="bar" '

Going off of this ^ proposal (changed useMaxPods to maxPods). The example spec provided above would translate as follows:

AL2

bootstrap.sh <clustername> --dns-cluster-ip 10.0.0.1 --use-max-pods 'false' --docker-config-json "<JSON payload>" --kubelet-extra-args '--max-pods "20" --eviction-hard "memory.available<300mi" --foo="bar" '

BottleRocket

[settings.kubernetes]
api-server   = <autogenerated>
cluster-certificate = <autogenerated>
cluster-name = <clustername>
cluster-dns-ip = "10.0.0.1"
max-pods = 20

Windows

$EKSBootstrapScriptFile -EKSClusterName {{ .ClusterName }} -KubeletExtraArgs '--max-pods "20" --eviction-hard "memory.available<300mi" --foo="bar" ' 

I'd advocate for sticking to this small surface area of properties, as they are all shared by at least 2 of the 3 OS families.

[eytan-avisror]

are we planning to add a KubeletArguments element struct down the road to support all possible kubelet options in free form? if so how would we handle a --max-pods flag provided there directly?
e.g.

configuration:
  bootstrapOptions:
    kubeletArguments: "--max-pods 10 --eviction-hard memory.available<300mi"
    maxPods: 20

Since you are abstracting the UseMaxPods bool into just taking the maxPods (which is definitely nicer), we need to think of what to do in case of 'conflict' (this is only a problem in non-bottlerocket OS).

Also, what will we do in cases where we get both bootstrapArguments and kubeletArguments prior to deprecation of bootstrapArguments? e.g.

configuration:
  bootstrapArguments: "--eviction-hard memory.available<600mi"
  bootstrapOptions:
    kubeletArguments: "--eviction-hard memory.available<300mi"

[backjo]

Great questions!

Given that we don't have an API for BottleRocket kubelet args at the moment, I don't want to get too deep into designing an API for it. I think a freeform string for now works - it's likely that for bottlerocket, we could leverage the same field to just pass in TOML.

I'm thinking that we just allow duplicate flags. I ran into this while testing, actually - Kubelet just uses the last argument passed in. With that in mind, I think we should go the following strategy:

--kubelet-extra-args '<bootstrapArguments> <kubeletArguments> <generatedArguments'

This will result in:

• Generated kubelet args having the highest priority
• The 'new' kubeletArguments having normal priority
• The to-be-deprecated bootstrapArguments having the lower priority.

[eytan-avisror]

Awesome, this approach should be good !

[eytan-avisror]

@backjo
I'll merge this shortly - I think before we release 0.10.0 though we should complete this feature as discussed above with support for all os families as well as documentation that describes the behavior you mentioned above and possibly a functional test.
For the meanwhile, let me know if you need an image with this change and I will push one for you.
🥇

[backjo]

Sounds good!