User volumes for service logs by mtojek · Pull Request #105 · elastic/elastic-package

mtojek · 2020-09-17T12:33:42Z

Closes: #85

I'm opening as draft, because we need to have a service that writes logs to files.

I introduced also service logs direcotyr env: ELASTIC_PACKAGE_SERVICE_LOGS_DIR

ycombinator · 2020-09-17T12:38:42Z

internal/testrunner/runners/system/servicedeployer/compose.go

-	cmd = exec.Command("docker", "network", "connect", stackNetwork, serviceContainer)
-	if err := cmd.Run(); err != nil {
-		return nil, errors.Wrap(err, "could not attach service container to stack network")
-	}


I think we still need to attach the service container to the stack network for metrics data streams to work, no?

I forgot about this requirement. I will bring it back.

elasticmachine · 2020-09-17T12:39:14Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #105 updated]
Start Time: 2020-09-18T15:32:16.633+0000
Duration: 9 min 33 sec

ycombinator · 2020-09-17T12:43:46Z

internal/testrunner/runners/system/servicedeployer/compose.go

-	if err != nil {
-		return nil, errors.Wrap(err, "could not create STDOUT file")
-	}
-	service.stdout = outFile


This is how a Docker-based service's STDOUT stream was being written to a log file on the local filesystem. How are we capturing a Docker-based service's logs now?

Related: what would be the syntax in a package's test configuration file to refer to such logs? In the current implementation the syntax is {{ STDOUT }} and {{ STDERR }}.

I considered this as a base assumption for this change - to base only on log file resources, as most apps emit them as two log streams (application, access). I think it simplified the implementation as we don't have to deal with opening and closing file.

In case of stdout, stderr we can "tee" these streams to local files.

I don't insist on such design, just wanted to handle all streams the same way (via filesystem).

Okay, let me try to understand this with the apache Docker container. If you look at elastic/integrations#263, the system test configuration file for the apache/access log dataset (for the current implementation of the system test runner) looks like this:

https://github.com/ycombinator/integrations/blob/124ce5912eaf010352c7054bbc0c5ec3e84664c4/packages/apache/dataset/access/_dev/test/system/config.yml

How do you see this changing in the new implementation (this PR)?

We know in advance that service logs are available in the directory /tmp/service_logs/ in the agent container.

I suppose the variable var.paths can contain /tmp/service_logs/access.log*?

EDIT:
Hmm.. ok, I see your point. We should introduce env variables there to be consistent. WDYT?

EDIT2:
We have LogsFolderLocal which is set to /tmp/service_logs. I think in the end it would be - {{ LogsFolderLocal }}/access.log*

EDIT3:
I triple-checked it. You're right, it's not value we want here :) I will adjust the PR.

Fixed. Additional context properties are exposed: LogsFolderLocal and LogsFolderAgent.

Speaking about apache, it requires following lines to be present in httpd.conf:

ErrorLog logs/error_log CustomLog "logs/access_log" combined

Thanks, so my understanding is that the system test configuration file for the apache/access log dataset would look like:

vars: ~ dataset: vars: paths: - "{{LogsFolderAgent}}/access.log*"

And httpd.conf would be configured as you showed in your last comment above.

But why would a package author just know that logs/access_log from their httpd.conf should be referred to as {{LogsFolderAgent}}/access_log*? I'm guessing this comes from the volume mount definitions that they would define in the docker-compose file for their service? Perhaps this last piece would be clear to me if you can make a PR to elastic/integrations#263 showing the necessary changes.

Thanks, so my understanding is that the system test configuration file for the apache/access log dataset would look like:

Correct.

And httpd.conf would be configured as you showed in your last comment above.

Yes

But why would a package author just know that logs/access_log from their httpd.conf should be referred to as {{LogsFolderAgent}}/access_log*?

Frankly speaking I considered this as part of standard stack setup - the Elastic Agent has a directory bound to service logs of the service under tests (SUT). As the agent is a long running instance and it's booted up first, it needs to create the volume (no the SUT). The local directory with logs can customized on the SUT side:

version: '2.3' services: apache: image: docker.elastic.co/integrations-ci/beats-apache:${SERVICE_VERSION:-2.4.20}-1 ports: - 80 volumes: - ${ELASTIC_PACKAGE_SERVICE_LOGS_DIR}:/usr/local/apache2/logs

To be honest I had in mind to document both {{LogsFolderAgent}} and ${ELASTIC_PACKAGE_SERVICE_LOGS_DIR} in some README or guide.

I'm willing to adjust it if you have an idea on how we can simplify it.

Okay, I understand the complete picture now. Thanks.

It's a bit annoying that these two variables slightly betray the inner workings of how logs get from a service container to the agent container. It would be nice if a user didn't need to know about that implementation detail.

What if we make both variables (from a user's perspective) the same and something more generic, e.g. SERVICE_LOGS_DIR? Then, as a user, it is more intuitive that I'm mapping my service logs folder (/usr/local/apache2/logs) to ${SERVICE_LOGS_DIR} in my Docker Compose file and then reading from the same* folder (- "{{SERVICE_LOGS_DIR}}/access.log*") in my system test config file.

I realize it breaks naming conventions as we're mixing env var naming conventions (snake case) with Go struct field naming conventions (camel case). But IMHO that's a worthy sacrifice for a better UX. Or we could even leave the struct field as camel case but in the context application function, we could first convert all snake case template identifiers in the config file to camel case on the fly, right before applying the context to it.

*Internally we know, of course, that it's not the same folder.

Good call, it's definitely a better user experience.

ycombinator · 2020-09-17T22:37:29Z

internal/testrunner/runners/system/servicedeployer/compose.go

-
+	// Connect service network with stack network (for the purpose of metrics collection)
 	stackNetwork := fmt.Sprintf("%s_default", stack.DockerComposeProjectName)
-	logger.Debugf("attaching service container %s to stack network %s", serviceContainer, stackNetwork)


Maybe bring back the debug logs (here and in other useful places) so we can tell what's going on while troubleshooting?

Fixed, thanks!

ycombinator · 2020-09-17T22:53:08Z

internal/testrunner/runners/system/servicedeployer/context.go

-			// the service are stored on the Agent container's filesystem.
-			Agent string
-		}
-	}


Any particular reason not to keep the same hierarchical structure as before? Just curious if the hierarchy vs. flat is a personal preference or if there's a stronger reason you prefer flat?

I kept it flat, because we've only two configuration options. I adjusted it back so it's again structured.

ycombinator · 2020-09-18T15:38:11Z

internal/testrunner/runners/system/test_config.go

-		return data, errors.Wrap(err, "could not render data with context")
+		return data, errors.Wrap(err, "parsing template body failed")
 	}
+	tmpl.RegisterHelpers(ctxt.Aliases())


ycombinator

LGTM. Very nice — much better than my stream redirection hack 😄 👍 🎉

User volumes for service logs

b91edfd

mtojek requested a review from ycombinator September 17, 2020 12:33

mtojek self-assigned this Sep 17, 2020

ycombinator reviewed Sep 17, 2020

View reviewed changes

mtojek added 2 commits September 17, 2020 16:12

Fixes

369318d

Bring back network connect

aee3341

mtojek requested a review from ycombinator September 17, 2020 20:43

mtojek marked this pull request as ready for review September 17, 2020 20:43

ycombinator reviewed Sep 17, 2020

View reviewed changes

mtojek added 6 commits September 18, 2020 08:37

Address PR comments

05493e0

Merge branch 'master' into 85-use-volume-mounts-for-logs

cb7b4df

WIP

89abff0

Expose env as aliases

e295e09

Fix

c1e73c1

Merge branch 'master' into 85-use-volume-mounts-for-logs

ea9fa86

mtojek requested a review from ycombinator September 18, 2020 15:32

ycombinator reviewed Sep 18, 2020

View reviewed changes

ycombinator approved these changes Sep 18, 2020

View reviewed changes

mtojek merged commit f5d9da1 into elastic:master Sep 18, 2020

Conversation

mtojek commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtojek commented Sep 17, 2020 •

edited

Loading

elasticmachine commented Sep 17, 2020 •

edited

Loading

mtojek Sep 17, 2020 •

edited

Loading

mtojek Sep 17, 2020 •

edited

Loading

ycombinator Sep 18, 2020 •

edited

Loading

ycombinator Sep 17, 2020 •

edited

Loading