Expose nodegroup container runtime configuration

[Skarlso]

Description

Closes #3979

Checklist

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

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

• Backfilled missing tests for code in same general area 🎉
• Refactored something and made the world a better place 🌟

[Callisto13]

And don't forget the docs 😜 In which we should highlight that we are not exposing a flag for this thing, just config

[cw-sakamoto]

Thanks for the great PR!!

If setting containerRuntime to containerd, I think you don't need the following command, is that correct?
https://github.com/weaveworks/eksctl/blob/50659eb7c041f12e5a481225db40b7ae11662be7/pkg/nodebootstrap/bindata/assets/bootstrap.al2.sh#L24

[Skarlso]

Thanks for the great PR!!

If setting containerRuntime to containerd, I think you don't need the following command, is that correct?
https://github.com/weaveworks/eksctl/blob/50659eb7c041f12e5a481225db40b7ae11662be7/pkg/nodebootstrap/bindata/assets/bootstrap.al2.sh#L24

Hi! :)

Yes, normally, I would agree, however, there is an issue here awslabs/amazon-eks-ami#698 (comment) that some people reported that this is breaking CI and their flow in some way. I thought about leaving this in there in that case. But I'm not a 100% sure about that. @Callisto13 any thoughts on the comment in the issue?

[cw-sakamoto]

@Skarlso

Thanks for the quick reply!

If keep it, we need to change the path of containerd's sock path below in docker service file.

$ cat /usr/lib/systemd/system/docker.service | grep ExecStart
ExecStartPre=/bin/mkdir -p /run/docker
ExecStartPre=/usr/libexec/docker/docker-setup-runtimes.sh
ExecStart=/usr/bin/dockerd -H fd:// --containerd=/run/containerd/containerd.sock $OPTIONS $DOCKER_STORAGE_OPTIONS $DOCKER_ADD_RUNTIMES

FYI: https://github.com/awslabs/amazon-eks-ami/blob/master/files/containerd-config.toml#L6

[Callisto13]

@Skarlso I think I referred to this line in a comment somewhere (maybe in slack?). This restart was added when we would add/edit the docker daemon config, which I believe we no longer do. So it is likely some legacy stuff which I forgot to remove when I removed some bits which changed the cgroup driver.

So we can:

1. Try removing it completely, see if it is needed when --container-runtime=dockerd. (I feel it really should not be needed but good to check)
2. If I am wrong and it is needed, then it will only been required if using dockerd in which can we can throw an if around it. (basically mirroring what that ami bootstrap script is doing: they do not run this if containerd)

make sense?

[Callisto13]

LGTM!