Pre-install nvidia container runtime + drivers on GPU instances

[olemarkus]

• install nvidia container runtime
• install nvidia drivers
• make it opt-in to avoid clash with those who prebake/use gpu operator or similar
• add device plugin as addon

[olemarkus]

Worth mentioning that we override the containerd configuration override. Or at least modify it. kOps only knows about if the instance will have GPUs during nodeup, while cloudup will set the config override to the kOps default config by default (hence the containerd config override will always be set to the assumed "final" config).

[hakman]

The idea is good in general, we should make this easy for people.

In particular, here are some of my thoughts:

• I think the vision for nodeup is to become dumb, as dump as possible. This is why we moved the containerd logic out of it. I think you can call it defensive programming, but probably it's a longer topic that would be good for tomorrow's office hours.
• Maybe add this as a new option of the container runtime, allowing to override of the runtime to something other than runc. Describing instance types just for this purpose seems wasteful for every node. It can also be done much earlier when generating the cloud model.
• The APT source task, maybe would be better just as APT key instead and a separate file task for the repo file.
• The APT list is focused on Ubuntu, there is a different list for debian10 for example.
• The APT key should be sent as a file asset url, same as the containerd package.
• containerd config is done at the moment outside nodeup. The reason it didn't change was because of logic moving to nodeup that we want to avoid. Again, topic for office hours. Though, it either should move completely to nodeup or manage it outside it as we do now.

[olemarkus]

* I think the vision for nodeup is to become dumb, as dump as possible. This is why we moved the containerd logic out of it. I think you can call it defensive programming, but probably it's a longer topic that would be good for tomorrow's office hours.

* Maybe add this as a new option of the container runtime, allowing to override of the runtime to something other than runc. Describing instance types just for this purpose seems wasteful for every node. It can also be done much earlier when generating the cloud model.

The above has a challenge if someone creates a mixed instance group with both GPU and non-GPU instance types. I am not sure why anyone would do that, but it is possible, and if nvidia runtime is used on non-GPU, it will break.

That being said, we could do some hardware probing instead, I guess. Just seemed more reliable to describe the instance type.

* The APT source task, maybe would be better just as APT key instead and a separate file task for the repo file.

* The APT list is focused on Ubuntu, there is a different list for debian10 for example.

Yeah, I am aware. I plan on adding other distros as needed and in the interim solve this with some distro checks and documentation.

* The APT key should be sent as a file asset url, same as the containerd package.

Thanks. I didn't like the way I did this one. asset file url is a much better idea.

* containerd config is done at the moment outside nodeup. The reason it didn't change was because of logic moving to nodeup that we want to avoid. Again, topic for office hours. Though, it either should move completely to nodeup or manage it outside it as we do now.

If we find a way to do this in cloudup I'd also like to see if this can be put the final config aux. I don't like using the override flag as a carry mechanism cloudup -> nodeup.

[hakman]

* I think the vision for nodeup is to become dumb, as dump as possible. This is why we moved the containerd logic out of it. I think you can call it defensive programming, but probably it's a longer topic that would be good for tomorrow's office hours.

* Maybe add this as a new option of the container runtime, allowing to override of the runtime to something other than runc. Describing instance types just for this purpose seems wasteful for every node. It can also be done much earlier when generating the cloud model.

The above has a challenge if someone creates a mixed instance group with both GPU and non-GPU instance types. I am not sure why anyone would do that, but it is possible, and if nvidia runtime is used on non-GPU, it will break.

That being said, we could do some hardware probing instead, I guess. Just seemed more reliable to describe the instance type.

No need for that, we have validation. If we can validate that instances should not have a mix of ARM and AMD, we can make sure GPU instances are validated in same way

* The APT source task, maybe would be better just as APT key instead and a separate file task for the repo file.

* The APT list is focused on Ubuntu, there is a different list for debian10 for example.

Yeah, I am aware. I plan on adding other distros as needed and in the interim solve this with some distro checks and documentation.

Meant that the code checks for Debian family instead of Ubuntu specific

* The APT key should be sent as a file asset url, same as the containerd package.

Thanks. I didn't like the way I did this one. asset file url is a much better idea.

👍

* containerd config is done at the moment outside nodeup. The reason it didn't change was because of logic moving to nodeup that we want to avoid. Again, topic for office hours. Though, it either should move completely to nodeup or manage it outside it as we do now.

If we find a way to do this in cloudup I'd also like to see if this can be put the final config aux. I don't like using the override flag as a carry mechanism cloudup -> nodeup.

I agree, but to keep this on track we can do it later, when the config aux is merged. I agree that aux would be a better place for it.

[olemarkus]

So if we don't allow mixing GPU and non-GPU instance types, building the config cloudup should be fine.
I would need to send some setting to nodeup telling it to do those package installs. Again seems like something worth putting on aux.

[hakman]

A prerequisite would be to allow configuring containerd for each IG, not sure if possible now. Once that is possible, the package installs can just be done in the ContainerRuntime builder. Package installs would still be hardcoded I guess for now based on OS type.

[olemarkus]

Right. I think that is a challenge with how nodeup config is being built. Probably the containerd config should be built based on IG and cluster spec and then written to one of the configs rather than writing it back to the IG/cluster spec. But we are a long way away from that.

I think continuing the current path makes sense for now. There is nothing here that is exposed to the user, so things can be moved later on (maybe even before 1.22 GA)

[olemarkus]

I didn't do many of the APT changes. Using assets was challenging as it is arch dependent, which doesn't make sense in this case. It could also run into run-time issues if we use hashes (upstream can change keys) and it is the TLS cert we trust here.

[olemarkus]

I'll defer device addon to another PR.

[johngmyers]

Determining the arch of an instance and whether or not it has a GPU appears to be firmly in the bailiwick of nodeup. I don't see why there's an objection to putting such logic in there. Making the admin configure this through the API is just causing them grief.

I do not understand or agree with this desire to make nodeup dumb. It should do the things it is well suited for and not do the things it is not well suited for.

[hakman]

GPU stuff is something that very few operators need or use. Checking each node in every cluster if it has that capability just for that is not exactly ideal. More over, this has to work on the various supported platforms, not just AWS.

[johngmyers]

Can't nodeup to the equivalent of lspci or somesuch?

[justinsb]

I think this is a behavioral change? i.e. workloads that previously ran on GPU instances will need to add this toleration to remain schedulable?

Or is there a webhook or similar that will add this automatically?

[olemarkus]

Good catch. I think this one should additionally be blocked by NvidiaGPU.enabled. That way one has explicitly subscribed to kOps way of doing things. I'll add docs to match as well.

[justinsb]

I was originally a little sad that we couldn't get this from the metadata or DescribeInstances. But I came round to your point of view, because InstanceTypes are generic data that won't really be sensitive, and if there's one we should get rid of it's probably DescribeInstances :-)

[justinsb]

I think this looks great, and as we agreed in office hours we can nest it under containerd.

My one remaining challenge is the automatic taint, because I'm worried that we'll break clusters that upgrade that are using GPU instances until users add that toleration. Or is there some other mechanism at play here: e.g. everyone is already using this toleration, or there's a webhook that adds it?

[olemarkus]

These concerns should be addressed now.

[justinsb]

Thanks @olemarkus

This lgtm now and we did agree on this at our last discussion; I'm going to approve but hold until after office hours.

/approve
/lgtm
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hakman]

/lgtm

[olemarkus]

/hold cancel