libct: speedup process.Env handling

[kolyshkin]

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 why it was implemented, I found commit 9744d72 and traced
it to 2, which discusses the actual reasons. At the time were:

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

Yet it was decided to not go ahead with this patch, but later 3 was
merged with the carry of this patch.

Now, from what I see:

1. Passing environment to exec is way faster than using os.Setenv and
   os.Environment() (benchmarks show ~20x improvement in simple Go test,
   and ~3x improvement in real-world test, see below).
2. Setting environment variables in the runc context can result is ugly
   side effects (think GODEBUG, LD_PRELOAD, _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). In C
   (Linux/glibc), the first value is used. In Go, it's the last one.
   We should probably stick to what we have in order to maintain
   backward compatibility.

This patch:

• switches to passing env directly to exec;
• adds deduplication mechanism to retain backward compatibility;
• sets PATH from process.Env in the current process (same as before);
• adds HOME to process.Env if not set (same as before);
• removes os.Clearenv call as it's no longer needed.

The benchmark added shows 3x improvement
for in-container exec with 5000 environment variables:

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

The remaining questions are:

• are there any potential regressions (for example, from not setting
  values from process.Env to the current process);
• should deduplication show warnings (maybe promoted to errors later);
• whether a default for PATH (e.g "/bin:/usr/bin" should be added,
  when PATH is not set.

[lifubang]

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

[kolyshkin]

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)

[thaJeztah]

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]

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:

1. 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.
2. 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.
3. 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]

PTAL @opencontainers/runc-maintainers

[lifubang]

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

[rata]

LGTM, thanks!

[kolyshkin]

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

[rata]

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]

This is totally 1.3 material; let me rebase

[cyphar]

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