Repository: opencontainers/runc
Pull request #3041 (2021-06-25 02:24:11)

libcontainer/intelrdt: cleanups
A bunch of cleanups for the code in `libcontainer/intelrdt`, mostly to simplify or remove the code, with occasional minor improvements and one nil dereference fix. See individual commit for details.

Please review commit-by-commit.

kolyshkin: @marquiz @Creatone @xiaochenshen PTAL; I'd appreciate your reviews.
xiaochenshen: @kolyshkin Thank you very much for the code cleanups and bug fixing for Intel RDT.

Three tiny typos in subject of this PR, description of this PR and subject of last commit: 😃 
libcontainer/intelrtd  --> libcontainer/intelrdt

kolyshkin: > Three tiny typos in subject of this PR, description of this PR and subject of last commit: smiley
> libcontainer/intelrtd --> libcontainer/intelrdt

@xiaochenshen thanks for your review!

Fixed, and I also found a 4th one, in a commit before last, also fixed:
```diff
@@ -400,7 +400,7 @@ func writeFile(dir, file, data string) error {
                return fmt.Errorf("no such directory for %s", file)
        }
        if err := ioutil.WriteFile(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil {
-               return NewLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err))
+               return NewLastCmdError(fmt.Errorf("intelrtd: unable to write %v: %w", data, err))
        }
        return nil
 }
```

Patch:
diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go
index bc01310e..9c32aabd 100644
--- a/libcontainer/intelrdt/intelrdt.go
+++ b/libcontainer/intelrdt/intelrdt.go
@@ -183,7 +183,7 @@ func NewManager(config *configs.Config, id string, path string) Manager {
 }
 
 const (
-	IntelRdtTasks = "tasks"
+	intelRdtTasks = "tasks"
 )
 
 var (
@@ -200,6 +200,8 @@ var (
 
 	// For Intel RDT initialization
 	initOnce sync.Once
+
+	errNotFound = errors.New("Intel RDT resctrl mount point not found")
 )
 
 type intelRdtData struct {
@@ -273,7 +275,7 @@ func findIntelRdtMountpointDir(f io.Reader) (string, error) {
 		return "", err
 	}
 	if len(mi) < 1 {
-		return "", NewNotFoundError("Intel RDT")
+		return "", errNotFound
 	}
 
 	// Check if MBA Software Controller is enabled through mount option "-o mba_MBps"
@@ -398,7 +400,7 @@ func writeFile(dir, file, data string) error {
 		return fmt.Errorf("no such directory for %s", file)
 	}
 	if err := ioutil.WriteFile(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil {
-		return fmt.Errorf("failed to write %v: %w", data, err)
+		return newLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err))
 	}
 	return nil
 }
@@ -499,13 +501,13 @@ func getLastCmdStatus() (string, error) {
 // WriteIntelRdtTasks writes the specified pid into the "tasks" file
 func WriteIntelRdtTasks(dir string, pid int) error {
 	if dir == "" {
-		return fmt.Errorf("no such directory for %s", IntelRdtTasks)
+		return fmt.Errorf("no such directory for %s", intelRdtTasks)
 	}
 
 	// Don't attach any pid if -1 is specified as a pid
 	if pid != -1 {
-		if err := ioutil.WriteFile(filepath.Join(dir, IntelRdtTasks), []byte(strconv.Itoa(pid)), 0o600); err != nil {
-			return fmt.Errorf("failed to write %v: %w", pid, err)
+		if err := ioutil.WriteFile(filepath.Join(dir, intelRdtTasks), []byte(strconv.Itoa(pid)), 0o600); err != nil {
+			return newLastCmdError(fmt.Errorf("intelrdt: unable to add pid %d: %w", pid, err))
 		}
 	}
 	return nil
@@ -547,7 +549,7 @@ func (m *intelRdtManager) Apply(pid int) (err error) {
 		return nil
 	}
 	d, err := getIntelRdtData(m.config, pid)
-	if err != nil && !IsNotFound(err) {
+	if err != nil {
 		return err
 	}
 
@@ -591,7 +593,7 @@ func (m *intelRdtManager) GetStats() (*Stats, error) {
 
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	stats := NewStats()
+	stats := newStats()
 
 	rootPath, err := getIntelRdtRoot()
 	if err != nil {
@@ -723,21 +725,21 @@ func (m *intelRdtManager) Set(container *configs.Config) error {
 		// Write a single joint schema string to schemata file
 		if l3CacheSchema != "" && memBwSchema != "" {
 			if err := writeFile(path, "schemata", l3CacheSchema+"\n"+memBwSchema); err != nil {
-				return NewLastCmdError(err)
+				return err
 			}
 		}
 
 		// Write only L3 cache schema string to schemata file
 		if l3CacheSchema != "" && memBwSchema == "" {
 			if err := writeFile(path, "schemata", l3CacheSchema); err != nil {
-				return NewLastCmdError(err)
+				return err
 			}
 		}
 
 		// Write only memory bandwidth schema string to schemata file
 		if l3CacheSchema == "" && memBwSchema != "" {
 			if err := writeFile(path, "schemata", memBwSchema); err != nil {
-				return NewLastCmdError(err)
+				return err
 			}
 		}
 	}
@@ -748,50 +750,19 @@ func (m *intelRdtManager) Set(container *configs.Config) error {
 func (raw *intelRdtData) join(id string) (string, error) {
 	path := filepath.Join(raw.root, id)
 	if err := os.MkdirAll(path, 0o755); err != nil {
-		return "", NewLastCmdError(err)
+		return "", newLastCmdError(err)
 	}
 
 	if err := WriteIntelRdtTasks(path, raw.pid); err != nil {
-		return "", NewLastCmdError(err)
+		return "", err
 	}
 	return path, nil
 }
 
-type NotFoundError struct {
-	ResourceControl string
-}
-
-func (e *NotFoundError) Error() string {
-	return fmt.Sprintf("mountpoint for %s not found", e.ResourceControl)
-}
-
-func NewNotFoundError(res string) error {
-	return &NotFoundError{
-		ResourceControl: res,
-	}
-}
-
-func IsNotFound(err error) bool {
-	var nfErr *NotFoundError
-	return errors.As(err, &nfErr)
-}
-
-type LastCmdError struct {
-	LastCmdStatus string
-	Err           error
-}
-
-func (e *LastCmdError) Error() string {
-	return e.Err.Error() + ", last_cmd_status: " + e.LastCmdStatus
-}
-
-func NewLastCmdError(err error) error {
-	lastCmdStatus, err1 := getLastCmdStatus()
+func newLastCmdError(err error) error {
+	status, err1 := getLastCmdStatus()
 	if err1 == nil {
-		return &LastCmdError{
-			LastCmdStatus: lastCmdStatus,
-			Err:           err,
-		}
+		return fmt.Errorf("%w, last_cmd_status: %s", err, status)
 	}
 	return err
 }
diff --git a/libcontainer/intelrdt/stats.go b/libcontainer/intelrdt/stats.go
index 70df0d14..4f3bd84e 100644
--- a/libcontainer/intelrdt/stats.go
+++ b/libcontainer/intelrdt/stats.go
@@ -54,6 +54,6 @@ type Stats struct {
 	CMTStats *[]CMTNumaNodeStats `json:"cmt_stats,omitempty"`
 }
 
-func NewStats() *Stats {
+func newStats() *Stats {
 	return &Stats{}
 }
diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index cbcb7d14..89172316 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -300,12 +300,9 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 			config.Seccomp = seccomp
 		}
 		if spec.Linux.IntelRdt != nil {
-			config.IntelRdt = &configs.IntelRdt{}
-			if spec.Linux.IntelRdt.L3CacheSchema != "" {
-				config.IntelRdt.L3CacheSchema = spec.Linux.IntelRdt.L3CacheSchema
-			}
-			if spec.Linux.IntelRdt.MemBwSchema != "" {
-				config.IntelRdt.MemBwSchema = spec.Linux.IntelRdt.MemBwSchema
+			config.IntelRdt = &configs.IntelRdt{
+				L3CacheSchema: spec.Linux.IntelRdt.L3CacheSchema,
+				MemBwSchema:   spec.Linux.IntelRdt.MemBwSchema,
 			}
 		}
 	}
@@ -313,9 +310,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
 		config.OomScoreAdj = spec.Process.OOMScoreAdj
 		config.NoNewPrivileges = spec.Process.NoNewPrivileges
 		config.Umask = spec.Process.User.Umask
-		if spec.Process.SelinuxLabel != "" {
-			config.ProcessLabel = spec.Process.SelinuxLabel
-		}
+		config.ProcessLabel = spec.Process.SelinuxLabel
 		if spec.Process.Capabilities != nil {
 			config.Capabilities = &configs.Capabilities{
 				Bounding:    spec.Process.Capabilities.Bounding,
@@ -551,12 +546,8 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi
 				if r.CPU.RealtimePeriod != nil {
 					c.Resources.CpuRtPeriod = *r.CPU.RealtimePeriod
 				}
-				if r.CPU.Cpus != "" {
-					c.Resources.CpusetCpus = r.CPU.Cpus
-				}
-				if r.CPU.Mems != "" {
-					c.Resources.CpusetMems = r.CPU.Mems
-				}
+				c.Resources.CpusetCpus = r.CPU.Cpus
+				c.Resources.CpusetMems = r.CPU.Mems
 			}
 			if r.Pids != nil {
 				c.Resources.PidsLimit = r.Pids.Limit


Test patch:
diff --git a/libcontainer/intelrdt/intelrdt_test.go b/libcontainer/intelrdt/intelrdt_test.go
index ef7abd33..b3f6ed97 100644
--- a/libcontainer/intelrdt/intelrdt_test.go
+++ b/libcontainer/intelrdt/intelrdt_test.go
@@ -3,6 +3,7 @@
 package intelrdt
 
 import (
+	"errors"
 	"io"
 	"strings"
 	"testing"
@@ -223,8 +224,8 @@ func TestFindIntelRdtMountpointDir(t *testing.T) {
 			mbaScEnabled = false
 			mp, err := findIntelRdtMountpointDir(tc.input)
 			if tc.isNotFoundError {
-				if !IsNotFound(err) {
-					t.Errorf("expected IsNotFound error, got %+v", err)
+				if !errors.Is(err, errNotFound) {
+					t.Errorf("expected errNotFound error, got %+v", err)
 				}
 				return
 			}