containers.conf: turn all `[]string` into `attributedstring.Slice` #1710

vrothberg · 2023-10-25T12:44:08Z

Please see the individual commits. No docs changes yet as I want to get this wired into Buildah and Podman first.

vrothberg · 2023-10-25T12:46:14Z

@rhatdan PTAL

vrothberg · 2023-10-25T14:02:40Z

Buildah PR: containers/buildah#5103

vrothberg · 2023-10-25T14:04:36Z

Podman PR: containers/podman#20483

Luap99

Just some small comments

pkg/config/config.go

Luap99 · 2023-10-25T14:37:55Z

pkg/config/config.go

@@ -1143,7 +1143,7 @@ func (c *Config) FindHelperBinary(name string, searchPATH bool) (string, error)
 		return exec.LookPath(name)
 	}
 	configHint := "To resolve this error, set the helper_binaries_dir key in the `[engine]` section of containers.conf to the directory containing your helper binaries."
-	if len(c.Engine.HelperBinariesDir) == 0 {
+	if len(c.Engine.HelperBinariesDir.Values) == 0 {


Is there a reason this uses Values vs Get()?

Get() will never return nil, so it adds some cost.

Why did you added this special handling to Get()? To me this seems to add unnecessary complexity. Returning nil shouldn't hurt or do podman/buildah tests fail becauase of it?

buildah tests failed. I can follow up on that if you want to.

Although it may be safer to let Get() return a real copy in the future.

I am fine with it but as you said it does extra allocations which means worse performance and memory footprint. It won't be noticeable (I assume) so it is not worth to block/argue about it but many small things start adding up at some point. Copying would definitely make it more expensive with the advantage that the API will be much safer with it.
The main issue is that technically callers can bypass that and just use Values directly so it is impossible to provide a safe API. Ideally go would allow us to keep the fields private but that won't work with the toml Unmarshaller obviously.

I had the very same line of thoughts. To really avoid callers abusing containers.conf, the loading must be made private and then we can add a carefully curated API (as we do in registries.conf).

pkg/config/config_local.go

pkg/config/default.go

Needed in Buildah (and potentially Podman later on) where some options must be overridden. Ultimately this should be avoided whenever possible but this is not my goal at the present. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Convenience function for callers. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

The tests are messy and should be turned into table-driven tests but I do not have time at the moment. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Also tag it as omitempty to fix the test. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

They broke after the attributedstring.Slice migrations. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

To improve the code and reduce memory allocations. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

vrothberg · 2023-10-26T08:06:41Z

Updated, see last commit. Used NewSlice, Get and Set where possible.

Luap99

LGTM

openshift-ci · 2023-10-26T12:39:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg · 2023-10-26T12:40:18Z

@rhatdan PTAL

rhatdan · 2023-10-26T13:02:15Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress approved labels Oct 25, 2023

Luap99 reviewed Oct 25, 2023

View reviewed changes

vrothberg added 24 commits October 26, 2023 09:17

attributedstring.Slice: add Set() method

137c400

Needed in Buildah (and potentially Podman later on) where some options must be overridden. Ultimately this should be avoided whenever possible but this is not my goal at the present. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

attributedstring: add NewSlice func

c3c1aec

Convenience function for callers. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn devices into attributedstring.Slice

a71ef23

The tests are messy and should be turned into table-driven tests but I do not have time at the moment. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn Annotations into attributedstring.Slice

b7a5a13

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn CgroupConf into attributedstring.Slice

7a6915b

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn Capabilities into attributedstring.Slice

3a0009f

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn DefaultSysctls into attributedstring.Slice

fc58fb6

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn Ulimites into attributedstring.Slice

0c3b731

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn DNSServers into attributedstring.Slice

56d438f

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn DNSOptions into attributedstring.Slice

53cc626

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn DNSSearches into attributedstring.Slice

7b8924f

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn ConmonEnvVars into attributedstring.Slice

2fbb01d

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn ConmonPath into attributedstring.Slice

7506733

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn ConmonRsPath into attributedstring.Slice

915d4e1

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn ComposeProviders into attributedstring.Slice

dc9dc83

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn engine Env into attributedstring.Slice

d4cdf16

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn HelperBinariesDir into attributedstring.Slice

edac755

Also tag it as omitempty to fix the test. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn DefaultHooksDir into attributedstring.Slice

a9b4b08

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn NetworkCmdOptions into attributedstring.Slice

b354c1d

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn AddCompression into attributedstring.Slice

f22b5d8

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn RuntimePath into attributedstring.Slice

f51fdbe

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn RuntimeSupportsJSON into attributedstring.Slice

1ca85fe

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn RuntimeSupportsNoCgroups into attributedstring.Slice

cc5a288

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn RuntimeSupportsKVM into attributedstring.Slice

e34516e

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

vrothberg added 6 commits October 26, 2023 09:17

pkg/config: turn CNIPluginDirs into attributedstring.Slice

fd0b58b

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn NetavarkPluginDirs into attributedstring.Slice

ee5721d

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn PastaOptions into attributedstring.Slice

4aaa7e9

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: turn machine Volumes into attributedstring.Slice

6fed91f

Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: fix remote tests

f335c36

They broke after the attributedstring.Slice migrations. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

pkg/config: use NewSlice, Get and Set where possible

82e88cc

To improve the code and reduce memory allocations. Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

vrothberg force-pushed the RUN-1934 branch from 3b797db to 82e88cc Compare October 26, 2023 08:06

vrothberg marked this pull request as ready for review October 26, 2023 08:33

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 26, 2023

Luap99 approved these changes Oct 26, 2023

View reviewed changes

openshift-ci bot assigned rhatdan Oct 26, 2023

openshift-ci bot added the lgtm label Oct 26, 2023

openshift-ci bot merged commit 78e0a90 into containers:main Oct 26, 2023
7 checks passed

vrothberg deleted the RUN-1934 branch October 26, 2023 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

containers.conf: turn all `[]string` into `attributedstring.Slice` #1710

containers.conf: turn all `[]string` into `attributedstring.Slice` #1710

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

Luap99 left a comment

Luap99 Oct 25, 2023

vrothberg Oct 26, 2023

Luap99 Oct 26, 2023

vrothberg Oct 26, 2023

vrothberg Oct 26, 2023

Luap99 Oct 26, 2023

vrothberg Oct 26, 2023

vrothberg commented Oct 26, 2023

Luap99 left a comment

openshift-ci bot commented Oct 26, 2023

vrothberg commented Oct 26, 2023

rhatdan commented Oct 26, 2023

containers.conf: turn all []string into attributedstring.Slice #1710

containers.conf: turn all []string into attributedstring.Slice #1710

Conversation

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

vrothberg commented Oct 25, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Oct 25, 2023

Choose a reason for hiding this comment

vrothberg Oct 26, 2023

Choose a reason for hiding this comment

Luap99 Oct 26, 2023

Choose a reason for hiding this comment

vrothberg Oct 26, 2023

Choose a reason for hiding this comment

vrothberg Oct 26, 2023

Choose a reason for hiding this comment

Luap99 Oct 26, 2023

Choose a reason for hiding this comment

vrothberg Oct 26, 2023

Choose a reason for hiding this comment

vrothberg commented Oct 26, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 26, 2023

vrothberg commented Oct 26, 2023

rhatdan commented Oct 26, 2023

containers.conf: turn all `[]string` into `attributedstring.Slice` #1710

containers.conf: turn all `[]string` into `attributedstring.Slice` #1710