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

refactor(nodeadm): rework containerd template mixin interface #2135

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions nodeadm/internal/containerd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,15 @@ type containerdTemplateVars struct {
RuntimeBinaryName string
}

func writeContainerdConfig(cfg *api.NodeConfig) error {
if err := writeBaseRuntimeSpec(cfg); err != nil {
return err
}
type modifier interface {
// Modify mutates a given containerd template configuration.
Modify(*containerdTemplateVars)

// Matches determines whether the configurator is relevant to the current instance.
Matches(*api.NodeConfig) bool
}

func writeContainerdConfig(cfg *api.NodeConfig) error {
containerdConfig, err := generateContainerdConfig(cfg)
if err != nil {
return err
Expand All @@ -59,12 +63,17 @@ func writeContainerdConfig(cfg *api.NodeConfig) error {
}

func generateContainerdConfig(cfg *api.NodeConfig) ([]byte, error) {
instanceOptions := applyInstanceTypeMixins(cfg.Status.Instance.Type)

configVars := containerdTemplateVars{
SandboxImage: cfg.Status.Defaults.SandboxImage,
RuntimeBinaryName: instanceOptions.RuntimeBinaryName,
RuntimeName: instanceOptions.RuntimeName,
RuntimeBinaryName: "/usr/sbin/runc",
RuntimeName: "runc",
}
for _, configurator := range []modifier{
NewNvidiaModifier(),
} {
if configurator.Matches(cfg) {
configurator.Modify(&configVars)
}
}
var buf bytes.Buffer
if err := containerdConfigTemplate.Execute(&buf, configVars); err != nil {
Expand Down
3 changes: 3 additions & 0 deletions nodeadm/internal/containerd/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ func NewContainerdDaemon(daemonManager daemon.DaemonManager) daemon.Daemon {
}

func (cd *containerd) Configure(c *api.NodeConfig) error {
if err := writeBaseRuntimeSpec(c); err != nil {
return err
}
Comment on lines +23 to +25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this about? Doesn't seem related to the other changes

Copy link
Member Author

@ndbaker1 ndbaker1 Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top level daemon config calls should contain separate components and i didn't like how the runtime spec was nested inside the config.toml call.

i think the kubelet daemon sets the right precedent

func (k *kubelet) Configure(cfg *api.NodeConfig) error {
if err := k.writeKubeletConfig(cfg); err != nil {
return err
}
if err := k.writeKubeconfig(cfg); err != nil {
return err
}
if err := k.writeImageCredentialProviderConfig(cfg); err != nil {
return err
}
if err := writeClusterCaCert(cfg.Spec.Cluster.CertificateAuthority); err != nil {
return err
}
if err := k.writeKubeletEnvironment(cfg); err != nil {
return err
}
return nil

return writeContainerdConfig(c)
}

Expand Down
65 changes: 65 additions & 0 deletions nodeadm/internal/containerd/nvidia.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package containerd

import (
"os"
"slices"
"strings"

"github.com/awslabs/amazon-eks-ami/nodeadm/internal/api"
"go.uber.org/zap"
)

type nvidiaModifier struct {
pcieDevicesPath string
}

func NewNvidiaModifier() *nvidiaModifier {
return &nvidiaModifier{
pcieDevicesPath: "/proc/bus/pci/devices",
}
}

func (m *nvidiaModifier) Matches(cfg *api.NodeConfig) bool {
return m.matchesInstanceType(cfg.Status.Instance.Type) || m.matchesPCIeVendor()
}

func (*nvidiaModifier) Modify(ctrdTemplate *containerdTemplateVars) {
zap.L().Info("Configuring NVIDIA runtime..")
ctrdTemplate.RuntimeName = "nvidia"
ctrdTemplate.RuntimeBinaryName = "/usr/bin/nvidia-container-runtime"
}

var nvidiaInstanceFamilies = []string{
"p3", "p3dn",
"p4d", "p4de",
"p5", "p5e", "p5en",
"g4", "g4dn",
"g5", "g5g",
"g6", "g6e",
}

// TODO: deprecate to avoid manual instance type tracking.
func (*nvidiaModifier) matchesInstanceType(instanceType string) bool {
family := strings.Split(instanceType, ".")[0]
return slices.Contains(nvidiaInstanceFamilies, family)
}

func (m *nvidiaModifier) matchesPCIeVendor() bool {
devices, err := os.ReadFile(m.pcieDevicesPath)
if err != nil {
zap.L().Error("Failed to read PCIe devices", zap.Error(err))
return false
}
// The contents of '/proc/bus/pci/devices' looks like the following, where
// the last column contains the vendor name if present.
//
// something like the following:
//
// 0018 1d0f1111 0 c1000008 0 0 0 0 0 c0002 400000 0 0 0 0 0 20000
// 0020 1d0f8061 b c1508000 0 0 0 0 0 0 4000 0 0 0 0 0 0 nvme
// 0028 1d0fec20 0 c1504000 0 c1400008 0 0 0 0 4000 0 100000 0 0 0 0 ena
// 00f0 10de1eb8 a c0000000 44000000c 0 45000000c 0 0 0 1000000 10000000 0 2000000 0 0 0 nvidia
// 00f8 1d0fcd01 0 c1500000 0 c150c008 0 0 0 0 4000 0 2000 0 0 0 0 nvme
// 0030 1d0fec20 0 c1510000 0 c1600008 0 0 0 0 4000 0 100000 0 0 0 0 ena
return strings.Contains(string(devices), "nvidia")
}
47 changes: 47 additions & 0 deletions nodeadm/internal/containerd/nvidia_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package containerd

import (
"os"
"path/filepath"
"testing"

"github.com/awslabs/amazon-eks-ami/nodeadm/internal/api"
"github.com/stretchr/testify/assert"
)

func TestNvidiaConfigurator(t *testing.T) {

t.Run("IsNvidiaUsingInstanceType", func(t *testing.T) {
configurator := nvidiaModifier{}
template := containerdTemplateVars{}
assert.True(t, configurator.Matches(nvidiaInstanceTypeNodeConfig("p5.48xlarge")))
configurator.Modify(&template)
assert.Equal(t, "nvidia", template.RuntimeName)
assert.Equal(t, "/usr/bin/nvidia-container-runtime", template.RuntimeBinaryName)
})

t.Run("IsNvidiaUsingPCIe", func(t *testing.T) {
configurator := nvidiaModifier{pcieDevicesPath: filepath.Join(t.TempDir(), "pcie-devices")}
os.WriteFile(configurator.pcieDevicesPath, []byte("nvidia"), 0777)
template := containerdTemplateVars{}
assert.True(t, configurator.Matches(nvidiaInstanceTypeNodeConfig("xxx.xxxxx")))
configurator.Modify(&template)
assert.Equal(t, "nvidia", template.RuntimeName)
assert.Equal(t, "/usr/bin/nvidia-container-runtime", template.RuntimeBinaryName)
})

t.Run("IsNotNvidia", func(t *testing.T) {
configurator := nvidiaModifier{}
assert.False(t, configurator.Matches(nvidiaInstanceTypeNodeConfig("m5.large")))
})
}

func nvidiaInstanceTypeNodeConfig(instanceType string) *api.NodeConfig {
return &api.NodeConfig{
Status: api.NodeConfigStatus{
Instance: api.InstanceDetails{
Type: instanceType,
},
},
}
}
65 changes: 0 additions & 65 deletions nodeadm/internal/containerd/runtime_config.go

This file was deleted.

31 changes: 0 additions & 31 deletions nodeadm/internal/containerd/runtime_config_test.go

This file was deleted.

Loading