From b081558086a18f3a6b8aa82bd75ee6ce5663f112 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 7 Feb 2024 17:59:12 -0500 Subject: [PATCH 1/2] client: prevent start on cgroups init error The Nomad client expects certain cgroups paths to exist in order to manage tasks. These paths are created when the agent first starts, but if process fails the agent would just log the error and proceed with its initialization, despite not being able to run tasks. This commit surfaces the errors back to the client initialization so the process can stop early and make clear to operators that something went wrong. --- client/client.go | 5 +- client/lib/cgroupslib/init.go | 62 +++++++++--------------- client/lib/proclib/wrangler_cg1_linux.go | 10 ++-- client/lib/proclib/wrangler_cg2_linux.go | 10 ++-- client/lib/proclib/wrangler_default.go | 4 +- client/lib/proclib/wrangler_linux.go | 9 ++-- 6 files changed, 48 insertions(+), 52 deletions(-) diff --git a/client/client.go b/client/client.go index 390e7a7b5ba..8f041bc41f6 100644 --- a/client/client.go +++ b/client/client.go @@ -477,10 +477,13 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulProxie ) // Create the process wranglers - wranglers := proclib.New(&proclib.Configs{ + wranglers, err := proclib.New(&proclib.Configs{ UsableCores: c.topology.UsableCores(), Logger: c.logger.Named("proclib"), }) + if err != nil { + return nil, fmt.Errorf("failed to initialize process manager: %w", err) + } c.wranglers = wranglers // Build the allow/denylists of drivers. diff --git a/client/lib/cgroupslib/init.go b/client/lib/cgroupslib/init.go index c09c93bdd94..18726e6ea65 100644 --- a/client/lib/cgroupslib/init.go +++ b/client/lib/cgroupslib/init.go @@ -7,6 +7,7 @@ package cgroupslib import ( "bytes" + "fmt" "os" "path/filepath" @@ -23,7 +24,7 @@ const ( // Init will initialize the cgroup tree that the Nomad client will use for // isolating resources of tasks. cores is the cpuset granted for use by Nomad. -func Init(log hclog.Logger, cores string) { +func Init(log hclog.Logger, cores string) error { log.Info("initializing nomad cgroups", "cores", cores) switch GetMode() { @@ -41,8 +42,7 @@ func Init(log hclog.Logger, cores string) { for _, ctrl := range controllers { p := filepath.Join(root, ctrl, NomadCgroupParent) if err := os.MkdirAll(p, 0755); err != nil { - log.Error("failed to create nomad cgroup", "controller", ctrl, "error", err) - return + return fmt.Errorf("failed to create nomad cgroup %s: %w", ctrl, err) } } @@ -56,8 +56,7 @@ func Init(log hclog.Logger, cores string) { // band from nomad itself var memsSet string if mems, err := detectMemsCG1(); err != nil { - log.Error("failed to detect memset", "error", err) - return + return fmt.Errorf("failed to detect memset: %w", err) } else { memsSet = mems } @@ -78,18 +77,15 @@ func Init(log hclog.Logger, cores string) { // def456.task/{cgroup.procs, cpuset.cpus, cpuset.mems} if err := writeCG(noClone, "cpuset", NomadCgroupParent, cloneFile); err != nil { - log.Error("failed to set clone_children on nomad cpuset cgroup", "error", err) - return + return fmt.Errorf("failed to set clone_children on nomad cpuset cgroup: %w", err) } if err := writeCG(memsSet, "cpuset", NomadCgroupParent, memsFile); err != nil { - log.Error("failed to set cpuset.mems on nomad cpuset cgroup", "error", err) - return + return fmt.Errorf("failed to set cpuset.mems on nomad cpuset cgroup: %w", err) } if err := writeCG(cores, "cpuset", NomadCgroupParent, cpusetFile); err != nil { - log.Error("failed to write cores to nomad cpuset cgroup", "error", err) - return + return fmt.Errorf("failed to write cores to nomad cpuset cgroup: %w", err) } // @@ -97,18 +93,15 @@ func Init(log hclog.Logger, cores string) { // if err := mkCG("cpuset", NomadCgroupParent, SharePartition()); err != nil { - log.Error("failed to create share cpuset partition", "error", err) - return + return fmt.Errorf("failed to create share cpuset partition: %w", err) } if err := writeCG(noClone, "cpuset", NomadCgroupParent, SharePartition(), cloneFile); err != nil { - log.Error("failed to set clone_children on nomad cpuset cgroup", "error", err) - return + return fmt.Errorf("failed to set clone_children on nomad cpuset cgroup: %w", err) } if err := writeCG(memsSet, "cpuset", NomadCgroupParent, SharePartition(), memsFile); err != nil { - log.Error("failed to set cpuset.mems on share cpuset partition", "error", err) - return + return fmt.Errorf("failed to set cpuset.mems on share cpuset partition: %w", err) } // @@ -116,18 +109,15 @@ func Init(log hclog.Logger, cores string) { // if err := mkCG("cpuset", NomadCgroupParent, ReservePartition()); err != nil { - log.Error("failed to create reserve cpuset partition", "error", err) - return + return fmt.Errorf("failed to create reserve cpuset partition: %w", err) } if err := writeCG(noClone, "cpuset", NomadCgroupParent, ReservePartition(), cloneFile); err != nil { - log.Error("failed to set clone_children on nomad cpuset cgroup", "error", err) - return + return fmt.Errorf("failed to set clone_children on nomad cpuset cgroup: %w", err) } if err := writeCG(memsSet, "cpuset", NomadCgroupParent, ReservePartition(), memsFile); err != nil { - log.Error("failed to set cpuset.mems on reserve cpuset partition", "error", err) - return + return fmt.Errorf("failed to set cpuset.mems on reserve cpuset partition: %w", err) } log.Debug("nomad cpuset partitions initialized", "cores", cores) @@ -144,8 +134,7 @@ func Init(log hclog.Logger, cores string) { // if err := writeCG(activation, subtreeFile); err != nil { - log.Error("failed to create nomad cgroup", "error", err) - return + return fmt.Errorf("failed to create nomad cgroup: %w", err) } // @@ -153,18 +142,15 @@ func Init(log hclog.Logger, cores string) { // if err := mkCG(NomadCgroupParent); err != nil { - log.Error("failed to create nomad cgroup", "error", err) - return + return fmt.Errorf("failed to create nomad cgroup: %w", err) } if err := writeCG(activation, NomadCgroupParent, subtreeFile); err != nil { - log.Error("failed to set subtree control on nomad cgroup", "error", err) - return + return fmt.Errorf("failed to set subtree control on nomad cgroup: %w", err) } if err := writeCG(cores, NomadCgroupParent, cpusetFile); err != nil { - log.Error("failed to write root partition cpuset", "error", err) - return + return fmt.Errorf("failed to write root partition cpuset: %w", err) } log.Debug("top level partition root nomad.slice cgroup initialized") @@ -174,13 +160,11 @@ func Init(log hclog.Logger, cores string) { // if err := mkCG(NomadCgroupParent, SharePartition()); err != nil { - log.Error("failed to create share cgroup", "error", err) - return + return fmt.Errorf("failed to create share cgroup: %w", err) } if err := writeCG(activation, NomadCgroupParent, SharePartition(), subtreeFile); err != nil { - log.Error("failed to set subtree control on cpuset share partition", "error", err) - return + return fmt.Errorf("failed to set subtree control on cpuset share partition: %w", err) } log.Debug("partition member nomad.slice/share cgroup initialized") @@ -190,17 +174,17 @@ func Init(log hclog.Logger, cores string) { // if err := mkCG(NomadCgroupParent, ReservePartition()); err != nil { - log.Error("failed to create share cgroup", "error", err) - return + return fmt.Errorf("failed to create share cgroup: %w", err) } if err := writeCG(activation, NomadCgroupParent, ReservePartition(), subtreeFile); err != nil { - log.Error("failed to set subtree control on cpuset reserve partition", "error", err) - return + return fmt.Errorf("failed to set subtree control on cpuset reserve partition: %w", err) } log.Debug("partition member nomad.slice/reserve cgroup initialized") } + + return nil } // detectMemsCG1 will determine the cpuset.mems value to use for diff --git a/client/lib/proclib/wrangler_cg1_linux.go b/client/lib/proclib/wrangler_cg1_linux.go index a8654a6337d..0d284b23e75 100644 --- a/client/lib/proclib/wrangler_cg1_linux.go +++ b/client/lib/proclib/wrangler_cg1_linux.go @@ -23,16 +23,20 @@ type LinuxWranglerCG1 struct { cg cgroupslib.Lifecycle } -func newCG1(c *Configs) create { +func newCG1(c *Configs) (create, error) { logger := c.Logger.Named("cg1") - cgroupslib.Init(logger, c.UsableCores.String()) + err := cgroupslib.Init(logger, c.UsableCores.String()) + if err != nil { + return nil, err + } + return func(task Task) ProcessWrangler { return &LinuxWranglerCG1{ task: task, log: logger, cg: cgroupslib.Factory(task.AllocID, task.Task, task.Cores), } - } + }, nil } func (w *LinuxWranglerCG1) Initialize() error { diff --git a/client/lib/proclib/wrangler_cg2_linux.go b/client/lib/proclib/wrangler_cg2_linux.go index 8dcbf3dfe5c..f0293171369 100644 --- a/client/lib/proclib/wrangler_cg2_linux.go +++ b/client/lib/proclib/wrangler_cg2_linux.go @@ -20,16 +20,20 @@ type LinuxWranglerCG2 struct { cg cgroupslib.Lifecycle } -func newCG2(c *Configs) create { +func newCG2(c *Configs) (create, error) { logger := c.Logger.Named("cg2") - cgroupslib.Init(logger, c.UsableCores.String()) + err := cgroupslib.Init(logger, c.UsableCores.String()) + if err != nil { + return nil, err + } + return func(task Task) ProcessWrangler { return &LinuxWranglerCG2{ task: task, log: c.Logger, cg: cgroupslib.Factory(task.AllocID, task.Task, task.Cores), } - } + }, nil } func (w LinuxWranglerCG2) Initialize() error { diff --git a/client/lib/proclib/wrangler_default.go b/client/lib/proclib/wrangler_default.go index 4dc9d84f252..1fb5890e487 100644 --- a/client/lib/proclib/wrangler_default.go +++ b/client/lib/proclib/wrangler_default.go @@ -7,14 +7,14 @@ package proclib // New creates a Wranglers backed by the DefaultWrangler implementation, which // does not do anything. -func New(configs *Configs) *Wranglers { +func New(configs *Configs) (*Wranglers, error) { w := &Wranglers{ configs: configs, m: make(map[Task]ProcessWrangler), create: doNothing(configs), } - return w + return w, nil } func doNothing(*Configs) create { diff --git a/client/lib/proclib/wrangler_linux.go b/client/lib/proclib/wrangler_linux.go index 757f2ead3ad..3dc7bb7c198 100644 --- a/client/lib/proclib/wrangler_linux.go +++ b/client/lib/proclib/wrangler_linux.go @@ -11,18 +11,19 @@ import ( // New creates a Wranglers factory for creating ProcessWrangler's appropriate // for the given system (i.e. cgroups v1 or cgroups v2). -func New(configs *Configs) *Wranglers { +func New(configs *Configs) (*Wranglers, error) { w := &Wranglers{ configs: configs, m: make(map[Task]ProcessWrangler), } + var err error switch cgroupslib.GetMode() { case cgroupslib.CG1: - w.create = newCG1(configs) + w.create, err = newCG1(configs) default: - w.create = newCG2(configs) + w.create, err = newCG2(configs) } - return w + return w, err } From fc18dc64d8d7d3ac3dfe897b36907c5fdfed4d1b Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 7 Feb 2024 18:44:09 -0500 Subject: [PATCH 2/2] changelog: add entry for #19915 --- .changelog/19915.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/19915.txt diff --git a/.changelog/19915.txt b/.changelog/19915.txt new file mode 100644 index 00000000000..3b8668c61c9 --- /dev/null +++ b/.changelog/19915.txt @@ -0,0 +1,3 @@ +```release-note:bug +client: Prevent client from starting if cgroup initialization fails +```