Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

containers.conf should be cached #1200

Closed
vrothberg opened this issue Oct 20, 2022 · 2 comments · Fixed by containers/podman#16238
Closed

containers.conf should be cached #1200

vrothberg opened this issue Oct 20, 2022 · 2 comments · Fixed by containers/podman#16238
Assignees
@vrothberg

Comments

@vrothberg
Copy link
Member

The below strace suggests that containers.conf should be cached. We are already doing that for registries.conf. The idea for registries.conf was to cache directly in the package to prevent users from having to take care of caching. SIGHUB can be used to trigger reloads (as done with cri-o).

podman (main) $ sudo strace -fff ./bin/podman run --rm docker.io/library/alpine:latest true 2>&1 | grep containers.conf
[pid 13793] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf", {st_mode=S_IFREG|0644, st_size=24151, ...}, 0) = 0
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc000147148, 0) = -1 ENOENT (No such file or directory)
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d", 0xc000147218, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
[pid 13793] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC) = 3
[pid 13793] read(3, "# The containers configuration f"..., 512) = 512
[pid 13793] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf", {st_mode=S_IFREG|0644, st_size=24151, ...}, 0) = 0
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc0006ac858, 0) = -1 ENOENT (No such file or directory)
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d", 0xc0006ac928, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
[pid 13793] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC) = 3
[pid 13793] read(3, "# The containers configuration f"..., 512) = 512
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc00047bbd8, 0) = -1 ENOENT (No such file or directory)
[pid 13793] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf", {st_mode=S_IFREG|0644, st_size=24151, ...}, 0) = 0
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc0004ece08, 0) = -1 ENOENT (No such file or directory)
[pid 13793] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d", 0xc0004eced8, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
[pid 13793] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC) = 3
[pid 13793] read(3, "# The containers configuration f"..., 512) = 512
[pid 13912] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf",  <unfinished ...>
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf",  <unfinished ...>
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d",  <unfinished ...>
[pid 13912] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC <unfinished ...>
[pid 13912] <... read resumed>"# The containers configuration f"..., 512) = 512
[pid 13912] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf", {st_mode=S_IFREG|0644, st_size=24151, ...}, 0) = 0
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc00026e2a8, 0) = -1 ENOENT (No such file or directory)
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d", 0xc00026e378, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
[pid 13912] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC) = 3
[pid 13912] read(3, "# The containers configuration f"..., 512) = 512
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc00023f078, 0) = -1 ENOENT (No such file or directory)
[pid 13912] newfstatat(AT_FDCWD, "/usr/share/containers/containers.conf", {st_mode=S_IFREG|0644, st_size=24151, ...}, 0) = 0
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf", 0xc00025c5e8, 0) = -1 ENOENT (No such file or directory)
[pid 13912] newfstatat(AT_FDCWD, "/etc/containers/containers.conf.d", 0xc00025c6b8, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory)
[pid 13912] openat(AT_FDCWD, "/usr/share/containers/containers.conf", O_RDONLY|O_CLOEXEC) = 3
[pid 13912] read(3, "# The containers configuration f"..., 512) = 512
@vrothberg
Copy link
Member Author

@rhatdan @baude FYI

@Luap99
Copy link
Member

Luap99 commented Oct 20, 2022

The default call is already cached:

common/pkg/config/config.go

Line 1165 in c73d138

func Default() (*Config, error) {

This means that the callers do not use it and call NewConfig("") instead.

@vrothberg vrothberg self-assigned this Oct 20, 2022
@vrothberg vrothberg mentioned this issue Oct 20, 2022
Merged
@rhatdan rhatdan mentioned this issue Oct 20, 2022
Closed
rhatdan added a commit to rhatdan/podman that referenced this issue Oct 20, 2022
@rhatdan
Only read containers.conf once.
ca9d371 
We should not be changing these fields on the client side unless the
user specified them.

This patch drops strace mentions of containers.conf from 116 down to 26.

Fixes: containers/common#1200

[NO NEW TESTS NEEDED] Existing tests should test this.

Signed-off-by: Daniel J Walsh <[email protected]>
vrothberg added a commit to vrothberg/libpod that referenced this issue Oct 21, 2022
@vrothberg
use cached containers.conf
4e29ce2 
Use `Default()` instead of re-loading containers.conf.

Also rework how the containers.conf objects are handled for parsing the
CLI.  Previously, we were conflating "loading the defaults" with
"storing values from the CLI" with "libpod may further change fields"
which ultimately led to various bugs and test failues.

To address the issue, separate the defaults from the values from the CLI
and properly name the fields to make the semantics less ambiguous.

[NO NEW TESTS NEEDED] as it's not a functional change.

Fixes: containers/common/issues/1200
Signed-off-by: Valentin Rothberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vrothberg vrothberg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use cached containers.conf vrothberg/libpod
Only read containers.conf once. rhatdan/podman
2 participants
@vrothberg @Luap99