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

jemalloc support - vendored in - source built in Dockerfile #788

Merged
merged 1 commit into from Dec 1, 2017

Conversation

richm
Copy link
Contributor

@richm richm commented Nov 16, 2017

jemalloc support
docker build will pull down the released tarball, configure, and
make install
If you want to use jemalloc:

oc set env ds/logging-fluentd USE_JEMALLOC=true

This will cause fluentd to run with jemalloc. It is off by default.

This also disables fluentd supervisor mode so that we
are only running the one fluentd process with jemalloc.

You can set jemalloc options by

oc set env ds/logging-fluentd MALLOC_CONF="stats_print:true,prof:true,...."

see https://github.com/jemalloc/jemalloc/wiki/Getting-Started for more
information
/test

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 16, 2017
@richm richm added area/performance component/fluentd do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/enhancement labels Nov 16, 2017
iproute && \
yum clean all

RUN mkdir -p ${HOME} && \
Copy link
Contributor

Choose a reason for hiding this comment

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

In order for us to use downstream, why not actually vendor in the dependency instead of pulling it in via curl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcantrill I don't know if we can do downstream without an rpm package . . . but sure, I can vendor it in - I just didn't want to check in a big tarball into git. I suppose I could check in the uncompressed source tree, but if I'm going to do that, I should just make it a git submodule

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to vendor the source but if that is our only recourse we should do it. Alternatively, maybe its worth us setting up the necessary repos to properly build it into an rpm. Is that something you are willing to take on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcantrill for downstream. separate rpm for the rhlog channel, then we just tag those builds in to build the fluentd image

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 17, 2017
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 17, 2017
@richm
Copy link
Contributor Author

richm commented Nov 18, 2017

Are we ok with this? If so, I'll remove the do-not-merge label, and someone can give it a /lgtm

@@ -1,14 +1,14 @@
#!/bin/bash

fluentdargs="--no-supervisor"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary to use jemalloc?

@jcantrill
Copy link
Contributor

@richm I am find merging this PR as long as we have a story for downstream

Copy link
Contributor

@ewolinetz ewolinetz left a comment

Choose a reason for hiding this comment

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

Also curious about the purpose of the fluentd arg --no-supervisor otherwise LGTM

@richm
Copy link
Contributor Author

richm commented Nov 20, 2017

the purpose of the fluentd arg --no-supervisor

it is problematic for monitoring and metrics to have two separate fluentd processes running in each pod. It is even more problematic if we want to use jemalloc stats/tracing features, because we will get two sets of stats. When running in a container, it is much better to let the container runtime (i.e. openshift) manage the process lifecycle rather than an external supervisor process running inside the same pod/container.

Copy link
Contributor

@portante portante left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2017
@richm richm removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2017
@richm
Copy link
Contributor Author

richm commented Nov 30, 2017

/retest

@richm
Copy link
Contributor Author

richm commented Nov 30, 2017

fixes bz 1502764 slow memory leak in logging-mux during steady-state logging.
https://bugzilla.redhat.com/show_bug.cgi?id=1502764

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2017
jemalloc is vendored in as a git submodule under fluentd
docker build will copy the source to the image, and build
and install the binaries and libraries in the image

If jemalloc is off, and you want to use it:

    oc set env ds/logging-fluentd USE_JEMALLOC=true

This commit also disables fluentd supervisor mode so that we
are only running the one fluentd process with jemalloc.

You can set jemalloc options by

    oc set env ds/logging-fluentd MALLOC_CONF="stats_print:true,prof:true,...."

see https://github.com/jemalloc/jemalloc/wiki/Getting-Started for more
information
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 30, 2017
@richm
Copy link
Contributor Author

richm commented Nov 30, 2017

/retest

@richm richm added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2017
@richm
Copy link
Contributor Author

richm commented Dec 1, 2017

/test json-file

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit ce27ba0 into openshift:master Dec 1, 2017
@richm richm deleted the jemalloc branch December 1, 2017 17:25
@richm
Copy link
Contributor Author

richm commented Dec 1, 2017

/cherrypick release-3.7

@openshift-cherrypick-robot

@richm: New pull request created: #807

In response to this:

/cherrypick release-3.7

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.

openshift-merge-robot added a commit that referenced this pull request Dec 7, 2017
…88-to-release-3.7

Automatic merge from submit-queue.

Automated cherry-pick of #788 on release-3.7

This is an automated cherry-pick of #788

/assign richm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance component/fluentd lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants