Add location library to elastic-package #303

fearful-symmetry · 2021-03-29T22:32:42Z

This is my attempt at "breaking up" #301 into more manageable parts. This is the first part, which is breaking up internal/install to create a locations library.

Right now, internal/install is taking on the role of managing the locations of all the files that elastic-package cares about. In an attempt to limit the duties of install to just installing things, and also to prepare for the internal/profile package that'll come right after this, we're adding a internal/location library that's solely responsible for owning where things are on the filesystem.

The fact that we have multiple methods for accessing some of the file paths is due to internal/install/install.go, which is the only library that cares about numerous file locations throughout ~/.elastic-package, so I decided to give it a provider.

I also chose not to touch internal/install/install.go here, as that file is particularly tied up with the newer profile library, and if I update that in the next PR with internal/profile, I won't have an ugly merge operation on my hands.

elasticmachine · 2021-03-30T00:30:52Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #303 updated
Start Time: 2021-04-06T16:39:09.880+0000
Duration: 21 min 3 sec
Commit: 0b44bdb

Test stats 🧪

Test	Results
Failed	0
Passed	316
Skipped	1
Total	317

Trends 🧪

mtojek

I also chose not to touch internal/install/install.go here, as that file is particularly tied up with the newer profile library, and if I update that in the next PR with internal/profile, I won't have an ugly merge operation on my hands.

I understand your concerns regarding hard merge operation, but unfortunately this is the way how we should perform refactoring operations - it's not just about breaking the bigger PR into pieces. I think you should remove the refactored code as part of this PR.
Please remember that the PR shouldn't just introduce a dead code, but it must provide an added value for the end-user or at least be used by other internals.

internal/locations/locations.go

mtojek · 2021-03-30T11:34:43Z

internal/cleanup/service_logs.go

@@ -8,15 +8,15 @@ import (
 	"github.com/pkg/errors"

 	"github.com/elastic/elastic-package/internal/files"
-	"github.com/elastic/elastic-package/internal/install"
+	"github.com/elastic/elastic-package/internal/locations"


I'm not sure if internal/locations is the correct place for this feature. This is a configuration that's strongly related to app configuration. I would be for placing it all together under internal/configuration, so we may have:
internal/configuration/locations
internal/configuration/profiles
internal/configuration/stack

In this case the goal would be transforming the internal/install into something more sophisticated that can deal with profiles and config extensions.

That makes sense. Should we do that as part of this PR, or later?

As you're adding locations, I think you can adapt it already.

internal/locations/locations.go

…tic-package into locations-refactor

fearful-symmetry · 2021-03-30T15:48:34Z

I think you should remove the refactored code as part of this PR.

I'm not sure what you're referring to @mtojek ? Just keep locations/locations.go and remove the components that're swapped over to it?

internal/locations/locations.go

mtojek · 2021-03-30T15:53:31Z

internal/cleanup/service_logs.go

@@ -8,15 +8,15 @@ import (
 	"github.com/pkg/errors"

 	"github.com/elastic/elastic-package/internal/files"
-	"github.com/elastic/elastic-package/internal/install"
+	"github.com/elastic/elastic-package/internal/locations"


As you're adding locations, I think you can adapt it already.

internal/locations/locations.go

mtojek · 2021-03-30T15:56:48Z

I'm not sure what you're referring to @mtojek ? Just keep locations/locations.go and remove the components that're swapped over to it?

Currently locations.go contains duplicated code from install. My point is to remove it from the old place and adjust all references to use locations.

fearful-symmetry · 2021-03-30T16:35:53Z

@mtojek I moved around locations to a new configuration library. However, I wasn't sure what your thinking was behind putting stack in internal/configuration ? It doesn't seem quite as related to the functionality of profiles and locations

mtojek · 2021-03-30T17:12:34Z

Cool! Please address also other comments and let me know when it's ready for re-review.

fearful-symmetry · 2021-03-30T19:51:36Z

Alright, that should be everything.

ycombinator · 2021-03-30T20:22:25Z

You might need to run make format to make CI happy.

fearful-symmetry · 2021-03-30T20:24:36Z

Figured I was forgetting something...

fearful-symmetry · 2021-03-30T21:25:29Z

@ycombinator can you tell if the CI issues are something I did? I'm trying to replicate it, the development files are there.

ycombinator · 2021-03-30T22:04:24Z

@fearful-symmetry It looks like one of the system tests for the apache.access data set failed. You can see the failure in the Tests tab in Jenkins.

ycombinator · 2021-03-30T22:13:15Z

I tried to reproduce the failure locally by building elastic-package from this PR and then running:

elastic-package stack up -d
$(elastic-package stack shellinit)

cd test/packages/apache
elastic-package test system

And the tests passed. So I'm going to tickle CI on this PR again and see what happens.

ycombinator · 2021-03-30T22:13:23Z

jenkins, run the tests please

ycombinator · 2021-03-30T23:16:39Z

Same test failed again. I'm going to try and dig into it more deeply locally and see if something turns up.

mtojek · 2021-03-31T09:36:27Z

Ok, it seems that it's related to this PR, will try to reproduce it locally.

mtojek · 2021-03-31T11:05:57Z

I think I found the problem, it seems to be related with volume's source.

Take a look at paths. The /tmp/service_logs/ path is mounted in elastic-agent's container:

    streams:
      - id: logfile-apache.access-ca17e851-8464-4100-a450-0733973daf00
        data_stream:
          dataset: apache.access
          type: logs
        paths:
          - /tmp/service_logs/access.log*
        exclude_files:
          - .gz$
        processors:
          - add_fields:
              target: ''
              fields:
                ecs.version: 1.5.0

The elastic-agent's container has the following definition:

  elastic-agent:
    image: ${ELASTIC_AGENT_IMAGE_REF}
    depends_on:
      elasticsearch:
        condition: service_healthy
      kibana:
        condition: service_healthy
    healthcheck:
      test: "sh -c 'grep \"Agent is starting\" /usr/share/elastic-agent/elastic-agent.log*'"
      retries: 30
      interval: 1s
    hostname: docker-fleet-agent
    environment:
    - "FLEET_ENROLL=1"
    - "FLEET_ENROLL_INSECURE=1"
    - "FLEET_INSECURE=1"
    - "FLEET_SETUP=1"
    - "FLEET_URL=http://kibana:5601"
    - "KIBANA_HOST=http://kibana:5601"
    volumes:
    - type: bind
      source: ../tmp/service_logs/
      target: /tmp/service_logs/

The volume's source is relative. I suspect that this path can't be found.

fearful-symmetry · 2021-03-31T15:29:55Z

That's odd. That path makes sense, and nothing in the code that would change it.

fearful-symmetry · 2021-03-31T19:41:58Z

Okay, I'm still baffled by the kibana errors. The path that @mtojek pointed to seems fine, the mount and files all exist and are pointing to the correct location. I can't see anything weird in the kibana install itself.

mtojek · 2021-04-01T07:49:26Z

I think you can ignore these Kibana errors:

Regarding failed tests - I did a short test:

elastic-package stack up -v -d
docker exec -it <agent_container> /bin/bash
ls -la /tmp/service_logs/

The directory exists.

cd ~/go/src/github.com/elastic/integrations/packages/apache
elastic-package test system -v

The mounted directory will disappear, but it should be present and contain log files:

 ls -la /tmp/service_logs/
ls: cannot access /tmp/service_logs/: No such file or directory

fearful-symmetry · 2021-04-01T18:15:56Z

@mtojek I can't seem to reproduce that. elastic-package test system -v fails to run because (I assume) all the kibana auth errors, and the directory still exists after. Could something be deleting the directory after the test?

mtojek · 2021-04-01T18:22:31Z

elastic-package test system -v fails to run because (I assume) all the kibana auth errors, and the directory still exists after.

I meant the directory inside the agent's container (/tmp/service_logs).

fearful-symmetry · 2021-04-01T18:24:42Z

I meant the directory inside the agent's container (/tmp/service_logs).

Yah, that's what I was referring to. I assume whatever is removing that directory isn't being run because the test system command is failing.

mtojek · 2021-04-01T18:31:49Z

The system test runner fails because it expects some access logs to be present in that directory. Unfortunately the entire directory is deleted, but in fact the tool should delete only it's content. You can compare the behavior and paths in your PR and master build.

This can be a reference point: https://github.com/elastic/elastic-package/blob/master/internal/testrunner/runners/system/servicedeployer/compose.go#L55

or this: https://github.com/elastic/elastic-package/blob/master/internal/testrunner/runners/system/servicedeployer/compose.go#L127

EDIT:

Maybe the path which is passed doesn't exist. Sounds to me like something to debug.

fearful-symmetry · 2021-04-01T20:10:22Z

Well, I'm still not sure what's up with the /tmp/service_logs thing. I ran the apache tests, and the logs exist inside the elastic-package-service_apache_1 container at /usr/local/apache2/logs, however, they don't exist inside the elastic-package-stack_elastic-agent_1 at /tmp/service_logs/

Here's what the mounts look like:

            {
                "Type": "bind",
                "Source": "/home/alexk/.elastic-package/tmp/service_logs",
                "Destination": "/usr/local/apache2/logs",
                "Mode": "rw",
                "RW": true,
                "Propagation": "rprivate"
            }

and

            {
                "Type": "bind",
                "Source": "/home/alexk/.elastic-package/tmp/service_logs",
                "Destination": "/tmp/service_logs",
                "Mode": "",
                "RW": true,
                "Propagation": "rprivate"
            }

mtojek · 2021-04-01T20:18:10Z

So you can see that /home/alexk/.elastic-package/tmp/service_logs is the bridge between both containers. Could you verify that before starting the test system but after stack up, you can create a file on your host machine and it's still visible from inside of the agent container?
I assume you can do the same exercise after the test system.

fearful-symmetry · 2021-04-01T20:26:00Z

Yah, something odd is going on inside that directory. Whatever I add from the docker host in /home/alexk/.elastic-package/tmp/service_logs doesn't appear on the container, and from inside the container I can't read or write to /tmp/service_logs, even though the directory itself exists.

I assume this is a config issue with docker-compose somehow, gonna tinker around after I get lunch.

fearful-symmetry · 2021-04-02T17:29:04Z

@mtojek so, I did some more tinkering around, and it seems like the test system is breaking things. The bindmount inside the container works fine before, but once the Apache container starts up, the agent container can't seem to see anything inside the bindmount.

fearful-symmetry · 2021-04-02T18:16:46Z

So, after the second container spins up, the link of the directory seems to get nuked:

before:

 67113534 drwxr-xr-x. 2 elastic-agent elastic-agent   6 Apr  2 18:07 service_logs

After:

 67113534 drwxr-xr-x. 0 elastic-agent elastic-agent   6 Apr  2 18:07 service_logs

fearful-symmetry · 2021-04-02T19:21:12Z

The weird part is I can't reproduce this by manually starting up the docker container for apache:

docker-compose -f _dev/deploy/docker/docker-compose.yml --verbose up

fearful-symmetry · 2021-04-02T19:55:22Z

Okay, think I figured it out. I was passing the wrong directory to the version check, so it was re-installing elastic-package every time.

ycombinator · 2021-04-05T14:45:51Z

internal/configuration/locations/locations.go

+	return filepath.Join(loc.stackPath, serviceLogsDir)
+}
+
+// ConfigurationDir returns the configuration directory location


Suggested change

// ConfigurationDir returns the configuration directory location

// configurationDir returns the configuration directory location

ycombinator

LGTM but please wait for @mtojek's review as well as he was actively reviewing this PR. Thanks!

mtojek

Nice! I think you can proceed with the next PR.

mtojek · 2021-04-06T19:55:58Z

@fearful-symmetry Once this PR is merged could you please open a PR to elastic/integrations to get this change in (update dependency on elastic-package)? We need to be sure that it doesn't affect integrations.

add location library to elastic-package

bd31ebb

fearful-symmetry requested review from ycombinator and mtojek March 29, 2021 22:32

fearful-symmetry self-assigned this Mar 29, 2021

Merge branch 'master' into locations-refactor

a9d08f6

mtojek reviewed Mar 30, 2021

View reviewed changes

fearful-symmetry added 2 commits March 30, 2021 08:43

reove newline

3193627

Merge branch 'locations-refactor' of github.com:fearful-symmetry/elas…

578725e

…tic-package into locations-refactor

Merge remote-tracking branch 'upstream/master' into locations-refactor

f12196b

mtojek reviewed Mar 30, 2021

View reviewed changes

migrate locations, fix formatting

5d90e76

remove old code from install

babf379

fearful-symmetry requested a review from mtojek March 30, 2021 19:51

make fmt

bea000a

fearful-symmetry added 3 commits March 30, 2021 13:35

write config.yml in correct location

fd3fa4d

fix locations path

6e21b0f

fix root paths

a969937

fearful-symmetry added 2 commits March 31, 2021 09:45

remove duplicated functions

12b31ff

Merge remote-tracking branch 'upstream/master' into locations-refactor

4eff1a2

fearful-symmetry requested a review from ycombinator March 31, 2021 16:47

Merge remote-tracking branch 'upstream/master' into locations-refactor

3da8346

Merge remote-tracking branch 'upstream/master' into locations-refactor

4a71a58

fix bug in install check

c89b776

ycombinator reviewed Apr 5, 2021

View reviewed changes

ycombinator approved these changes Apr 5, 2021

View reviewed changes

mtojek approved these changes Apr 6, 2021

View reviewed changes

fearful-symmetry added 2 commits April 6, 2021 09:38

Merge remote-tracking branch 'upstream/master' into locations-refactor

0b44bdb

fix case

4798ca1

fearful-symmetry merged commit f187867 into elastic:master Apr 6, 2021

fearful-symmetry mentioned this pull request Apr 6, 2021

update dependencies for new elastic-package elastic/integrations#888

Merged

1 task

	// ConfigurationDir returns the configuration directory location
	// configurationDir returns the configuration directory location

Add location library to elastic-package #303

Add location library to elastic-package #303

Conversation

fearful-symmetry commented Mar 29, 2021

elasticmachine commented Mar 30, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

mtojek left a comment

Choose a reason for hiding this comment

mtojek Mar 30, 2021

Choose a reason for hiding this comment

fearful-symmetry Mar 30, 2021

Choose a reason for hiding this comment

mtojek Mar 30, 2021

Choose a reason for hiding this comment

fearful-symmetry commented Mar 30, 2021

mtojek Mar 30, 2021

Choose a reason for hiding this comment

mtojek commented Mar 30, 2021

fearful-symmetry commented Mar 30, 2021

mtojek commented Mar 30, 2021

fearful-symmetry commented Mar 30, 2021

ycombinator commented Mar 30, 2021

fearful-symmetry commented Mar 30, 2021

fearful-symmetry commented Mar 30, 2021

ycombinator commented Mar 30, 2021

ycombinator commented Mar 30, 2021

ycombinator commented Mar 30, 2021

ycombinator commented Mar 30, 2021

mtojek commented Mar 31, 2021

mtojek commented Mar 31, 2021

fearful-symmetry commented Mar 31, 2021

fearful-symmetry commented Mar 31, 2021

mtojek commented Apr 1, 2021

fearful-symmetry commented Apr 1, 2021

mtojek commented Apr 1, 2021

fearful-symmetry commented Apr 1, 2021

mtojek commented Apr 1, 2021 • edited Loading

fearful-symmetry commented Apr 1, 2021 • edited Loading

mtojek commented Apr 1, 2021

fearful-symmetry commented Apr 1, 2021 • edited Loading

fearful-symmetry commented Apr 2, 2021

fearful-symmetry commented Apr 2, 2021

fearful-symmetry commented Apr 2, 2021

fearful-symmetry commented Apr 2, 2021

ycombinator Apr 5, 2021

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

mtojek commented Apr 6, 2021

elasticmachine commented Mar 30, 2021 •

edited

Loading

mtojek commented Apr 1, 2021 •

edited

Loading

fearful-symmetry commented Apr 1, 2021 •

edited

Loading

fearful-symmetry commented Apr 1, 2021 •

edited

Loading