libct: speedup process.Env handling #4325

kolyshkin · 2024-06-24T19:25:49Z

This is a rework/carry of #1983.

The current implementation sets all the environment variables passed in
Process.Env in the current process, one by one, then uses os.Environ to
read those back.

As pointed out in [1], this is slow, as runc calls os.Setenv for every
variable, and there may be a few thousands of those. Looking into how
os.Setenv is implemented, it is indeed slow, especially when cgo is
enabled.

Looking into why it was implemented the way it is, I found commit
9744d72c and traced it to [2], which discusses the actual reasons.
It boils down to these two:

 - HOME is not passed into container as it is set in setupUser by
   os.Setenv and has no effect on config.Env;
 - there is a need to deduplication of environment variables.

Yet it was decided in [2] to not go ahead with this patch, but
later [3] was opened with the carry of this patch, and merged.

Now, from what I see:

1. Passing environment to exec is way faster than using os.Setenv and
   os.Environ (tests show ~20x speed improvement in a simple Go test,
   and ~3x improvement in real-world test, see below).
2. Setting environment variables in the runc context may result is some
   ugly side effects (think GODEBUG, LD_PRELOAD, or _LIBCONTAINER_*).
3. Nothing in runtime spec says that the environment needs to be
   deduplicated, or the order of preference (whether the first or the
   last value of a variable with the same name is to be used). We should
   stick to what we have in order to maintain backward compatibility.

So, this patch:
 - switches to passing env directly to exec;
 - adds deduplication mechanism to retain backward compatibility;
 - takes care to set PATH from process.Env in the current process
   (so that supplied PATH is used to find the binary to execute),
   also to retain backward compatibility;
 - adds HOME to process.Env if not set.

The benchmark added by the previous commit shows ~3x improvement:

                        │   before    │                after                 │
                        │   sec/op    │    sec/op     vs base                │
        ExecInBigEnv-20   61.53m ± 1%   21.87m ± 16%  -64.46% (p=0.000 n=10)

Remaining questions (and my answers to those):

Q: Are there any potential regressions (for example, from not setting values from process.Env to the current process?
A: Pprobably not; if yes, someone is exploiting some undocumented behavior.

Q: Should deduplication show warnings (maybe promoted to errors later)?
A: For best backward compatibility, let's not do that. Can always be added later (maybe with some addition to runtime-spec).

Q: Whether a default for PATH (e.g "/bin:/usr/bin" should be added, when PATH is not set.
A: This needs to be done in runtime-spec first (document the default for PATH, then add it to runtimes).

libcontainer/integration/execin_test.go

libcontainer/env.go

libcontainer/init_linux.go

lifubang · 2024-06-27T08:30:02Z

libcontainer/standard_init_linux.go

@@ -277,7 +278,7 @@ func (l *linuxStandardInit) Init() error {

 	if l.dmzExe != nil {
 		l.config.Args[0] = name
-		return system.Fexecve(l.dmzExe.Fd(), l.config.Args, os.Environ())
+		return system.Fexecve(l.dmzExe.Fd(), l.config.Args, l.config.Env)


Before we merge #4323 , maybe we should also need to include l.config.Env when running StartContainer hook.

This is definitely something to think about. Maybe it makes sense to do it selectively (for those hooks that are run inside the container -- AFAIR not all of them are)

Better late than never -- thank you @lifubang, I finally got it now.

This is now fixed and a test case is added.

I still prefer something like #4323 as it's much more straightforward.

thaJeztah · 2024-06-29T17:47:19Z

Nothing in runtime spec says that the environment needs to be deduplicated, or the order of preference (whether the first or the last value of a variable with the same name is to be used). In C (Linux/glibc), the first value is used. In Go, it's the last one.

Interesting; I didn't know about that discrepancy; sounds like something that wouldn't hurt to define in the OCI spec; given that all original implementations were in Go, and ISTR Docker also had its own code to remove duplicates, I'm inclined to describe that as the expected behavior (possibly recommend producers of the OCI config to handle duplicates themselves to prevent any ambiguity).

kolyshkin · 2024-07-01T23:28:00Z

Nothing in runtime spec says that the environment needs to be deduplicated, or the order of preference (whether the first or the last value of a variable with the same name is to be used). In C (Linux/glibc), the first value is used. In Go, it's the last one.

I was not entirely correct here. Let me rephrase this:

When the environment is set using glibc's setenv(3) or putenv(3), or Go's os.Putenv, the value from the latter call for a given key takes precedence. This is obvious.
If the existing environment is not deduplicated (such as when you supply it directly to execve), both glibc's getenv(3) and Go's os.Getenv return the value of the first element.
When using a higher level primitives (from os/exec) in Go, the environment passed is deduplicated in a way that the last value of a key is used. In glibc, there is no such deduplication (as far as I can see), and thus the first value of the key will be used. This is the difference between Go and C.

On an unrelated note, I also took a look at crun and it seems it is following runc logic, calling clearenv(3) followed by putenv(3) for every item in Process.Env, when using environ for execve -- which result in the latter value of the same env var being used. There is probably no room for a similar optimization as putenv in glibc is not thread safe and there are no mutexes to be held etc.

kolyshkin · 2024-09-03T17:57:44Z

PTAL @opencontainers/runc-maintainers

lifubang · 2024-09-03T23:00:00Z

PTAL @opencontainers/runc-maintainers

LGTM, it’s a big step.
But as I mentioned in #4325 (comment), I suggest to add these envs to hook’s process execution before we have a conclusion for #4323.

rata

@kolyshkin left some comments, but seems great it makes a difference! This is marked for 1.3, though, but I guess you asking for reviews now means you want this for 1.2?

I'm fine with this in 1.2 or 1.3

libcontainer/env.go

rata

LGTM, thanks!

kolyshkin · 2024-10-21T20:50:53Z

OTOH I think this is ready for more reviews and potential inclusion.

rata · 2024-10-22T08:25:46Z

The PR to release 1.2 is out already, and it will be released today. I'd prefer to merge this after the 1.2.0 release, if the speedup is nice, we can add it in a patch release (or maybe to a 1.3 release in 3 months ;))

kolyshkin · 2024-10-31T23:06:53Z

This is totally 1.3 material; let me rebase

cyphar · 2024-12-04T08:32:45Z

FWIW, I sent a patch to improve the performance of os.Clearenv (with the patch it's ~30% faster for an environment with 1000 variables set), though this is fixing other issues in addition to the os.Clearenv one. golang/go#70672

kolyshkin · 2024-12-05T00:03:48Z

I've updated the patch (with somewhat better commit message and prepareEnv documentation. I think we can go ahead and merge this for v1.3, and leave the question raised in #4325 (comment) for later.

lifubang · 2024-12-05T09:12:25Z

and leave the question raised in #4325 (comment) for later

But it's a break change for StartContainer hook, though this hook may be not wide spread used in practice.
Maybe we should add this break to CHAGELOG because we may forget it when making a release.

cyphar · 2024-12-07T09:14:23Z

Maybe we should add this break to CHANGELOG because we may forget it when making a release.

I think we need to start adding things to the changelog in the implementation PR in general because we have missed breaking changes before (the most recent example I can think of is #3468, but I've definitely made the same mistake before as well -- it's very easy to skim over a patch when reading through the log to write the release notes).

Here's what it shows on my laptop (with -count 10 -benchtime 10s, summarized by benchstat): │ sec/op │ ExecTrue-20 8.477m ± 2% ExecInBigEnv-20 61.53m ± 1% Signed-off-by: Kir Kolyshkin <[email protected]>

This is a slight refactor of TestExecInEnvironment, making it more strict wrt checking the exec output. 1. Explain why DEBUG is added twice to the env. 2. Reuse the execEnv for the check. 3. Make the check more strict -- instead of looking for substrings, check line by line. 4. Add a check for extra environment variables. Signed-off-by: Kir Kolyshkin <[email protected]>

This is to ensure that changes in Process.Env handling won't affect StartContainer hook. Reported-by: lfbzhm <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>

1. Make CommandHook.Command a pointer, which reduces the amount of data being copied when using hooks, and allows to modify command hooks. 2. Add SetDefaultEnv, which is to be used by the next commit. Signed-off-by: Kir Kolyshkin <[email protected]>

@lifubang

The current implementation sets all the environment variables passed in Process.Env in the current process, one by one, then uses os.Environ to read those back. As pointed out in [1], this is slow, as runc calls os.Setenv for every variable, and there may be a few thousands of those. Looking into how os.Setenv is implemented, it is indeed slow, especially when cgo is enabled. Looking into why it was implemented the way it is, I found commit 9744d72 and traced it to [2], which discusses the actual reasons. It boils down to these two: - HOME is not passed into container as it is set in setupUser by os.Setenv and has no effect on config.Env; - there is a need to deduplication of environment variables. Yet it was decided in [2] to not go ahead with this patch, but later [3] was opened with the carry of this patch, and merged. Now, from what I see: 1. Passing environment to exec is way faster than using os.Setenv and os.Environ (tests show ~20x speed improvement in a simple Go test, and ~3x improvement in real-world test, see below). 2. Setting environment variables in the runc context may result is some ugly side effects (think GODEBUG, LD_PRELOAD, or _LIBCONTAINER_*). 3. Nothing in runtime spec says that the environment needs to be deduplicated, or the order of preference (whether the first or the last value of a variable with the same name is to be used). We should stick to what we have in order to maintain backward compatibility. So, this patch: - switches to passing env directly to exec; - adds deduplication mechanism to retain backward compatibility; - takes care to set PATH from process.Env in the current process (so that supplied PATH is used to find the binary to execute), also to retain backward compatibility; - adds HOME to process.Env if not set; - ensures any StartContainer CommandHook entries with no environment set explicitly are run with the same environment as before. Thanks to @lifubang who noticed that peculiarity. The benchmark added by the previous commit shows ~3x improvement: │ before │ after │ │ sec/op │ sec/op vs base │ ExecInBigEnv-20 61.53m ± 1% 21.87m ± 16% -64.46% (p=0.000 n=10) [1]: opencontainers#1983 [2]: docker-archive/libcontainer#418 [3]: docker-archive/libcontainer#432 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2024-12-23T02:27:27Z

Maybe we should add this break to CHANGELOG because we may forget it when making a release.

I think we need to start adding things to the changelog in the implementation PR in general because we have missed breaking changes before (the most recent example I can think of is #3468, but I've definitely made the same mistake before as well -- it's very easy to skim over a patch when reading through the log to write the release notes).

@cyphar the above incompatibility issue reported by @lifubang is now fixed, but in the process I had to add a small change to libcontainer API, which is documented in the CHANGELOG now.

Doesn't mean this PR won't break something else, but at least I am not aware of anything.

kolyshkin · 2024-12-23T02:30:02Z

libcontainer/standard_init_linux.go

+
+	// In case we have any StartContainer hooks to run, and they don't
+	// have environment configured explicitly, make sure they will be run
+	// with the same environment as container's init.
+	if h := l.config.Config.Hooks[configs.StartContainer]; len(h) > 0 {
+		h.SetDefaultEnv(l.config.Env)
+	}
+


@lifubang FYI this is the fix to the startContainer hook issue wrt environment you've reported back in June.

kolyshkin · 2024-12-23T02:41:21Z

libcontainer/standard_init_linux.go

+
+	// In case we have any StartContainer hooks to run, and they don't
+	// have environment configured explicitly, make sure they will be run
+	// with the same environment as container's init.


Maybe add a note that this behavior is not part of runtime-spec, but a de-facto historical thing we afraid to change.

kolyshkin requested review from lifubang, cyphar and AkihiroSuda June 24, 2024 19:29

kolyshkin mentioned this pull request Jun 24, 2024

Avoid repeated os.SetEnv calls in container init. #1983

Closed

lifubang reviewed Jun 26, 2024

View reviewed changes

libcontainer/integration/execin_test.go Outdated Show resolved Hide resolved

lifubang reviewed Jun 26, 2024

View reviewed changes

libcontainer/env.go Show resolved Hide resolved

lifubang reviewed Jun 26, 2024

View reviewed changes

libcontainer/init_linux.go Show resolved Hide resolved

lifubang reviewed Jun 27, 2024

View reviewed changes

kolyshkin force-pushed the opt-env branch 2 times, most recently from 9b67a06 to b0512bc Compare June 27, 2024 17:09

kolyshkin force-pushed the opt-env branch from b0512bc to 3739bf2 Compare July 3, 2024 05:40

kolyshkin added this to the 1.3.0 milestone Jul 10, 2024

kolyshkin force-pushed the opt-env branch from 3739bf2 to 35ae918 Compare July 12, 2024 20:12

rata reviewed Sep 9, 2024

View reviewed changes

libcontainer/env.go Show resolved Hide resolved

libcontainer/env.go Outdated Show resolved Hide resolved

libcontainer/env.go Show resolved Hide resolved

libcontainer/env.go Show resolved Hide resolved

kolyshkin force-pushed the opt-env branch from 35ae918 to 1534a34 Compare September 10, 2024 01:24

rata approved these changes Sep 10, 2024

View reviewed changes

kolyshkin force-pushed the opt-env branch from 1534a34 to 6e1ae36 Compare October 21, 2024 20:47

kolyshkin force-pushed the opt-env branch from 6e1ae36 to 3b7b613 Compare November 1, 2024 00:44

kolyshkin force-pushed the opt-env branch from 3b7b613 to 8595c5e Compare December 4, 2024 03:34

kolyshkin force-pushed the opt-env branch from 8595c5e to 1058a6f Compare December 4, 2024 23:57

kolyshkin requested a review from lifubang December 5, 2024 00:03

kolyshkin mentioned this pull request Dec 20, 2024

ci: fix a race in TestExecIn and TestExecInTTY #4445

Open

kolyshkin added 2 commits December 22, 2024 17:45

libct/int: add BenchmarkExecInBigEnv

8dfbf10

Here's what it shows on my laptop (with -count 10 -benchtime 10s, summarized by benchstat): │ sec/op │ ExecTrue-20 8.477m ± 2% ExecInBigEnv-20 61.53m ± 1% Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the opt-env branch 2 times, most recently from ae9f5e6 to 03faf10 Compare December 23, 2024 01:59

kolyshkin added 3 commits December 22, 2024 18:27

tests: add test to check StartContainer hook env

89868cb

This is to ensure that changes in Process.Env handling won't affect StartContainer hook. Reported-by: lfbzhm <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the opt-env branch from 03faf10 to 545bd05 Compare December 23, 2024 02:27

kolyshkin commented Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct: speedup process.Env handling #4325

libct: speedup process.Env handling #4325

kolyshkin commented Jun 24, 2024 •

edited

Loading

lifubang Jun 27, 2024 •

edited

Loading

kolyshkin Jun 27, 2024

kolyshkin Dec 23, 2024

thaJeztah commented Jun 29, 2024

kolyshkin commented Jul 1, 2024

kolyshkin commented Sep 3, 2024

lifubang commented Sep 3, 2024

rata left a comment •

edited

Loading

rata left a comment

kolyshkin commented Oct 21, 2024

rata commented Oct 22, 2024

kolyshkin commented Oct 31, 2024

cyphar commented Dec 4, 2024

kolyshkin commented Dec 5, 2024

lifubang commented Dec 5, 2024

cyphar commented Dec 7, 2024 •

edited

Loading

kolyshkin commented Dec 23, 2024

kolyshkin Dec 23, 2024

kolyshkin Dec 23, 2024

libct: speedup process.Env handling #4325

Are you sure you want to change the base?

libct: speedup process.Env handling #4325

Conversation

kolyshkin commented Jun 24, 2024 • edited Loading

lifubang Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

kolyshkin Jun 27, 2024

Choose a reason for hiding this comment

kolyshkin Dec 23, 2024

Choose a reason for hiding this comment

thaJeztah commented Jun 29, 2024

kolyshkin commented Jul 1, 2024

kolyshkin commented Sep 3, 2024

lifubang commented Sep 3, 2024

rata left a comment • edited Loading

Choose a reason for hiding this comment

rata left a comment

Choose a reason for hiding this comment

kolyshkin commented Oct 21, 2024

rata commented Oct 22, 2024

kolyshkin commented Oct 31, 2024

cyphar commented Dec 4, 2024

kolyshkin commented Dec 5, 2024

lifubang commented Dec 5, 2024

cyphar commented Dec 7, 2024 • edited Loading

kolyshkin commented Dec 23, 2024

kolyshkin Dec 23, 2024

Choose a reason for hiding this comment

kolyshkin Dec 23, 2024

Choose a reason for hiding this comment

kolyshkin commented Jun 24, 2024 •

edited

Loading

lifubang Jun 27, 2024 •

edited

Loading

rata left a comment •

edited

Loading

cyphar commented Dec 7, 2024 •

edited

Loading