More linting fixes (with golint/revive enabled) #2998

thaJeztah · 2021-06-03T21:32:04Z

extracted from #2966

See individual commits for details

thaJeztah · 2021-06-03T21:33:30Z

Remaining issues are outlined in #2966 (comment)

thaJeztah · 2021-06-03T21:49:13Z

@kolyshkin @AkihiroSuda ptal

libcontainer/cgroups/systemd/common.go

libcontainer/cgroups/utils.go

kolyshkin · 2021-06-04T01:45:27Z

libcontainer/cgroups/utils.go

+// ConvertCPUSharesToCgroupV2Value converts a CPU shares cgroup v1 configuration
+// to cgroup v2.
+//
+// The OCI spec is designed for cgroup v1, in some cases there is need to
+// convert from the cgroup v1 configuration to cgroup v2 the formula for cpuShares


The "in some cases" was a generic comment (as in "in some cases we need to convert from cgroup v1 to cgroup v2, and cpu shares is one such case").

The comment was talking about generic stuff first, when went to specifics of CPU shares.

I am not sure how to rephrase this in a good way if we must start with specifics (ConvertCPUSharesToCgroupV2Value). Maybe just skip the generic part?

Suggested change

// ConvertCPUSharesToCgroupV2Value converts a CPU shares cgroup v1 configuration

// to cgroup v2.

//

// The OCI spec is designed for cgroup v1, in some cases there is need to

// convert from the cgroup v1 configuration to cgroup v2 the formula for cpuShares

// ConvertCPUSharesToCgroupV2Value converts a CPU shares value

// from cgroup v1 to cgroup v2.

//

// The OCI spec is designed for cgroup v1, in some cases there is need to

// convert from the cgroup v1 configuration to cgroup v2. The formula for cpuShares

// is y = (1 + ((x - 2) * 9999) / 262142), which is a conversion from

// [2-262144] to [1-10000].

or some such.

(note you did not separated the two sentences, the second one starts with "the formula for")

OK I think the generic part (in some cases we need to convert) should be its own comment,
e.g.

// The OCI spec is designed for cgroup v1, in some cases there is need to // convert from the cgroup v1 configuration to cgroup v2. A few functions // below are doing such conversion. // ConvertCPUSharesToCgroupV2Value converts a CPU shares value // from cgroup v1 to cgroup v2. // // The formula for CPU shares is .... func ConvertCPUSharesToCgroupV2Value(cpuShares uint64) uint64 { ... // ConvertMemorySwapToCgroupV2Value converts MemorySwap value from OCI spec // ... func ConvertMemorySwapToCgroupV2Value(memorySwap, memory int64) (int64, error) { ... // ConvertBlkIOToIOWeightValue converts a BlkIOWeight cgroup v1 value // to cgroup v2. // // The formula for BlkIOWeight to IOWeight is y = (1 + (x - 10) * 9999 / 990), // as we convert linearly from [10-1000] to [1-10000]. func ....

and so on

libcontainer/cgroups/utils.go

libcontainer/cgroups/v1_utils.go

libcontainer/configs/cgroup_linux.go

libcontainer/configs/config.go

libcontainer/configs/devices.go

libcontainer/system/proc.go

libcontainer/intelrdt/intelrdt.go

kolyshkin · 2021-06-04T02:17:46Z

I have only reviewed the first two commits.

My gut feeling is we should be more conservative about how much changes do we want. Not just making linters warnings go away, but think about readability and maintainability first. IOW, let's take linter warnings as hints, rather than be linter slaves.

Say, if changing a comment makes a linter silent, but reduces the comment readability, this is not a good change.

Another example -- it does not make sense to fix comments describing some variables or struct fields that are only used for one particular test.

It does not make sense to try to clean up documentation for something that is deprecated.

OTOH if the changes are like "Sets the values" -> "Set sets the values", I am totally for it. I am not against renames like Cpu -> CPU either.

thaJeztah · 2021-06-05T13:19:29Z

It does not make sense to try to clean up documentation for something that is deprecated.

Addressed by #2999, and rebased this one because that was merged.

thaJeztah · 2021-07-05T20:08:20Z

Rebased, PTAL

kolyshkin · 2021-07-12T09:41:04Z

libcontainer/configs/namespaces_linux.go

@@ -6,6 +6,7 @@ import (
 	"sync"
 )

+// NamespaceType constants


nit: missing period.

I am finally addressing this in #3304, yay!

kolyshkin · 2021-07-12T09:54:38Z

libcontainer/system/proc.go

@@ -44,10 +46,10 @@ func (s State) String() string {
 	}
 }

-// Stat_t represents the information from /proc/[pid]/stat, as
+// StatT represents the information from /proc/[pid]/stat, as


I guess the name Stat_t was used as it resembles C / Unix stat_t type (which has _t suffix to distinguish it from stat() function), and is used here for the same reason (to distingush from Stat() function).

I like Stat_t much better, despite its non-conformance to Go standards. Note that golang.org/x/sys/unix (as well as stdlib's syscall package) also have names like Stat_t and Statfs_t.

Surely we can just rename it to e.g. ProcStat or something.

kolyshkin · 2021-07-12T10:00:17Z

libcontainer/intelrdt/cmt_test.go

@@ -13,10 +13,10 @@ func TestGetCMTNumaNodeStats(t *testing.T) {
 		"llc_occupancy": 9123911,
 	}

-	mockedL3_MON, err := mockResctrlL3_MON(mocksNUMANodesToCreate, mocksFilesToCreate)


I think it's called L3_MON because there is a directory with the same name under /sys/fs/resctrl (L3_MON), and thus rename will make things harder to find. Not super important of course but I'd rather keep this as is.

runc/libcontainer/intelrdt/intelrdt.go

Line 63 in 3a041e9

* | |-- L3_MON

kolyshkin · 2021-07-12T10:01:02Z

libcontainer/intelrdt/intelrdt.go

 	Apply(pid int) error

-	// Returns statistics for Intel RDT
+	// GetStats returns statistics for Intel RDT/


nit: s|/$|.|

kolyshkin · 2021-07-12T10:01:28Z

libcontainer/intelrdt/intelrdt.go

 	Set(container *configs.Config) error
 }

-// This implements interface Manager
+// intelRdtManager implements interface Manager


nit: missing period.

kolyshkin · 2021-07-12T10:21:52Z

Not quite sure we should rename all ThisId and ThatId to *ID. After all, Id is not an acronym, but short for Identifier.

Hmm, Wikipedia says it is. https://en.wikipedia.org/wiki/Identifier, so let it be.

thaJeztah · 2021-11-17T19:33:34Z

I need to have a look at rebasing this one, to see which commits are still relevant

thaJeztah · 2021-12-02T08:46:30Z

@kolyshkin I rebased this one

Warning: receiver-naming: receiver name source should be consistent with previous receiver name e for Emulator (revive) Warning: var-declaration: should omit type int from declaration of var cgroupFd; it will be inferred from the right-hand side (revive) Warning: var-naming: don't use underscores in Go names; type Stat_t should be StatT (revive) Warning: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive) Warning: var-naming: method parameter containerId should be containerID (revive) Warning: var-naming: method parameter containerId should be containerID (revive) Warning: exported: exported var ErrV1NoUnified should have comment or be unexported (revive) Warning: exported: comment on exported function FindCgroupMountpoint should be of the form "FindCgroupMountpoint ..." (revive) Warning: exported: comment on exported type Device should be of the form "Device ..." (with optional leading article) (revive) Warning: exported: comment on exported type DeviceRule should be of the form "DeviceRule ..." (with optional leading article) (revive) Warning: exported: comment on exported type DeviceType should be of the form "DeviceType ..." (with optional leading article) (revive) Warning: exported: comment on exported type DevicePermissions should be of the form "DevicePermissions ..." (with optional leading article) (revive) Warning: exported: comment on exported function ConvertCPUSharesToCgroupV2Value should be of the form "ConvertCPUSharesToCgroupV2Value ..." (revive) Warning: exported: comment on exported function ConvertBlkIOToIOWeightValue should be of the form "ConvertBlkIOToIOWeightValue ..." (revive) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Warning: var-naming: func parseCpuInfoFile should be parseCPUInfoFile (revive) Warning: var-naming: don't use underscores in Go names; func mockResctrlL3_MON should be mockResctrlL3MON (revive) Warning: var-naming: don't use underscores in Go names; var mockedL3_MON should be mockedL3MON (revive) libcontainer/intelrdt: fix various linting issues libcontainer/intelrdt/intelrdt.go:530:1: exported: comment on exported function IsCATEnabled should be of the form "IsCATEnabled ..." (revive) // Check if Intel RDT/CAT is enabled ^ libcontainer/intelrdt/intelrdt.go:536:1: exported: comment on exported function IsMBAEnabled should be of the form "IsMBAEnabled ..." (revive) // Check if Intel RDT/MBA is enabled ^ libcontainer/intelrdt/intelrdt.go:542:1: exported: comment on exported function IsMBAScEnabled should be of the form "IsMBAScEnabled ..." (revive) // Check if Intel RDT/MBA Software Controller is enabled ^ libcontainer/intelrdt/intelrdt.go:548:1: exported: comment on exported function GetIntelRdtPath should be of the form "GetIntelRdtPath ..." (revive) // Get the 'container_id' path in Intel RDT "resource control" filesystem ^ libcontainer/intelrdt/util_test.go:31:40: unexported-return: exported func NewIntelRdtTestUtil returns unexported type *intelrdt.intelRdtTestUtil, which can be annoying to use (revive) func NewIntelRdtTestUtil(t *testing.T) *intelRdtTestUtil { ^ libcontainer/indelrdt: un-export IntelRdtTasks It's only used within the package itself, so un-exporting libcontainer/intelrdt/intelrdt.go:184:2: exported: exported const IntelRdtTasks should have comment (or a comment on this block) or be unexported (revive) IntelRdtTasks = "tasks" ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libcontainer/system/linux.go:50:2: if-return: redundant if ...; err != nil check, just return error instead. (revive) if err := unix.Prctl(unix.PR_SET_PDEATHSIG, sig, 0, 0, 0); err != nil { return err } libcontainer/system/linux.go:65:2: if-return: redundant if ...; err != nil check, just return error instead. (revive) if err := unix.Prctl(unix.PR_SET_KEEPCAPS, 1, 0, 0, 0); err != nil { return err } libcontainer/system/linux.go:73:2: if-return: redundant if ...; err != nil check, just return error instead. (revive) if err := unix.Prctl(unix.PR_SET_KEEPCAPS, 0, 0, 0, 0); err != nil { return err } Signed-off-by: Sebastiaan van Stijn <[email protected]>

libcontainer/utils/cmsg.go:28:1: exported: comment on exported const MaxNameLen should be of the form "MaxNameLen ..." (revive) Signed-off-by: Sebastiaan van Stijn <[email protected]>

libcontainer/configs/config_linux.go:10:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) return -1, fmt.Errorf("User namespaces enabled, but no uid mappings found.") ^ libcontainer/configs/config_linux.go:14:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) return -1, fmt.Errorf("User namespaces enabled, but no user mapping found.") ^ libcontainer/configs/config_linux.go:33:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) return -1, fmt.Errorf("User namespaces enabled, but no gid mappings found.") ^ libcontainer/configs/config_linux.go:37:26: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) return -1, fmt.Errorf("User namespaces enabled, but no group mapping found.") ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Unexporting NewCgroupTestUtil to prevent linting warnings libcontainer/cgroups/fs/util_test.go:37:56: unexported-return: exported func NewCgroupTestUtil returns unexported type *fs.cgroupTestUtil, which can be annoying to use (revive) func NewCgroupTestUtil(subsystem string, t *testing.T) *cgroupTestUtil { ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

The CgroupNamePrefix looks like something unlikely to change (perhaps better for mountCgroupV1 to be moved to libcontainer/cgroups?). libcontainer/cgroups/v1_utils.go:21:2: exported: exported const CgroupNamePrefix should have comment (or a comment on this block) or be unexported (revive) libcontainer/cgroups/utils.go:25:2: exported: exported const CgroupProcesses should have comment (or a comment on this block) or be unexported (revive) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

notify_socket.go:97:1: receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive) func (n *notifySocket) waitForContainer(container libcontainer.Container) error { notify_socket.go:105:1: receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive) func (n *notifySocket) run(pid1 int) error { Signed-off-by: Sebastiaan van Stijn <[email protected]>

libcontainer/specconv/spec_linux_test.go:647:2: var-naming: var ttyUid should be ttyUID (revive) libcontainer/init_linux.go:456:7: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive) const _P_PID = 1 ^ libcontainer/state_linux.go:76:2: if-return: redundant if ...; err != nil check, just return error instead. (revive) libcontainer/init_linux.go:460:2: var-naming: don't use underscores in Go names; struct field si_signo should be siSigno (revive) libcontainer/init_linux.go:461:2: var-naming: don't use underscores in Go names; struct field si_errno should be siErrno (revive) libcontainer/init_linux.go:462:2: var-naming: don't use underscores in Go names; struct field si_code should be siCode (revive) libcontainer/init_linux.go:464:2: var-naming: don't use underscores in Go names; struct field si_pid should be siPid (revive) libcontainer/standard_init_linux.go:59:6: var-naming: var sessKeyId should be sessKeyID (revive) libcontainer/rootfs_linux.go:232:2: if-return: redundant if ...; err != nil check, just return error instead. (revive) libcontainer/rootfs_linux.go:920:11: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive) libcontainer/integration/exec_test.go:563:6: var-naming: func testCpuShares should be testCPUShares (revive) Signed-off-by: Sebastiaan van Stijn <[email protected]>

utils_linux.go:395:2: exported: exported const CT_ACT_CREATE should have comment (or a comment on this block) or be unexported (revive) CT_ACT_CREATE CtAct = iota + 1 ^ utils_linux.go:396:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive) CT_ACT_RUN ^ utils_linux.go:397:2: var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive) CT_ACT_RESTORE ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>