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

Add docs for Docker images #4312

Merged
merged 1 commit into from
May 23, 2017
Merged

Add docs for Docker images #4312

merged 1 commit into from
May 23, 2017

Conversation

ninaspitfire
Copy link
Contributor

  • Add Docker image info to the Getting Started pages.
  • Add/extend dedicated Docker page for each (Unix) Beat.
  • Refactor some duplicated fragments into includes with attributes.
  • Remove some whitespaces (sorry for the noisy diff, my editor just did this stuff).

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

LGTM (just one minor comment). Thanks so much for the refactoring!

code can be found on
https://github.com/elastic/beats-docker/tree/{doc-branch}[GitHub].

=== Configuring {beatname_uc} on Docker
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a [float] tag to make this a sub-section of the main topic (right now, when the doc is built, it becomes a separate topic). You might need to add [float] tags to the other sub-sections in this topic. The main point is that all the content about Running on Docker should be in a single topic, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I added the [float] tags; it looks much better now.

I'm glad I didn't cross a line with the refactors. I always get nervous pushing patches for new projects in new languages. :)

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

I added a few comments for the non linux platforms, apart from those this LGTM!


endif::[]

The base image is https://hub.docker.com/_/centos/[Centos 7] and the source
Copy link
Contributor

Choose a reason for hiding this comment

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

Use [centos:7] for consistency.


[float]
[[monitoring-host]]
=== Monitoring the Host Machine
When executing Metricbeat in a container, there are some important
things to be aware of if you want to monitor the host machine or other
containers. Let's walk-through some examples using Docker as our container
orchestration tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

A few clarifications or warnings could be provided for Docker on Windows + Mac users where the bind mounts won't work as expected.

isolated virtual network, with a limited view of network traffic. You may wish
to connect the container directly to the host network in order to see traffic
destined for, and originating from, the host system. With +docker run+, this can
be achieved by specifying +--network=host+.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here, for users on Docker for Mac+Windows have a VM layer that won't let them sniff things happening on the real host OS, unless settings have been tweaker.

@ninaspitfire
Copy link
Contributor Author

Thanks, @dliappis. Added your suggestions.

@ninaspitfire
Copy link
Contributor Author

OK to merge this now, @ruflin?

@ruflin
Copy link
Contributor

ruflin commented May 22, 2017

Happy to merge if @dliappis gives a 👍

@ruflin
Copy link
Contributor

ruflin commented May 22, 2017

jenkins, retest it

@dliappis
Copy link
Contributor

LGTM from me as well

@ruflin ruflin merged commit 8915c16 into elastic:master May 23, 2017
@ruflin
Copy link
Contributor

ruflin commented May 23, 2017

@dedemorton Can you take care of the backporting in case we backport this?

@dedemorton
Copy link
Contributor

@ruflin Sure. I just need to backport this to 5.4 right?

@ninaspitfire
Copy link
Contributor Author

@dedemorton: yes, thanks. The images in question existed in "alpha" state earlier, but I think it's best to call 5.4 the release version, and thus only backport that far.

dedemorton pushed a commit to dedemorton/beats that referenced this pull request May 30, 2017
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label May 30, 2017
dedemorton added a commit that referenced this pull request Jun 6, 2017
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Jun 16, 2017
dedemorton added a commit that referenced this pull request Jun 21, 2017
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants