Repository: opencontainers/runc
Pull request #2479 (2020-06-17 20:31:09)

systemd: parse systemdVersion when only an int is returned
there have been cases observed where instead of `v$VER.0-$OS` the systemdVersion returned is just `$VER`. handle this case

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander: fixes failure found [here](https://storage.googleapis.com/origin-federated-results/pr-logs/pull/cri-o_cri-o/3868/test_pull_request_crio_integration_rhel/13239/artifacts/testout.txt)
haircommander: oops, I messed up the quotes handling
kolyshkin: So, I guess, we should start handling the string by trimming the quotes, then trimming the `v` prefix, then trimming the `.whatever` suffix, then using Atoi. A shortcut suggested by @thaJeztah above also works (as we don't care if there's `v` at the end.
haircommander: > left a suggestion
> 
> also curious if the situation where this problem happened is a standard distro, or if someone made a "whoopsie" when compiling :sweat_smile:

I ran into this on a rhel 7 box in AWS. I would not rule out a messed up compilation :laughing: 
haircommander: since there do not 

> Please add UT

tests added, and comments addressed, PTAL
haircommander: PTAL @kolyshkin @mrunalp 
haircommander: hold on, found another case
haircommander: ref: https://storage.googleapis.com/origin-federated-results/pr-logs/pull/cri-o_cri-o/3891/test_pull_request_crio_e2e_fedora/13724/artifacts/e2e.log

Patch:
diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go
index cdf656b8..53fa3f2c 100644
--- a/libcontainer/cgroups/systemd/common.go
+++ b/libcontainer/cgroups/systemd/common.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math"
 	"os"
+	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -365,22 +366,28 @@ func systemdVersion(conn *systemdDbus.Conn) (int, error) {
 			return
 		}
 
-		// verStr is like "v245.4-1.fc32" (including quotes)
-		if !strings.HasPrefix(verStr, `"v`) {
-			versionErr = fmt.Errorf("can't parse version %s", verStr)
-			return
-		}
-		// remove `"v` prefix and everything after the first dot
-		ver, err := strconv.Atoi(strings.SplitN(verStr[2:], ".", 2)[0])
-		if err != nil {
-			versionErr = err
-		}
-		version = ver
+		version, versionErr = systemdVersionAtoi(verStr)
+		return
 	})
 
 	return version, versionErr
 }
 
+func systemdVersionAtoi(verStr string) (int, error) {
+	// verStr should be of the form:
+	// "v245.4-1.fc32", "245", "v245-1.fc32", "245-1.fc32"
+	// all the input strings include quotes, and the output int should be 245
+	// thus, we unconditionally remove the `"v`
+	// and then match on the first integer we can grab
+	re := regexp.MustCompile(`"?v?([0-9]+)`)
+	matches := re.FindStringSubmatch(verStr)
+	if len(matches) < 2 {
+		return 0, errors.Errorf("can't parse version %s: incorrect number of matches %v", verStr, matches)
+	}
+	ver, err := strconv.Atoi(matches[1])
+	return ver, errors.Wrapf(err, "can't parse version %s", verStr)
+}
+
 func addCpuQuota(conn *systemdDbus.Conn, properties *[]systemdDbus.Property, quota int64, period uint64) {
 	if period != 0 {
 		// systemd only supports CPUQuotaPeriodUSec since v242
diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go
index 6e88b5df..b86c4151 100644
--- a/libcontainer/cgroups/utils.go
+++ b/libcontainer/cgroups/utils.go
@@ -138,7 +138,7 @@ func readProcsFile(file string) ([]int, error) {
 			out = append(out, pid)
 		}
 	}
-	return out, s.Err()
+	return out, nil
 }
 
 // ParseCgroupFile parses the given cgroup file, typically /proc/self/cgroup


Test patch:
diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go
new file mode 100644
index 00000000..0c98a117
--- /dev/null
+++ b/libcontainer/cgroups/systemd/systemd_test.go
@@ -0,0 +1,32 @@
+package systemd
+
+import (
+	"testing"
+)
+
+func TestSystemdVersion(t *testing.T) {
+	var systemdVersionTests = []struct {
+		verStr      string
+		expectedVer int
+		expectErr   bool
+	}{
+		{`"219"`, 219, false},
+		{`"v245.4-1.fc32"`, 245, false},
+		{`"241-1"`, 241, false},
+		{`"v241-1"`, 241, false},
+		{"NaN", 0, true},
+		{"", 0, true},
+	}
+	for _, sdTest := range systemdVersionTests {
+		ver, err := systemdVersionAtoi(sdTest.verStr)
+		if !sdTest.expectErr && err != nil {
+			t.Errorf("systemdVersionAtoi(%s); want nil; got %v", sdTest.verStr, err)
+		}
+		if sdTest.expectErr && err == nil {
+			t.Errorf("systemdVersionAtoi(%s); wanted failure; got nil", sdTest.verStr)
+		}
+		if ver != sdTest.expectedVer {
+			t.Errorf("systemdVersionAtoi(%s); want %d; got %d", sdTest.verStr, sdTest.expectedVer, ver)
+		}
+	}
+}