Add missing fields to be used with podman #29

rhatdan · 2019-12-16T15:43:09Z

We also want to be able to default namespaces to host, so that HPC
machines can default to not using most of the namespaces by default.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2019-12-23T16:57:17Z

Fixes: #31

rhatdan · 2019-12-23T16:58:01Z

@vrothberg @TomSweeneyRedHat @QiWang19 @baude @mheon @giuseppe PTAL

mheon · 2019-12-23T17:01:25Z

General comment, but I think we need to define formats for all the []string fields. Stuff like AdditionalVolumes. We need to make sure that it will be consistently parsed by Buildah, Podman, etc. I would almost rather we do structures here (Source, Destination, Options) to remove the ambiguity of how Buildah, Podman, CRI-O could parse them.

vrothberg · 2019-12-23T17:07:20Z

General comment, but I think we need to define formats for all the []string fields. Stuff like AdditionalVolumes. We need to make sure that it will be consistently parsed by Buildah, Podman, etc. I would almost rather we do structures here (Source, Destination, Options) to remove the ambiguity of how Buildah, Podman, CRI-O could parse them.

That's a very valid concern and I think it's smart to prevent any boilerplate code from consumers of this library. If we have fields that must be post-processed by consumers, we should do it for them.

giuseppe · 2019-12-23T18:36:16Z

pkg/config/config.go

+	// DNS Set default DNS servers.
+	DNS string `toml:"default_ulimits"`
+
+	// DNSOptions Set  default DNS options.


extra space after set.

Should the various "Set" used in the comments be lower case?

giuseppe · 2019-12-23T18:36:51Z

pkg/config/config.go

 	// HTTPProxy is the proxy environment variable list to apply to container process
 	HTTPProxy []string `toml:"http_proxy"`

+	// Init tells containre runtimes wither to run init inside the


typo in containre and wither

giuseppe · 2019-12-23T18:37:10Z

pkg/config/config.go

+	// InitPath is the path for init to run if the Init bool is enabled
+	InitPath string `toml:"init_path"`
+
+	// IPCNS way to to create a ipc namespace for the container


double "to"

giuseppe · 2019-12-23T18:37:44Z

pkg/config/config.go

+	// NetNS indicates how to create a network namespace for the container
+	NetNS string `toml:"netns"`
+
+	// NoHost tells container engine wheter to create its own /etc/hosts


typo in "wheter"

giuseppe · 2019-12-23T18:38:18Z

pkg/config/config.go

@@ -179,7 +221,7 @@ type LibpodConfig struct {
 	// containers and pods will be visible. The default namespace is "".
 	Namespace string `toml:"namespace,omitempty"`

-	// NetworkCmdPath is the path to the slirp4netns binary.
+	// NewtorkCmdPath is the path to the slirp4netns binary.


typo in "NewtorkCmdPath"

TomSweeneyRedHat · 2019-12-23T19:55:43Z

pkg/config/containers.conf

+#  2. /etc/containers/containers.conf
+#  3. $HOME/.config/containers/containers.conf (Rootless containers ONLY)
+#  Items specified in the latter containers.conf, if they exist, override the
+# previous containers.conf # settings, or the default settings.


I'm still not clear on the ordering for rootless. If a variable is defined in all three files with different values, what does it get set to in a rootless environment? Does the processing really go through all three files and the value from 1 would be overridden by the 2nd, and then finally by the 3rd file's value? If so, then this is good, if not and we instead read just file 3 in a rootless environment, I think this needs to be adjusted.

Goes through all three, and LAST one with a field get's used.

So if ~/.config/containers/containers.conf has a field in a rootless container, it wins. If it does not, then the precedence goes
/etc/containers/containers.conf
/usr/share/containers/containers.conf
Defaults.

All of these are checked even in the case of rootless.

I look at these 4 configuration settings as being controlled by different entities.
Defaults -> Upstream
/usr/share -> distros
/etc -> administrators
$HOME-> Users
Each able to override the other.

TomSweeneyRedHat

One question, then other than @giuseppe 's comments, LGTM

TomSweeneyRedHat · 2019-12-24T13:46:17Z

@rhatdan glint is barking at you:

$ make validate
golangci-lint run
pkg/config/config.go:717:2: ineffectual assignment to `rootlessStorage` (ineffassign)
	rootlessStorage := strings.Replace(c.Libpod.RootlessStoragePath, "$HOME", home, -1)
	^
pkg/config/config.go:719:2: ineffectual assignment to `rootlessStorage` (ineffassign)
	rootlessStorage = strings.Replace(c.Libpod.RootlessStoragePath, "$UID", string(uid), -1)

	^
Makefile:37: recipe for target 'validate' failed
make: *** [validate] Error 1

The command "make validate" exited with 2.

We also want to be able to default namespaces to host, so that HPC machines can default to not using most of the namespaces by default. Signed-off-by: Daniel J Walsh <[email protected]>

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2019-12-29T10:57:55Z

I have fixed comments and tests pass, merging.

Add network information to setupopts

rhatdan force-pushed the master branch 2 times, most recently from 56d319c to 05fe824 Compare December 16, 2019 15:55

rhatdan mentioned this pull request Dec 16, 2019

configuration option for dns server to use containers/podman#4508

Closed

giuseppe reviewed Dec 23, 2019

View reviewed changes

TomSweeneyRedHat reviewed Dec 23, 2019

View reviewed changes

rhatdan added 2 commits December 28, 2019 07:19

Add missing fields to be used with podman

eb9e7ab

We also want to be able to default namespaces to host, so that HPC machines can default to not using most of the namespaces by default. Signed-off-by: Daniel J Walsh <[email protected]>

Add new fields to containers.conf

bd0a08c

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the master branch from 5fcc64d to bd0a08c Compare December 28, 2019 12:22

rhatdan merged commit 695eb89 into containers:master Dec 29, 2019

M1cha pushed a commit to M1cha/common that referenced this pull request Dec 20, 2022

Merge pull request containers#29 from baude/setupopts

254cbcb

Add network information to setupopts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing fields to be used with podman #29

Add missing fields to be used with podman #29

rhatdan commented Dec 16, 2019

rhatdan commented Dec 23, 2019

rhatdan commented Dec 23, 2019

mheon commented Dec 23, 2019

vrothberg commented Dec 23, 2019 •

edited

Loading

giuseppe Dec 23, 2019

rhatdan Dec 24, 2019

giuseppe Dec 23, 2019

rhatdan Dec 24, 2019

giuseppe Dec 23, 2019

rhatdan Dec 24, 2019

giuseppe Dec 23, 2019

rhatdan Dec 24, 2019

giuseppe Dec 23, 2019

rhatdan Dec 24, 2019

TomSweeneyRedHat Dec 23, 2019

rhatdan Dec 24, 2019

TomSweeneyRedHat left a comment

TomSweeneyRedHat commented Dec 24, 2019

rhatdan commented Dec 29, 2019

Add missing fields to be used with podman #29

Add missing fields to be used with podman #29

Conversation

rhatdan commented Dec 16, 2019

rhatdan commented Dec 23, 2019

rhatdan commented Dec 23, 2019

mheon commented Dec 23, 2019

vrothberg commented Dec 23, 2019 • edited Loading

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

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Dec 24, 2019

rhatdan commented Dec 29, 2019

vrothberg commented Dec 23, 2019 •

edited

Loading