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

Update Dockerfiles to reference executable via ENTRYPOINT #815

Merged

Conversation

Projects
None yet
7 participants
@zdicesare
Copy link
Contributor

commented May 13, 2018

In all Dockerfiles, reference the executable in ENTRYPOINT to allow
consumers to override default parameters without needing to specify
the executable path. Standard defaults move to ENTRYPOINT too for the same reason.

Signed-off-by: Zachary DiCesare zachary.v.dicesare@gmail.com

Which problem is this PR solving?

Short description of the changes

  • In all Dockerfiles, reference the executable path and standard default arguments in ENTRYPOINT
    to allow consumers to override defaults without needing to specify them again.

  • Move hotrod default argument to CMD

@coveralls

This comment has been minimized.

Copy link

commented May 13, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 864d66b on zdicesare:feature/dockerfile-entrypoint into a4706c0 on jaegertracing:master.

@yurishkuro

This comment has been minimized.

Copy link
Member

commented May 13, 2018

build shows errors like this

jaeger-collector_1      | Error: unknown command "/go/bin/collector-linux" for "jaeger-collector"
@zdicesare

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Hey, sorry about that, I'm still looking into the cause.

@zdicesare zdicesare force-pushed the zdicesare:feature/dockerfile-entrypoint branch from e6f3657 to 39a93ce May 14, 2018

@zdicesare

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

This is ready for review. I'll add a PR to the documentation repository as well

@@ -8,4 +8,5 @@ COPY --from=certs /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certifica
EXPOSE 16686
COPY query-linux /go/bin/
ADD jaeger-ui-build /go/jaeger-ui/
CMD ["/go/bin/query-linux", "--query.static-files=/go/jaeger-ui/"]
ENTRYPOINT ["/go/bin/query-linux"]
CMD ["--query.static-files=/go/jaeger-ui/"]

This comment has been minimized.

Copy link
@vprithvi

vprithvi May 14, 2018

Member

The dockerfile docs state that ENTRYPOINT may be used with arguments, as such ENTRYPOINT ["top", "-b"].

I'm not clear on what advantage the combination for ENTRYPOINT to set the command and CMD to set defaults gives us. Could you elaborate?

This comment has been minimized.

Copy link
@vprithvi

vprithvi May 14, 2018

Member

To answer my own question: It seems that we need to use the --entrypoint option to override the entry point while anything after the image in the docker command overrides cmd

This comment has been minimized.

Copy link
@vprithvi

vprithvi May 14, 2018

Member

I was discussing with @yurishkuro about this, and agree with him in that moving "--query.static-files=/go/jaeger-ui" as an argument to the ENTRYPOINT command is better.

This is because overriding CMD in docker run overrides all the command parameters forcing users to pass in "--query.static-files=/go/jaeger-ui/".

By putting this in the ENTRYPOINT, other overrides become easier, and people may still override the static file location by using the -d option

This comment has been minimized.

Copy link
@zdicesare

zdicesare May 15, 2018

Author Contributor

That sounds reasonable to me, however in the case that a user did override query.static-files, the resulting command would look like:

/go/bin/query-linux --query.static-files=/go/jaeger-ui/ --query.static-files=/my/desired/path

which seems a little weird at first glance, but it works because flags sets the last value it encounters. Are we ok relying on this?

Also, it seems like the same change would make sense for the defaults in cmd/standalone/Dockerfile.

I'm very new to the Jaeger repo, so thank you for the patience here!

@vprithvi

This comment has been minimized.

Copy link
Member

commented May 14, 2018

lgtm, will merge when the documentation PR is up

Update Dockerfiles to reference executable via ENTRYPOINT
- In all Dockerfiles, reference the executable in ENTRYPOINT to allow
consumers to override default parameters without needing to specify
the executable path.

- Common default arguments in the query and standalone Dockerfiles
also move to ENTRYPOINT

- Move hotrod default argument to CMD

Signed-off-by: Zachary DiCesare <zachary.v.dicesare@gmail.com>

@zdicesare zdicesare force-pushed the zdicesare:feature/dockerfile-entrypoint branch from 39a93ce to 864d66b May 15, 2018

@zdicesare

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

Updated this PR w/ feedback, but want to make sure we're ok with the new usage

Documentation PR at jaegertracing/documentation#74

@yurishkuro

This comment has been minimized.

Copy link
Member

commented May 15, 2018

/go/bin/query-linux --query.static-files=/go/jaeger-ui/ --query.static-files=/my/desired/path

That's just how it would look internally, right? Don't think it matters as long as it works. Eventually we should fix #609 and not require this option to begin with.

@jpkrohling
Copy link
Member

left a comment

Note that this is a breaking change and will affect everyone overriding the default flags.

This should also be coordinated with changes on:

https://github.com/jaegertracing/jaeger-kubernetes
https://github.com/jaegertracing/jaeger-openshift
https://github.com/kubernetes/charts/tree/master/incubator/jaeger

@@ -22,4 +22,4 @@ COPY ./jaeger-ui-build /go/src/jaeger-ui-build
COPY ./cmd/standalone/standalone-linux /go/bin/
COPY ./cmd/standalone/sampling_strategies.json /go/src/config/

CMD ["/go/bin/standalone-linux","--query.static-files=/go/src/jaeger-ui-build/build/","--sampling.strategies-file=/go/src/config/sampling_strategies.json"]
ENTRYPOINT ["/go/bin/standalone-linux", "--query.static-files=/go/src/jaeger-ui-build/build/", "--sampling.strategies-file=/go/src/config/sampling_strategies.json"]

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling May 15, 2018

Member

While I think the static files would "never" change for the Docker image (or would they?), the strategies file are most likely to change in some scenarios. As it's easier to override a config file than a flag on an entrypoint directive, it would be best to use a configuration file instead of passing values via flags, like:

COPY ./cmd/standalone/standalone.yaml /go/src/config/
ENTRYPOINT ["/go/bin/standalone-linux", "--config-file=/go/src/config/standalone.yaml"]

./cmd/standalone/standalone.yaml doesn't exist yet, but could look like this:

    query:
      static-files: /go/jaeger-ui/
    sampling:
      strategies-file: /go/src/config/sampling_strategies.json

The other Dockerfiles should also follow the same pattern.

And finally, I would personally prefer /conf/ instead of /go/src/config/, and /bin/ instead of /go/bin/, unless there's a specific reason to keep go in the path name.

cc @yurishkuro, @pavolloffay

This comment has been minimized.

Copy link
@pavolloffay

pavolloffay May 15, 2018

Member

I don't have a strong opinion on directories.

I have noticed that some images put binaries into /bin or /usr/share and configuration into /etc so in our case /etc/jaeger

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling May 15, 2018

Member

Those images are probably following the LSB (Linux Standard Base), which is a standard for this kind of thing on the Linux world. I'd be OK with that, if we do indeed place the config files at /etc/jaeger/standalone.yaml instead of /etc/jaeger.yaml.

This comment has been minimized.

Copy link
@pavolloffay

pavolloffay May 15, 2018

Member

I meant /etc/jaeger as a dir for whatever configuration we need.

This comment has been minimized.

Copy link
@pavolloffay

pavolloffay May 15, 2018

Member

But as I said I don't have a strong opinion. I am not sure what benefits putting binaries into /bin has as we use from scratch. One way or another following standard Linux conventions is not a bad thing :).

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling May 16, 2018

Member

If there are no options that need to be passed, then there's no reason to have a config file.

If there are CLI options to be passed (as is the case for the current state of this PR), then it's easier for a consumer to override a config file than override the entrypoint directive.

This comment has been minimized.

Copy link
@zdicesare

zdicesare May 16, 2018

Author Contributor

I may be a step behind you here- but doesn't the consumer overriding a config file via an argument to the container require a volume mount? In that case it seems easier to override the entrypoint with additional command line arguments.

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Jul 9, 2018

Member

The embedding of UI assets is done in #918. Once it's merged I suggest we change this one to

ENTRYPOINT ["/go/bin/standalone-linux"]
CMD ["--sampling.strategies-file=/go/src/config/sampling_strategies.json"]

I now see the value of using the config as the lowest priority default value, but I propose we leave it for another PR, as it won't be such a breaking change as this PR.

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling Jul 9, 2018

Member

Or rather:

ENTRYPOINT ["/go/bin/standalone-linux"]
CMD ["--sampling.strategies-file=/etc/jaeger/sampling_strategies.json"]

https://github.com/jaegertracing/jaeger/pull/918/files#diff-da47d3e14c88bf43b29700d0fa2b7a27R22

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling Jul 9, 2018

Member

@jkandasa scenarios to test:

  • Override the CMD with a custom option (--log-level, for instance)
  • Override the /etc/jaeger/sampling_strategies.json file
@zdicesare

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

Additional PRs at:
jaegertracing/jaeger-openshift#81
jaegertracing/jaeger-kubernetes#87

Command line params to the jaeger containers in https://github.com/kubernetes/charts/tree/master/incubator/jaeger are all empty, and I did not see any references to the executables in that repo but definitely could use a confirmation on that.

@jpkrohling

This comment has been minimized.

Copy link
Member

commented May 16, 2018

Command line params to the jaeger containers in https://github.com/kubernetes/charts/tree/master/incubator/jaeger are all empty

It would then need only a version bump, to make use of the new feature.

but definitely could use a confirmation on that.

Also looks empty to me, but the best person to confirm would be @dvonthenen

@dvonthenen

This comment has been minimized.

Copy link

commented May 16, 2018

@jpkrohling @zdicesare Yup, command line params are empty. All configuration is done via ENV variables. A bump in the version should take care of it.

@@ -8,4 +8,4 @@ COPY --from=certs /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certifica
EXPOSE 16686
COPY query-linux /go/bin/
ADD jaeger-ui-build /go/jaeger-ui/
CMD ["/go/bin/query-linux", "--query.static-files=/go/jaeger-ui/"]
ENTRYPOINT ["/go/bin/query-linux", "--query.static-files=/go/jaeger-ui/"]

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling Jul 9, 2018

Member

After #918, the --query.static-files option is optional and the assets are embedded into the binary, so, remove this from the entrypoint.

@@ -80,4 +80,4 @@ services:

jaeger-query:
# override to disable static files
command: ["/go/bin/query-linux", "--query.static-files=", "--cassandra.keyspace=jaeger_v1_dc1", "--cassandra.servers=cassandra"]
command: ["--query.static-files=", "--cassandra.keyspace=jaeger_v1_dc1", "--cassandra.servers=cassandra"]

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling Jul 9, 2018

Member

The first option can be removed

@ghost ghost assigned yurishkuro Jul 9, 2018

@ghost ghost added the review label Jul 9, 2018

@codecov

This comment has been minimized.

Copy link

commented Jul 9, 2018

Codecov Report

Merging #815 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #815   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         126    126           
  Lines        6070   6070           
=====================================
  Hits         6070   6070

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bbdeeb...38500a5. Read the comment docs.

@zdicesare

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

Thank you for updating this @yurishkuro, I can work on updating the other PRs with new images when this is out, including creating a version bump PR in https://github.com/kubernetes/charts/tree/master/incubator/jaeger

@yurishkuro
Copy link
Member

left a comment

🎉

@yurishkuro yurishkuro merged commit 48bbbae into jaegertracing:master Jul 9, 2018

5 checks passed

DCO All commits have a DCO sign-off from the author
Details
WIP ready for review
Details
codecov/patch Coverage not affected when comparing 5bbdeeb...38500a5
Details
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the review label Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.