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.

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

Allow a mounted repository to be injected into yum #9947

Merged
merged 3 commits into from
Jul 22, 2016

Conversation

smarterclayton
Copy link
Contributor

Mounting a yum repository file at /var/run/secrets/overrides.repo will
allow any image based on this to use those repositories.

@smarterclayton
Copy link
Contributor Author

This adds --mount to oc ex dockerbuild, which allows a client to bind mount in a secret to the build at runtime. We'll need to pass this flag in the build if we want it to be respected.

Also, added the symlink to the overrides repo, so adding the new repo to /usr/share/rhel/secrets/overrides.repo should be sufficient on a RHEL system to trigger using the mirror.

@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

Addresses part of #8571, still need to determine whether we need the "magic" solution for the CI machines or whether we just support additional flags to the builders.

There is no reason not to
Allows files to be bind mounted into the running container that will not
be part of the final build.
@smarterclayton
Copy link
Contributor Author

@stevekuznetsov this is the PR for adding the mount. We'll want to

  1. Set up an overrides repo somewhere on the AMI (location doesn't matter)
  2. Add this env var that is used when hack/build-base-images.sh and make release are invoked: OS_BUILD_IMAGE_ARGS="--mount PATH_TO_OVERRIDES:/etc/yum.repos.d/overrides.repo"

@stevekuznetsov
Copy link
Contributor

@smarterclayton and what's the use case for arbitrary args given how difficult it is to do properly? Is it not alright to have a log-level and over-ride location as two named args for the make target?

@smarterclayton
Copy link
Contributor Author

I don't want the complexity of those args to surface in Make or the test environment.

@stevekuznetsov
Copy link
Contributor

The fragility and complexity of trying to and failing to handle the "any args" case doesn't seem preferable to me ...

@smarterclayton
Copy link
Contributor Author

I don't see how it's failing.

@smarterclayton
Copy link
Contributor Author

Show me how this will fail

@@ -17,7 +17,12 @@ Build a Dockerfile into a single layer

.PP
Builds the provided directory with a Dockerfile into a single layered image.
Requires that you have a working connection to a Docker engine.
Requires that you have a working connection to a Docker engine. You may mount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's kinda bizarre to me that this is part of the oc tooling when it's not actually interacting with openshift at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so we have a vehicle for it to be testable. Eventually I think both this and s2i commands would be under the CLI.

@stevekuznetsov
Copy link
Contributor

Anything you pass through with whitespace is going to be split and not make it to the script the way you wanted.

Not only do I see there being merit to a global LOGLEVEL variable in our Makefile for other jobs that run OpenShift server or client stuff, but also we already have job-specific variables for test jobs. REPO_OVERRIDE would be the only job-specific variable that this would introduce...

I don't know... it just seems like Makefile has been set up explicitly not to handle this case and also explicitly to handle well-defined, named, variables. Working around it in a fragile way reeks of the "Bash hammer" that I really don't like us using.

@stevekuznetsov
Copy link
Contributor

One failure condition, for example -- what if the path to the override file has a space in it?

sharedMount = v.Mountpoint
opts.HostConfig = &docker.HostConfig{
Binds: []string{sharedMount + ":/tmp/__temporarymount"},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry but i'm seriously struggling to follow what is going on here... it seems like you're creating a docker volume, then setting up a bindmount for that (empty) volume.

Then you copy the source paths the user specified into that volume? Except when you do the copies, you are copying to /tmp/_temporarymount, which i would have thought is the path inside the container, not the path on the host that represents the volume. So i don't see how that copy actually works (but i haven't dug into what e.Copy is actually doing).

Then you bindmount all those copied directories, again seemingly mounting from the target path of the bindmount, not the source(host) path, so the container is bindmounting from within itself?

I would have expected something much more straightforward (setup the host paths, mount them into the container, then delete the host paths before committing the image(I guess))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to do this without being local to the machine, so I need a volume. If you try to bind mount something before it exists, it gets bind mounted as a dir. I don't want the bind mounts to be writable (otherwise we have a quota problem), so I need a two phase mount + copy, then remount.

@smarterclayton
Copy link
Contributor Author

I'll do

FOO="--bar 'baz'" bash -c 'echo $FOO'
--bar 'baz'

This does not have to support arbitrary input.

@stevekuznetsov
Copy link
Contributor

So Joe Shmoe has:
OS_BUILD_IMAGE_ARGS="--mount /home/joe/my long name/overrides.repo:/etc/yum.repos.d/overrides.repo"

And your house of cards comes tumbling down. I really don't get why you need an arbitrary number of flags -- do you expect this to grow? If we want to do one thing, and that one thing is to enable the over-ride flag to be set, pass the one thing you need (override location on host) in as the one envar, defined in the Makefile and everything.

@smarterclayton
Copy link
Contributor Author

Joe Schmoe does:

OS_BUILD_IMAGE_ARGS="--mount '/home/joe/my long  name/overrides.repo:/etc/yum.repos.d/overrides.repo'"

which works for me.

@smarterclayton
Copy link
Contributor Author

Also I may have to specify multiple mounts in the future (which has the same array problem).

@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Jul 20, 2016

$ my_args="--flag='long value'"
$ for arg in ${my_args}; do echo "${arg}"; done
--flag='long
value'

@stevekuznetsov
Copy link
Contributor

I may have to specify multiple mounts in the future

fair enough

Allows a set of arguments to be passed to the image builder
@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jul 20, 2016 via email

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 20, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6690/) (Image: devenv-rhel7_4643)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5fb8206

@smarterclayton
Copy link
Contributor Author

[test]

2 similar comments
@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

[test]

On Thu, Jul 21, 2016 at 1:05 AM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6639/)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9947 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p4g8XpajxYKcEbYcdt7iDkmKYoi9ks5qXv4egaJpZM4JQZJm
.

@stevekuznetsov
Copy link
Contributor

@smarterclayton post what flake happened on each failed job before you re-trigger tests so the poor sod that investigates them later has logs to look through

@smarterclayton
Copy link
Contributor Author

[test] #9929

On Thu, Jul 21, 2016 at 10:50 AM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6654/)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9947 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_py4HFMCPt1155HhYzoMRkNGYpbV1ks5qX4dPgaJpZM4JQZJm
.

@smarterclayton
Copy link
Contributor Author

[test] database sync bug and extended docker flake

@smarterclayton
Copy link
Contributor Author

Cluster resource quota flake [test]

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Jul 22, 2016 via email

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5fb8206

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6690/)

@smarterclayton
Copy link
Contributor Author

OMG.

On Thu, Jul 21, 2016 at 10:26 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6690/)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9947 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p_QL2vSO_lDcpJ7fTBKTJ4dJQOZcks5qYCpogaJpZM4JQZJm
.

@smarterclayton
Copy link
Contributor Author

[merge]

@openshift-bot openshift-bot merged commit cee4861 into openshift:master Jul 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants