Less interfaces #3316

kolyshkin · 2021-12-09T23:01:17Z

This refactors the code to remove some interfaces, methods and tests that are not needed, and in general make code more comprehensible.

Splitting this one into more digestible pieces

kolyshkin · 2021-12-13T21:03:27Z

No longer a draft; if others agree, this can be merged right after 1.1

libcontainer/configs/config.go

libcontainer/container_linux.go

thaJeztah · 2022-01-19T11:17:17Z

libcontainer/container_linux.go

+// If the Container state is RUNNING or CREATED, sets the Container
+// state to PAUSING and pauses the execution of any user processes.


Suggested change

// If the Container state is RUNNING or CREATED, sets the Container

// state to PAUSING and pauses the execution of any user processes.

// Pause sets the Container state to PAUSING and pauses the execution of any

// user processes if the Container state is RUNNING or CREATED.

What this commit did is

Remove BaseContainer and Container interfaces, moving their methods
documentation to linuxContainer.

I was not changing any comments.

Also, it seems that the doc is wrong and the async part and "pausing" state was removed long time ago (I was not able to find it being used in git history).

Let's address this separately (on top of this PR once it's merged). Opened #3343 so we won't forget.

thaJeztah · 2022-01-19T11:20:27Z

libcontainer/container_linux.go

@@ -35,7 +35,7 @@ import (

 const stdioFdCount = 3

-type linuxContainer struct {
+type Container struct {


Perhaps copy the previous docs here?

Suggested change

type Container struct {

// Container is a libcontainer container object.

//

// Each container is thread-safe within the same process. Since a container can

// be destroyed by a separate process, any function may return that the container

// was not found.

type Container struct {

Frankly, I do not understand the part about thread-safeness, and the rest is kind of obvious.

thaJeztah · 2022-01-19T11:24:52Z

utils_linux.go

-	if err != nil {
-		newuidmap = ""
+	if newuidmap, err := exec.LookPath("newuidmap"); err == nil {
+		f.NewuidmapPath = newuidmap


TBH; not sure I like this direction; with these changes, it looks like loadFactory() becomes the actual constructor, and not New(). I feel like either New() should do the constructing, or we should remove New() altogether if it's not really a constructor

Yes, I guess I was going that way but fell short of the last step.

OK, with a few more commits here and there I think I did it. PTAL @thaJeztah

The only question is, should we call what's left of it a factory? I can certainly rename it but can't think of any good name -- suggestions welcome.

Almost tempted to say; perhaps it should be named LibContainer (as it's constructed using libcontainer.New() 🤔

Even further; I'm even starting to wonder if libcontainer.New() should be public at all. All cases except for getContainers() ("list containers"), basically follow the pattern;

factory := libcontainer.New()

factory.Create() / factory.Load()

done

It would almost make sense to just libcontainer.Create(), libcontainer.Get(), libcontainer.List() (which internally can do a new()), adding root as argument.

It would almost make sense to just libcontainer.Create(), libcontainer.Get(), libcontainer.List() (which internally can do a new()), adding root as argument.

This makes great sense (and we can get rid of factory entirely, so we don't have to think about naming it!).

libcontainer/factory.go

kolyshkin · 2022-01-25T20:26:46Z

Rebased; PTAL @thaJeztah

This method was removed earlier by commit 097c6d7, but the documentation was not updated. Fix it. Fixes: 097c6d7 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

It's clear at this point that runc won't support Windows. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Since the next commit is going to touch this structure, our CI (lint-extra) is about to complain about improperly named field: > Warning: var-naming: struct field ContainerId should be ContainerID (revive) Make it happy. Brought to use by gopls rename. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

StartInitialization does not have to be a method of Factory (while it is clear why it was done that way initially, now we only have Linux containers so it does not make sense). Fix callers and docs accordingly. No change in functionality. Also, since this was the only user of libcontainer.New with the empty string as an argument, the corresponding check can now be removed from it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Currently, TestInit sets up logrus, and init uses it to log an error from StartInitialization(). This is solely used by TestExecInError to check that error returned from StartInitialization is the one it expects. Note that the very same error is communicated to the runc init parent and is ultimately returned by container.Run(), so checking what StartInitialization returned is redundant. Remove logrus setup and use from TestMain/init, in preparation for the next commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Note that since this is now also called from libcontainer/integration, which do not set _LIBCONTAINER_LOGLEVEL, make it optional. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

> libcontainer/factory.go#L245 > var-naming: var fdsJson should be fdsJSON (revive) and > libcontainer/init_linux.go#L181 > error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) and > notify_socket.go#L94 > receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Instead of having newContainerInit return an interface, and let its caller call Init(), it is easier to call Init directly. Do that, and rename newContainerInit to containerInit. I think it makes the code more readable and straightforward. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Those are *always* /proc/self/exe init, and it does not make sense to change these (in case there ever will be, I guess, it can be addressed separately). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

We only have one implementation of config validator, which is always used. It makes no sense to have Validator interface. Having validate.Validator field in Factory does not make sense for all the same reasons. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

These were introduced in commit d8b6694 back in 2017, with a TODO of "make binary names configurable". Apparently, everyone is happy with the hardcoded names. In fact, they *are* configurable (by prepending the PATH with a directory containing own version of newuidmap/newgidmap). Now, these binaries are only needed in a few specific cases (when rootless is set etc.), so let's loop them up when needed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

All its users were removed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This was introduced in an initial commit, and is not really needed for most of the users, since they rely on CRIU binary provided by their distro. Now, if someone needs a custom criu binary to be used, they can put it into some directory and make sure this directory is listed first in $PATH. Make --criu a hidden option (thus removing it from help). Remove it from man pages, integration tests, etc. Remove all traces of CriuPath from data structures. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

- move filepath.Abs to libcontainer.New; - remove loadFactory. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2022-01-25T21:11:21Z

This PR lacked needed documentation changes in libcontainer/README. Added now.

kolyshkin · 2022-01-25T21:24:37Z

@adrianreber can you please review this single commit and tell us WDYT?

5ce43c4

adrianreber · 2022-01-26T13:42:44Z

@adrianreber can you please review this single commit and tell us WDYT?

5ce43c4

I don't like it 😉 This is probably only an interface used by people adding new CRIU features to runc like me, but I use it regularly. I am also using it in my (hopefully) upcoming CRI-O checkpoint/restore implementation.

It is strange to have it at the main command level, it could also work as a checkpoint or restore subcommand option.

I am probably the only user of it but I like it. I will manage without that flag, but I would prefer to keep it.

thaJeztah

made another pass at reviewing; looks like it needs a rebase (sorry!)

overall looking good; left some suggestions (happy to discuss)

thaJeztah · 2022-01-26T22:01:17Z

libcontainer/configs/config.go

-type Capabilities struct {
-	// Bounding is the set of capabilities checked by the kernel.
-	Bounding []string
-	// Effective is the set of capabilities checked by the kernel.
-	Effective []string
-	// Inheritable is the capabilities preserved across execve.
-	Inheritable []string
-	// Permitted is the limiting superset for effective capabilities.
-	Permitted []string
-	// Ambient is the ambient set of capabilities that are kept.
-	Ambient []string
-}


I don't think it will cause any issues, but mentioninging it (just in case); the type is the same from a "go" perspective, but the runtime-spec type uses lowercase names (and omitempty) for the JSON representation; https://github.com/opencontainers/runtime-spec/blob/a3c33d663ebc56c4d35dbceaa447c7bf37f6fab3/specs-go/config.go#L65-L78

// LinuxCapabilities specifies the list of allowed capabilities that are kept for a process. // http://man7.org/linux/man-pages/man7/capabilities.7.html type LinuxCapabilities struct { // Bounding is the set of capabilities checked by the kernel. Bounding []string `json:"bounding,omitempty" platform:"linux"` // Effective is the set of capabilities checked by the kernel. Effective []string `json:"effective,omitempty" platform:"linux"` // Inheritable is the capabilities preserved across execve. Inheritable []string `json:"inheritable,omitempty" platform:"linux"` // Permitted is the limiting superset for effective capabilities. Permitted []string `json:"permitted,omitempty" platform:"linux"` // Ambient is the ambient set of capabilities that are kept. Ambient []string `json:"ambient,omitempty" platform:"linux"` }

Well, this might open the hell gates:

[root@kir-rhat runc]# jq < /run/runc/xxx/state.json | grep -A10 capab "capabilities": { "Bounding": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ], "Effective": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ], [root@kir-rhat runc]# jq < /run/runc/xx344/state.json | grep -A10 capab "capabilities": { "bounding": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ], "effective": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ],

From what I can see, this is not used anywhere, and yet it might become problematic.

I guess I'll revert this.

If these files are only used by runc and/or only used by Go code, then it shouldn't (AFAICS) be an issue. json.Unmarshal() is case insensitive for keys (for better or worse 😅)

https://go.dev/play/p/9bo-nSvEARD

package main import ( "encoding/json" "fmt" "reflect" ) type Capabilities struct { // Bounding is the set of capabilities checked by the kernel. Bounding []string // Effective is the set of capabilities checked by the kernel. Effective []string // Inheritable is the capabilities preserved across execve. Inheritable []string // Permitted is the limiting superset for effective capabilities. Permitted []string // Ambient is the ambient set of capabilities that are kept. Ambient []string } type LinuxCapabilities struct { // Bounding is the set of capabilities checked by the kernel. Bounding []string `json:"bounding,omitempty" platform:"linux"` // Effective is the set of capabilities checked by the kernel. Effective []string `json:"effective,omitempty" platform:"linux"` // Inheritable is the capabilities preserved across execve. Inheritable []string `json:"inheritable,omitempty" platform:"linux"` // Permitted is the limiting superset for effective capabilities. Permitted []string `json:"permitted,omitempty" platform:"linux"` // Ambient is the ambient set of capabilities that are kept. Ambient []string `json:"ambient,omitempty" platform:"linux"` } type runc struct { Capabilities *Capabilities `json:"capabilities"` } type oci struct { Capabilities *LinuxCapabilities `json:"capabilities"` } const jsonLower = ` { "capabilities": { "bounding": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ], "effective": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ] } }` const jsonUpper = ` { "capabilities": { "Bounding": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ], "Effective": [ "CAP_AUDIT_WRITE", "CAP_KILL", "CAP_NET_BIND_SERVICE" ] } }` func main() { var ( runcLower runc runcUpper runc ociLower oci ociUpper oci ) if err := json.Unmarshal([]byte(jsonLower), &runcLower); err != nil { fmt.Println(err) } if err := json.Unmarshal([]byte(jsonUpper), &runcUpper); err != nil { fmt.Println(err) } if err := json.Unmarshal([]byte(jsonLower), &ociLower); err != nil { fmt.Println(err) } if err := json.Unmarshal([]byte(jsonUpper), &ociUpper); err != nil { fmt.Println(err) } if reflect.DeepEqual(runcLower, runcUpper) { fmt.Printf("SAME: %v <--> %v\n", runcLower.Capabilities, runcUpper.Capabilities) } if reflect.DeepEqual(ociLower, ociUpper) { fmt.Printf("SAME: %v <--> %v\n", ociLower.Capabilities, runcUpper.Capabilities) } }

thaJeztah · 2022-01-26T22:25:59Z

utils_linux.go

-	if err != nil {
-		newuidmap = ""
+	if newuidmap, err := exec.LookPath("newuidmap"); err == nil {
+		f.NewuidmapPath = newuidmap


Almost tempted to say; perhaps it should be named LibContainer (as it's constructed using libcontainer.New() 🤔

Even further; I'm even starting to wonder if libcontainer.New() should be public at all. All cases except for getContainers() ("list containers"), basically follow the pattern;

factory := libcontainer.New()

factory.Create() / factory.Load()

done

It would almost make sense to just libcontainer.Create(), libcontainer.Get(), libcontainer.List() (which internally can do a new()), adding root as argument.

thaJeztah · 2022-01-26T23:19:32Z

libcontainer/configs/validate/validator.go

 type check func(config *configs.Config) error

-func (v *ConfigValidator) Validate(config *configs.Config) error {
+func Validate(config *configs.Config) error {


Thanks for doing this! I recall looking at this a couple of times (why the "Validator"??)

Perhaps as a follow-up, we should move configs/validate to configs. It's a bit weird to have it in a separate package, as it's validating the config. Moving it into configs, if could just be validate(config) (no need to export), or

func (config *configs.Config) validate() error

Perhaps as a follow-up, we should move configs/validate to configs.

I gave it a try and got circular deps as a result. Will take a look later, but it seems this was the reason why it's in a separate package.

Oh, and we still have to export it because it's also used from libcontainer/specconv/spec_linux_test.go

thaJeztah · 2022-01-26T23:31:13Z

libcontainer/container_linux.go

+				// We resolve the paths for new{u,g}idmap from
+				// the context of runc to avoid doing a path
+				// lookup in the nsexec context.
+				if path, err := exec.LookPath("newuidmap"); err == nil {


Silly question; I haven't fully checked which codepaths are executed, but I see that bootstrapData() is called multiple times; can this lead to exec.LookPath() being called multiple times (which will be iterating over all directories in PATH); is that a concern? https://github.com/golang/go/blob/f229e7031a6efb2f23241b5da000c3b3203081d6/src/os/exec/lp_unix.go#L47-L57

bootstrapData is caller either for runc start/run, or for runc init

thaJeztah · 2022-01-26T23:51:55Z

libcontainer/factory.go

 	if err := os.MkdirAll(root, 0o700); err != nil {
 		return nil, err
 	}
 	return &Factory{
-		Root: root,
+		Root: absRoot,


Wondering why we made Root public; after all, its expected to only be set once (when doing libcontainer.New()), then add a .Root() function.

Once it's private, we should also validate (if needed) root here, so that we can remove redundant checks such as
https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L145-L147

and

https://github.com/opencontainers/runc/blob/master/libcontainer/factory_linux.go#L220-L222

And remove redundant code, such as

runc/list.go

Lines 118 to 122 in 6d35069

root := context.GlobalString("root")

absRoot, err := filepath.Abs(root)

if err != nil {

return nil, err

}

Also, not 100% sure about this utility; perhaps it's still needed though (but it's a bit of an odd one)

runc/utils.go

Lines 99 to 101 in c9b8b4f

// reviseRootDir convert the root to absolute path

func reviseRootDir(context *cli.Context) error {

root := context.GlobalString("root")

Yes, I was going to clean this up.

thaJeztah · 2022-01-27T00:00:13Z

⚠️ somewhere in the middle of it hides the removal of runc --criu option -- please review carefully (or I can certainly move it out to a separate PR).

I think it would make sense to move it separate (for visibility)

kolyshkin · 2022-01-27T03:01:46Z

OK, I am going to split this one into parts (and will update description accordingly).

kolyshkin · 2022-06-16T00:20:03Z

As per description, this was split into smaller PRs, of which all but #3385 were merged, so this can be closed now.

kolyshkin added the kind/refactor refactoring label Dec 9, 2021

kolyshkin force-pushed the less-factories branch from 43300a7 to a3bc8cc Compare December 9, 2021 23:53

kolyshkin changed the title ~~Less factories~~ Less interfaces Dec 9, 2021

kolyshkin force-pushed the less-factories branch 3 times, most recently from 088c13b to 213a682 Compare December 11, 2021 03:07

kolyshkin marked this pull request as ready for review December 13, 2021 21:03

kolyshkin added this to the 1.2.0 milestone Jan 17, 2022

kolyshkin requested review from cyphar, thaJeztah, mrunalp and AkihiroSuda and removed request for cyphar and thaJeztah January 19, 2022 01:33

thaJeztah reviewed Jan 19, 2022

View reviewed changes

This was referenced Jan 20, 2022

Minor cleanups in libcontainer and loadFactory #3340

Closed

remove Pausing state, update docs #3343

Closed

kolyshkin force-pushed the less-factories branch 4 times, most recently from e718e3c to 34afdfa Compare January 21, 2022 23:03

kolyshkin requested a review from thaJeztah January 22, 2022 02:15

kolyshkin force-pushed the less-factories branch from 34afdfa to 429dff9 Compare January 25, 2022 20:26

kolyshkin added 3 commits January 25, 2022 13:07

libct/README: rm Cgroupfs

604917e

This method was removed earlier by commit 097c6d7, but the documentation was not updated. Fix it. Fixes: 097c6d7 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct/configs: rm Windows TODO

8b9fa19

It's clear at this point that runc won't support Windows. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin added 11 commits January 25, 2022 13:09

init.go: move logger setup to StartInitialization

742be71

Note that since this is now also called from libcontainer/integration, which do not set _LIBCONTAINER_LOGLEVEL, make it optional. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct: Container, Factory: rm InitPath, InitArgs

b9b9fe1

Those are *always* /proc/self/exe init, and it does not make sense to change these (in case there ever will be, I guess, it can be addressed separately). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct: New: rm opts arguments

13c6cae

All its users were removed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

utils: remove loadFactory

3d5fd4a

- move filepath.Abs to libcontainer.New; - remove loadFactory. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the less-factories branch from 429dff9 to 3d5fd4a Compare January 25, 2022 21:10

thaJeztah reviewed Jan 26, 2022

View reviewed changes

kolyshkin mentioned this pull request Jan 27, 2022

libcontainer/README.md is out of date #3348

Closed

kolyshkin marked this pull request as draft January 27, 2022 03:01

This was referenced Jan 27, 2022

Misc nits #3352

Merged

runc: remove --criu option #3353

Merged

Factory cleanup #3354

Merged

This was referenced Feb 11, 2022

Remove Factory and LinuxFactory #3373

Merged

libct: rm BaseContainer and Container interfaces #3375

Merged

kolyshkin mentioned this pull request Feb 19, 2022

init simplification #3385

Merged

kolyshkin closed this Jun 16, 2022

lifubang removed this from the 1.2.0 milestone Jul 22, 2023

cyphar mentioned this pull request Mar 14, 2024

release 1.2.0-rc.1 #4221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less interfaces #3316

Less interfaces #3316

kolyshkin commented Dec 9, 2021 •

edited

Loading

kolyshkin commented Dec 13, 2021

thaJeztah Jan 19, 2022

kolyshkin Jan 20, 2022

thaJeztah Jan 19, 2022

kolyshkin Jan 20, 2022

thaJeztah Jan 19, 2022

kolyshkin Jan 20, 2022

kolyshkin Jan 21, 2022

thaJeztah Jan 26, 2022

kolyshkin Jan 28, 2022

kolyshkin commented Jan 25, 2022

kolyshkin commented Jan 25, 2022

kolyshkin commented Jan 25, 2022

adrianreber commented Jan 26, 2022

thaJeztah left a comment

thaJeztah Jan 26, 2022

kolyshkin Jan 27, 2022

thaJeztah Jan 27, 2022

thaJeztah Jan 26, 2022

thaJeztah Jan 26, 2022

kolyshkin Jan 27, 2022

thaJeztah Jan 26, 2022

kolyshkin Jan 27, 2022

thaJeztah Jan 26, 2022

kolyshkin Jan 27, 2022

thaJeztah commented Jan 27, 2022

kolyshkin commented Jan 27, 2022

kolyshkin commented Jun 16, 2022

		// If the Container state is RUNNING or CREATED, sets the Container
		// state to PAUSING and pauses the execution of any user processes.

-type Container struct {
+// Container is a libcontainer container object.
+//
+// Each container is thread-safe within the same process. Since a container can
+// be destroyed by a separate process, any function may return that the container
+// was not found.
+type Container struct {

	root := context.GlobalString("root")
	absRoot, err := filepath.Abs(root)
	if err != nil {
	return nil, err
	}

	// reviseRootDir convert the root to absolute path
	func reviseRootDir(context *cli.Context) error {
	root := context.GlobalString("root")

Less interfaces #3316

Less interfaces #3316

Conversation

kolyshkin commented Dec 9, 2021 • edited Loading

kolyshkin commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Jan 25, 2022

kolyshkin commented Jan 25, 2022

kolyshkin commented Jan 25, 2022

adrianreber commented Jan 26, 2022

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jan 27, 2022

kolyshkin commented Jan 27, 2022

kolyshkin commented Jun 16, 2022

kolyshkin commented Dec 9, 2021 •

edited

Loading