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

Make it possible to use Stack logging in Docker #65778

Merged
merged 17 commits into from
Dec 10, 2020

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Dec 2, 2020

Closes #62758.

Include the Stack log4j config in the Docker image, in order to make it
possible to write logs in a container environment in the same way as for
an archive or package deployment. This is useful in situations where the
user is bind-mounting the logs directory and has their own arrangements
for log shipping.

To use stack logging, set the environment variable ES_LOG_STYLE to
file. It can also be set to console, which is the same as not
specifying it at all.

The Docker logging config is now auto-generated at image build time, by
running the default config through a transformer program when preparing
the distribution in an image builder step.

Implementation notes:

  • Regarding the docker config change - The major constraint on the implementation here is the need to publish reproducible build files to Docker hub. It means that all you have to work with is the archive distro of ES and whatever you put in the build context. For that reason I opted for an executable jar, since the v8.0 image has almost nothing useful in it otherwise. Same thing goes for Iron Bank - they build the images, not us.
  • In the docker distribution build.gradle, I changed a helper closure into a class with a static method in order to work around an issue where the Docker image was always being rebuilt, even when there were no changes.

Closes elastic#62758.

Include the Stack log4j config in the Docker image, in order to make it
possible to write logs in a container environment in the same way as for
an archive or package deployment. This is useful in situations where the
user is bind-mounting the logs directory and has their own arrangements
for log shipping.
@pugnascotia pugnascotia added >enhancement :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.11.0 labels Dec 2, 2020
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Dec 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

This doesn't seem to address the configuration drift issue as described in #62758. So we still have the problem of multiple logging config files that need to be kept in since. If we still intend to address that we should open a separate issue for that.

Second, and mostly from curiosity, is there a specific use case for using the file-based logging? Is this something cloud requested?

distribution/docker/src/docker/bin/docker-entrypoint.sh Outdated Show resolved Hide resolved
Rather than maintaining a separate log4j config specifically for
Docker, we can generate a config by processing the ES distribtution's
default configuration so that everything is emitted to the console.
@pugnascotia
Copy link
Contributor Author

@mark-vieira yes, Cloud don't want to write to the console since they use filebeat to ship their logs, and would prefer to be able to use the default logging config.

I also make an attempt at transforming the default config into a console-only config. The major constraint on the implementation here is the need to publish reproducible build files to Docker hub. It means that all you have to work with is the archive distro of ES and whatever you put in the build context. For that reason I opted for a jshell script, since the v8.0 image has almost nothing useful in it otherwise. We could probably change it to a standard Java class if you think that using jshell is too gonzo, but we'd still need to put the source in the Docker context in order to provide reproducible builds. Same thing goes for Iron Bank - they build the images, not us.

@pugnascotia
Copy link
Contributor Author

BTW I ran the Docker test for the default and UBI distros locally, and also build the Iron Bank image.

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2 - known issue.

@mark-vieira
Copy link
Contributor

We could probably change it to a standard Java class if you think that using jshell is too gonzo

My main concern is lack of test coverage. I say let's just punt on that for now. We don't have to solve the config drift issue as part of this PR. My preference however would be to use some kind of log4j2 construct here. A custom "tee" appender perhaps that could send logging to one of two locations based on some config/env var/etc.

@pugnascotia
Copy link
Contributor Author

Looking at the RoutingAppender that you pointed me at, I believe we'd need to configure it for every appender that we currently, which would be pretty ugly. I'd rather make the transformer script testable.

What about moving it to distribution/docker/src/main/java, along with tests, then compiling and running it in the Docker build?

@mark-vieira
Copy link
Contributor

mark-vieira commented Dec 3, 2020

What about moving it to distribution/docker/src/main/java, along with tests, then compiling and running it in the Docker build?

That's probably messy to call javac in the Dockerfile. Could we put it in a separate subproject, package it in a jar and then ship that jar in the dockerfile. Effectively treat it like one of our cli tools?

@pugnascotia pugnascotia requested a review from breskeby December 7, 2020 15:50
@pugnascotia
Copy link
Contributor Author

@mark-vieira OK, we're now using an executable jar instead of jshell.

@breskeby any thoughts on improving the new sub-project?

@pugnascotia
Copy link
Contributor Author

@breskeby I'd particularly like a review on the new gradle config in this PR.

Copy link
Contributor

@breskeby breskeby left a comment

Choose a reason for hiding this comment

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

lgtm. some minor remarks.

I remember there were memory leaking issues reported with using Groovy template expansion as we rely on here in our docker file setups atm. Therefore in the long term ideally we would move away from google template expansion (but totally out of scope here, just as a general comment as we also havn't received related issues reported for the es build afaik).


/**
* Used in the Dockerfile template to wrap commands in a retry loop. It can't be
* a closure because Gradle can't effectively cache those.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate what can't be cached here?

* Used in the Dockerfile template to wrap commands in a retry loop. It can't be
* a closure because Gradle can't effectively cache those.
*/
class Retry {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I'm in favour to have class definitions living in buildSrc somewhere no matter how small they are. To keep our build scripts a bit cleaner and smaller. Also these classes should change less often than the script itself.

@@ -100,7 +108,7 @@ ${indent}${exitKeyword} \$exit_code"""
'docker_base' : base.name().toLowerCase(),
'version' : VersionProperties.elasticsearch,
'major_minor_version' : "${major}.${minor}",
'retry_loop' : retry_loop
'retry' : Retry,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the , is too much here

@pugnascotia
Copy link
Contributor Author

@breskeby I moved the retry class to buildSrc.

The logic for build the the Dockerfile is already pretty hairy, so I wouldn't be surprised if we choose to spin that off into its own class at some point. It seems to be working well enough at the moment however.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

@pugnascotia pugnascotia merged commit 68b5465 into elastic:master Dec 10, 2020
@pugnascotia pugnascotia deleted the 62758-docker-stack-logging branch December 10, 2020 12:25
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Dec 10, 2020
Backport of elastic#65778.

Closes elastic#62758.

Include the Stack log4j config in the Docker image, in order to
make it possible to write logs in a container environment in the
same way as for an archive or package deployment. This is useful
in situations where the user is bind-mounting the logs directory
and has their own arrangements for log shipping.

To use stack logging, set the environment variable `ES_LOG_STYLE`
to `file`. It can also be set to `console`, which is the same as
not specifying it at all.

The Docker logging config is now auto-generated at image build time,
by running the default config through a transformer program when
preparing the distribution in an image builder step.

Also, in the docker distribution `build.gradle`, I changed a helper
closure into a class with a static method in order to fix an
issue where the Docker image was always being rebuilt, even when
there were no changes.
pugnascotia added a commit that referenced this pull request Dec 10, 2020
Backport of #65778.

Closes #62758.

Include the Stack log4j config in the Docker image, in order to
make it possible to write logs in a container environment in the
same way as for an archive or package deployment. This is useful
in situations where the user is bind-mounting the logs directory
and has their own arrangements for log shipping.

To use stack logging, set the environment variable `ES_LOG_STYLE`
to `file`. It can also be set to `console`, which is the same as
not specifying it at all.

The Docker logging config is now auto-generated at image build time,
by running the default config through a transformer program when
preparing the distribution in an image builder step.

Also, in the docker distribution `build.gradle`, I changed a helper
closure into a class with a static method in order to fix an
issue where the Docker image was always being rebuilt, even when
there were no changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >enhancement Team:Delivery Meta label for Delivery team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include the default log4j config in Docker images
5 participants