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.

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

Add container environment variables correctly to the health check #33249

Merged
merged 1 commit into from May 31, 2017

Conversation

Projects
None yet
4 participants
@boaz1337
Contributor

boaz1337 commented May 17, 2017

fixes/closes #33021

- What I did

Add environment variables and set the environment variables for the health check.

- How I did it

Use ReplaceOrAppendEnvValues, CreateDaemonEnvironment and setupLinkedContainers to collect all of them and to set them correctly.

- How to verify it

Run the following test:

$ TESTFLAGS='-check.f TestUnsetEnvVarHealthCheck' hack/make.sh test-integration-cli
Show outdated Hide outdated opts/env_test.go Outdated
@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 May 17, 2017

Contributor

OK, now I realized that if the variable isn't set, it should be discarded (DockerSuite.TestRunEnvironmentErase).

I am going to change this now.

Contributor

boaz1337 commented May 17, 2017

OK, now I realized that if the variable isn't set, it should be discarded (DockerSuite.TestRunEnvironmentErase).

I am going to change this now.

@mlaventure

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2017

Member

Failure in docker-py looks related;

19:09:27 =================================== FAILURES ===================================
19:09:27 ______ CreateContainerTest.test_create_with_environment_variable_no_value ______
19:09:27 /docker-py/tests/integration/api_container_test.py:370: in test_create_with_environment_variable_no_value
19:09:27     environment={'Foo': None, 'Other': 'one', 'Blank': ''},
19:09:27 /docker-py/docker/api/container.py:446: in create_container
19:09:27     return self.create_container_from_config(config, name)
19:09:27 /docker-py/docker/api/container.py:457: in create_container_from_config
19:09:27     return self._result(res, True)
19:09:27 /docker-py/docker/api/client.py:220: in _result
19:09:27     self._raise_for_status(response)
19:09:27 /docker-py/docker/api/client.py:216: in _raise_for_status
19:09:27     raise create_api_error_from_http_exception(e)
19:09:27 /docker-py/docker/errors.py:30: in create_api_error_from_http_exception
19:09:27     raise cls(e, response=response, explanation=explanation)
19:09:27 E   APIError: 500 Server Error: Internal Server Error ("environment variable does not exists")
19:09:27 ======== 1 failed, 205 passed, 31 skipped, 2 xfailed in 244.48 seconds =========
19:09:27 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-docker-py)
19:09:27 +++++ cat bundles/17.06.0-dev/test-docker-py/docker.pid
19:09:27 ++++ kill 31590
19:09:28 ++++ /etc/init.d/apparmor stop
Member

thaJeztah commented May 17, 2017

Failure in docker-py looks related;

19:09:27 =================================== FAILURES ===================================
19:09:27 ______ CreateContainerTest.test_create_with_environment_variable_no_value ______
19:09:27 /docker-py/tests/integration/api_container_test.py:370: in test_create_with_environment_variable_no_value
19:09:27     environment={'Foo': None, 'Other': 'one', 'Blank': ''},
19:09:27 /docker-py/docker/api/container.py:446: in create_container
19:09:27     return self.create_container_from_config(config, name)
19:09:27 /docker-py/docker/api/container.py:457: in create_container_from_config
19:09:27     return self._result(res, True)
19:09:27 /docker-py/docker/api/client.py:220: in _result
19:09:27     self._raise_for_status(response)
19:09:27 /docker-py/docker/api/client.py:216: in _raise_for_status
19:09:27     raise create_api_error_from_http_exception(e)
19:09:27 /docker-py/docker/errors.py:30: in create_api_error_from_http_exception
19:09:27     raise cls(e, response=response, explanation=explanation)
19:09:27 E   APIError: 500 Server Error: Internal Server Error ("environment variable does not exists")
19:09:27 ======== 1 failed, 205 passed, 31 skipped, 2 xfailed in 244.48 seconds =========
19:09:27 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-docker-py)
19:09:27 +++++ cat bundles/17.06.0-dev/test-docker-py/docker.pid
19:09:27 ++++ kill 31590
19:09:28 ++++ /etc/init.d/apparmor stop
@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 May 18, 2017

Contributor

@thaJeztah yes it is. I am working on fixing it 😺

Contributor

boaz1337 commented May 18, 2017

@thaJeztah yes it is. I am working on fixing it 😺

@boaz1337 boaz1337 changed the title from Validate env vars during creation and set env vars if not exist to Clean unset variable in the health check process May 18, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 19, 2017

Member

Did some testing with this. First ran a regular container, no healthcheck, and variations of ENV vars, to check the behavior;

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
EOF


$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=211bf084aab2
IAMEMPTY=
FROMENV=hello
HOME=/root

So; -e SOMEVAR will be ;

  • empty if followed by =
  • get its value from the current environment (if exists)
  • unset if it existed in the image, but does not exist in the environment

Basically, in .Config.Env, "SOMEVAR" means: unset the env-var

With this patch (adding a healthcheck):

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
HEALTHCHECK --interval=1s --timeout=5s --retries=5 CMD env; exit 0;
EOF

$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=2c0f5e0add62
IAMEMPTY=
FROMENV=hello
HOME=/root

All of the above are the same (as expected).

Checking the healthcheck;

$ docker inspect --format '{{json .State.Health.Log }}' foobar | jq .
[
  {
    "Start": "2017-05-19T11:27:41.45071088Z",
    "End": "2017-05-19T11:27:41.505086124Z",
    "ExitCode": 0,
    "Output": "SHLVL=1\nHOME=/root\nPATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\nFROMENV=hello\nIAMEMPTY=\nPWD=/\n"
  }
]

(expanding, and sorting a bit)

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
IAMEMPTY=
FROMENV=hello
HOME=/root
SHLVL=1
PWD=/

Interesting; HOSTNAME is present in docker exec, but not present in the healthcheck's environment

Member

thaJeztah commented May 19, 2017

Did some testing with this. First ran a regular container, no healthcheck, and variations of ENV vars, to check the behavior;

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
EOF


$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=211bf084aab2
IAMEMPTY=
FROMENV=hello
HOME=/root

So; -e SOMEVAR will be ;

  • empty if followed by =
  • get its value from the current environment (if exists)
  • unset if it existed in the image, but does not exist in the environment

Basically, in .Config.Env, "SOMEVAR" means: unset the env-var

With this patch (adding a healthcheck):

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
HEALTHCHECK --interval=1s --timeout=5s --retries=5 CMD env; exit 0;
EOF

$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=2c0f5e0add62
IAMEMPTY=
FROMENV=hello
HOME=/root

All of the above are the same (as expected).

Checking the healthcheck;

$ docker inspect --format '{{json .State.Health.Log }}' foobar | jq .
[
  {
    "Start": "2017-05-19T11:27:41.45071088Z",
    "End": "2017-05-19T11:27:41.505086124Z",
    "ExitCode": 0,
    "Output": "SHLVL=1\nHOME=/root\nPATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\nFROMENV=hello\nIAMEMPTY=\nPWD=/\n"
  }
]

(expanding, and sorting a bit)

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
IAMEMPTY=
FROMENV=hello
HOME=/root
SHLVL=1
PWD=/

Interesting; HOSTNAME is present in docker exec, but not present in the healthcheck's environment

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 19, 2017

Member

So, as far as I can see, this works as expected.

I wonder if we (separate from this PR) should look into why HOSTNAME is not present in the health check's environment.

Member

thaJeztah commented May 19, 2017

So, as far as I can see, this works as expected.

I wonder if we (separate from this PR) should look into why HOSTNAME is not present in the health check's environment.

@thaJeztah

LGTM, but @mlaventure PTAL if this is all good. Also wondering how a regular run / exec differs from this health check

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 May 21, 2017

Contributor

@thaJeztah I have an idea what happened! Thanks for noticing the "HOSTNAME" thing.

Contributor

boaz1337 commented May 21, 2017

@thaJeztah I have an idea what happened! Thanks for noticing the "HOSTNAME" thing.

Add container environment variables correctly to the health check
The health check process doesn't have all the environment
varialbes in the container or has them set incorrectly.

This patch should fix that problem.

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

@boaz1337 boaz1337 changed the title from Clean unset variable in the health check process to Add container environment variables correctly to the health check May 21, 2017

@thaJeztah thaJeztah referenced this pull request May 31, 2017

Closed

17.06.0 RC2 tracker #2

23 of 23 tasks complete
@mlaventure

LGTM.

Thanks for the fix!

@mlaventure mlaventure merged commit 7c2f201 into moby:master May 31, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34389 has succeeded
Details
janky Jenkins build Docker-PRs 42987 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3374 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14425 has succeeded
Details
z Jenkins build Docker-PRs-s390x 3098 has succeeded
Details

@andrewhsu andrewhsu referenced this pull request Jun 6, 2017

Closed

17.06.0 RC3 tracker #8

40 of 40 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment