hack/env: Remove tmp volume #16686

detiber · 2017-10-04T20:18:36Z

hack/env: Remove tmp volume

Use host TMPDIR or pass in /tmp and /var/tmp from host instead of using a volume for /tmp
Add BASEVARTMPDIR variable and update tito to use it

Resolves: #15573
Removes the use of a volume for /tmp introduced here: #15770
Resolves: #16273

openshift-merge-robot · 2017-10-04T20:18:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: detiber
We suggest the following additional approver: stevekuznetsov

Assign the PR to them by writing /assign @stevekuznetsov in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files:

hack/lib/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

detiber · 2017-10-04T20:19:27Z

/assign @smarterclayton @jim-minter
/unassign @jhadvig

smarterclayton · 2017-10-04T20:28:59Z

hack/lib/build/environment.sh

@@ -9,7 +9,7 @@
 function os::build::environment::create() {
  set -o errexit
  local release_image="${OS_BUILD_ENV_IMAGE}"
-  local additional_context="${OS_BUILD_ENV_DOCKER_ARGS:-}"
+  local additional_context="${OS_BUILD_ENV_DOCKER_ARGS:-} --tmpfs /tmp:exec"


what default limits do /tmpfs have?

@smarterclayton this tells me it is rw, noexec, nosuid, size=65536k

Inspecting a container run without specifying any mount options:

docker run --rm --tmpfs /tmp -it centos:7 mount | grep /tmp tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noexec,relatime,context="system_u:object_r:container_file_t:s0:c280,c857")

jim-minter · 2017-10-04T20:44:30Z

Great :)
I guess a follow-up will be to remove references to OS_BUILD_ENV_TMP_VOLUME in openshift/aos-cd-jobs.

detiber · 2017-10-04T20:51:18Z

@jim-minter like this?

jim-minter · 2017-10-04T20:54:03Z

@detiber ideal.

mfojtik · 2017-10-05T10:22:06Z

yey, this might actually fix a tons of etcd flakes :)

jim-minter · 2017-10-05T16:15:59Z

On IRC @smarterclayton requested that hack/env respect $TMPDIR. It should use mktemp to create a dedicated directory under $TMPDIR (and clean up after) and bind mount that to /tmp in the container.
That way if $TMPDIR is running on a tmpfs on the host, so it is in the container -- and if not, not.

detiber · 2017-10-06T21:24:46Z

@jim-minter @stevekuznetsov @smarterclayton ptal, This now respects $TMPDIR if set, otherwise mounts in a subdirectory of both /tmp and /var/tmp. I also added a $BASEVARTMP var to hack/lib/util/environment.sh so that scripts can distinguish between fast/volatile temp space and slower/persistent temp space, with both getting overridden by $TMPDIR if set to conform as closely to both POSIX and FHS standards as possible.

stevekuznetsov

Overall seems reasonable to me

stevekuznetsov · 2017-10-09T12:27:41Z

hack/env

@@ -31,12 +31,12 @@
 #

 # NOTE:   only committed code is built.
-source "$(dirname "${BASH_SOURCE}")/lib/init.sh"
+OS_TMP_ENV_SET=y source "$(dirname "${BASH_SOURCE}")/lib/init.sh"


This is to prevent os::util::environment::setup_tmpdir_vars from being called, otherwise we are making a quite a few unnecessary temporary directories that are not used by hack/env.

Are they not? The output from os::log::* for instance will go to _output/scripts/env/logs/script.log

I've removed it with the latest push.

stevekuznetsov · 2017-10-09T12:28:28Z

hack/lib/build/environment.sh

+  local tmpdir_template="openshift-env-XXXXXXXXXX"
+  if [[ -n "${TMPDIR:-}" ]]; then
+    # If TMPDIR is specified, respect it
+    local container_tmpdir=$(mktemp -d --tmpdir ${tmpdir_template})


don't mix scoping statements (local) with expressions that evaluate, like subshells

stevekuznetsov · 2017-10-09T12:29:15Z

hack/lib/build/environment.sh

+      # get the value of TMPDIR from the container
+      local container_tmpdir=$(docker inspect -f '{{range $index, $value := .Config.Env}}{{if eq (index (split $value "=") 0) "TMPDIR"}}{{index (split $value "=") 1}}{{end}}{{end}}' "${container}")
+      os::log::debug "Removing container tmp directory: ${container_tmpdir}"
+      rm -rf "${container_tmpdir}" || os::log::warning "Failed to remove tmpdir: ${container_tmpdir}"


if ! rm -rf; then os::log::warning ... fi

stevekuznetsov · 2017-10-09T12:29:24Z

hack/lib/build/environment.sh

+      os::log::debug "Removing container tmp directories: ${container_tmpdirs[@]}"
+      for tmpdir in "${container_tmpdirs[@]}"; do
+	for tmpdir_prefix in /tmp /var/tmp; do
+          if [[ "${tmpdir}" == ${tmpdir_prefix}/* ]]; then


mixed indents

detiber · 2017-10-09T17:03:04Z

/retest

- Use host TMPDIR or pass in /tmp and /var/tmp from host instead of using a volume for /tmp - Add BASEVARTMPDIR variable and update tito to use it

detiber · 2017-10-09T21:45:30Z

/retest

detiber · 2017-10-10T15:31:10Z

/retest

detiber · 2017-10-10T15:46:56Z

@smarterclayton @stevekuznetsov I believe all issues and comments have been addressed.

openshift-ci-robot · 2017-10-10T16:48:43Z

@detiber: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/experimental/integration	`adaccdc`	link	`/test origin-it`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

detiber · 2017-10-12T16:12:48Z

@stevekuznetsov @smarterclayton anything else needed to get this in the queue?

stevekuznetsov · 2017-10-16T17:32:43Z

This LGTM -- @smarterclayton any last comments?

bparees · 2017-11-03T17:36:48Z

this is supposedly a fix to one of our top flakes #16273

@mfojtik since your team owns the flake, can you figure out how to get this PR merged?

openshift-merge-robot · 2017-11-10T08:11:37Z

@detiber PR needs rebase

mfojtik · 2017-12-05T12:31:30Z

@detiber can you please rebase?

stevekuznetsov · 2018-01-23T18:40:38Z

Superseded by #18242

/close

openshift-merge-robot assigned stevekuznetsov and jhadvig Oct 4, 2017

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 4, 2017

openshift-ci-robot assigned jim-minter and smarterclayton and unassigned jhadvig Oct 4, 2017

smarterclayton reviewed Oct 4, 2017

View reviewed changes

detiber mentioned this pull request Oct 4, 2017

Remove use of OS_BUILD_ENV_TMP_VOLUME openshift-eng/aos-cd-jobs#707

Closed

mfojtik mentioned this pull request Oct 6, 2017

github.com/openshift/origin/test/setup/start-master: /healthz failure #16273

Closed

detiber changed the title ~~hack/env: use tmpfs for /tmp instead of volume~~ [WIP] hack/env: use tmpfs for /tmp instead of volume Oct 6, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2017

detiber force-pushed the tmpfs branch from 376d0a8 to b88ec03 Compare October 6, 2017 21:19

detiber changed the title ~~[WIP] hack/env: use tmpfs for /tmp instead of volume~~ hack/env: Remove tmp volume Oct 6, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2017

stevekuznetsov reviewed Oct 9, 2017

View reviewed changes

detiber force-pushed the tmpfs branch from b88ec03 to fe81673 Compare October 9, 2017 15:15

hack/env: Remove tmp volume

adaccdc

- Use host TMPDIR or pass in /tmp and /var/tmp from host instead of using a volume for /tmp - Add BASEVARTMPDIR variable and update tito to use it

detiber force-pushed the tmpfs branch from fe81673 to adaccdc Compare October 9, 2017 19:32

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2017

ironcladlou mentioned this pull request Nov 28, 2017

flake: test-cmd does not start up correctly sometimes #17458

Closed

mfojtik mentioned this pull request Dec 5, 2017

hack/env: Remove tmp volume #17603

Closed

stevekuznetsov mentioned this pull request Jan 23, 2018

test-cmd and test-end-to-end not using tmpfs for etcd #18240

Closed

openshift-ci-robot closed this Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hack/env: Remove tmp volume #16686

hack/env: Remove tmp volume #16686

detiber commented Oct 4, 2017 •

edited

Loading

openshift-merge-robot commented Oct 4, 2017

detiber commented Oct 4, 2017

smarterclayton Oct 4, 2017

detiber Oct 4, 2017 •

edited

Loading

detiber Oct 4, 2017

jim-minter commented Oct 4, 2017

detiber commented Oct 4, 2017

jim-minter commented Oct 4, 2017

mfojtik commented Oct 5, 2017

jim-minter commented Oct 5, 2017

detiber commented Oct 6, 2017

stevekuznetsov left a comment

stevekuznetsov Oct 9, 2017

detiber Oct 9, 2017

stevekuznetsov Oct 9, 2017

detiber Oct 9, 2017

stevekuznetsov Oct 9, 2017

stevekuznetsov Oct 9, 2017

stevekuznetsov Oct 9, 2017

detiber commented Oct 9, 2017

detiber commented Oct 9, 2017

detiber commented Oct 10, 2017

detiber commented Oct 10, 2017

openshift-ci-robot commented Oct 10, 2017

detiber commented Oct 12, 2017

stevekuznetsov commented Oct 16, 2017

bparees commented Nov 3, 2017

openshift-merge-robot commented Nov 10, 2017

mfojtik commented Dec 5, 2017

stevekuznetsov commented Jan 23, 2018

hack/env: Remove tmp volume #16686

hack/env: Remove tmp volume #16686

Conversation

detiber commented Oct 4, 2017 • edited Loading

openshift-merge-robot commented Oct 4, 2017

detiber commented Oct 4, 2017

Choose a reason for hiding this comment

detiber Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Oct 4, 2017

detiber commented Oct 4, 2017

jim-minter commented Oct 4, 2017

mfojtik commented Oct 5, 2017

jim-minter commented Oct 5, 2017

detiber commented Oct 6, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detiber commented Oct 9, 2017

detiber commented Oct 9, 2017

detiber commented Oct 10, 2017

detiber commented Oct 10, 2017

openshift-ci-robot commented Oct 10, 2017

detiber commented Oct 12, 2017

stevekuznetsov commented Oct 16, 2017

bparees commented Nov 3, 2017

openshift-merge-robot commented Nov 10, 2017

mfojtik commented Dec 5, 2017

stevekuznetsov commented Jan 23, 2018

detiber commented Oct 4, 2017 •

edited

Loading

detiber Oct 4, 2017 •

edited

Loading