electricitymaps / brick Goto Github PK

Instead of using our own (slow) implementation to collect specific output files after a build, we should use the --output flag from docker build.
See https://docs.docker.com/engine/reference/commandline/build/
Double check that it supports multiple output directories.

This could be useful to cache builds even further.
For example, a first prepare step could install apt packages (almost never modified), another could install python requirements (sometimes modified)

Definition of problem

When the CI runs the test or deploy steps, it also runs previous steps (prepare + build). These previous steps are potentially redundant if you are running separate brick commands in CI like we do.

However, depending on the setup, those prepare/builds might already have been run as part of the CI build stage!

Although the operation results in a no-op, it still takes time and when the amount of packages increase this slows down CI.

It would be great to avoid running the prepare/build/test phase of brick when running in CI.

Example output from the "test" stage in the CI:

We see that the prepare/build operations are also running here, which enforces consistency.

Proposed solution

Add an option for skipping previous steps (prepare + build) when running in CI.

Things to be aware of:

• when running brick test directly (e.g. locally), it should still run prepare+build to ensure using latest build
• naming of the flag/option - is "ci" too broad? Would love some inputs :)

Suggestion

Add a flag that can be used when called from CI (fx brick -r --ci test)

The way docker_build() is currently checking if the dependencies have been updated, the ability to skip builds with no changes does not work on very first run.

This seems to be because we're adding the branch as a new tag, then checking if this new tag is part of the existing images - which it will never be the first time it's running.

Is there anyway where we can avoid this?

Here's an example (see link below for full version):

🔨 Preparing functions/mailchimp-proxy..

Tags: ['functions_mailchimp-proxy_prepare:latest', 'functions_mailchimp-proxy_prepare:mn-test-0828']
Images: ['functions_mailchimp-proxy_prepare:fbarl-3374-generalize-activity-badge', 'functions_mailchimp-proxy_prepare:latest', 'functions_mailchimp-proxy_prepare:master', ...]

images_are_up_to_date (set(tags).issubset(set(images))): False

See build #5514 in Drone

Hi!

I just ran the updated brick, and I had an issue with a missing dependency. I did not have sha1sum installed, which led to most brick commands failing. Perhaps we could install this automatically when installing brick, or add it to the README?

This is the line that throws the error
https://github.com/tmrowco/brick/blob/master/brick/lib.py#L89

• add build dependencies (#14)
• support a case where docker-compose tests are used (#509)
• support gcloud deploys (https://github.com/tmrowco/tmrowapp/pull/508)
• support integrations (docker-compose) tests (https://github.com/tmrowco/electricitymap/issues/1529)
• support deploy case
• cache builds depending on branch
• cache/mount previous node_modules / yarn cache (#17)
• support watch case
• allow concurrent builds (by having a different .brickdockerfile) (https://github.com/tmrowco/brick/issues/new)
• support a case where Dockerfile are used
• remove name and only use tags / folder names? (#59)

This should help users debug caching of build steps.

Maybe comparing the SHA of the image before & after is a good way?

Origin #3

TBD

We recently introduced a change to not not tag images on non-main branches with "latest". This avoids cache breaking as all branches will not all push the "latest" image.

At least locally we shouldn't do this as the latest tag is usually used for local development in Docker compose files.

Ideas:

1. I think most CIs adds a CI=1 environment variable (we could use that)
2. Or be explicit with an additional argument (but might be confusing)

• Add brick develop
• Bases itself on prepare stage by running a command (typically a watch command)
• Add port mappings in config
• Automatically maps volumes declared as inputs

Would delete images + volumes + containers that are not tagged "master" and that are older than X
Would fix https://github.com/tmrowco/electricitymap/issues/1588

The caching mechanism checks for file inputs, and might abort a docker build if the inputs haven't changed.
However, other informations might invalidate the build, such as e.g. an environment variable.
The dependency hash should therefore include all information (and not only paths):
https://github.com/tmrowco/brick/blob/master/brick/dockerlib.py#L54-L79

Secrets can only be <500kb in size so we should exclude some folders.
Excluding the logs folder from the root of secrets seems like a simple thing to do.

Hey, I just realised that the --skip-previous-steps isn't working with recursive :(

So I guess we have to pass it down to the ctx.invoke() function, but I'm not quite sure what's the best approach for doing that - should I define a new "@click.argument()" for build, test, deploy functions?

Any inputs on how to do this, @corradio?

Seems like it's broken again for some reason..
Haven't dived too deep into it, but as far as I can tell, it finds images with same dependency-hash yet still builds all packages :(

See example from CI here: https://drone.tmrow.com/tmrowco/bloom/107/1/3

When running brick build on my local Linux machine, I can't seem to get the docker build steps output that seem to be always present in the Drone builds.

Here is an example of what I get running with the latest master brick build and the --verbose flag:

fbarl@fbarl-XPS-15-9550 ~/Projects/tmrowco/tmrow/lighthouse.tmrow.com $ brick --verbose build
2020-05-29 15:28:50,288 [DEBUG] Using yarn cache located at /usr/local/share/.cache/yarn/v6
2020-05-29 15:28:50,288 [INFO] 🔨 Preparing lighthouse.tmrow.com..
2020-05-29 15:28:50,293 [DEBUG] docker build . --iidfile /tmp/tmpipjw7itq -f /home/fbarl/Projects/tmrowco/tmrow/.brickdockerfile
2020-05-29 15:28:53,354 [DEBUG] Tagging as lighthouse.tmrow.com_prepare:latest..
2020-05-29 15:28:53,387 [DEBUG] Tagging as lighthouse.tmrow.com_prepare:fbarl-3135-infer-json-schema-from-realm..
2020-05-29 15:28:53,408 [INFO] 💯 Preparation phase done!
2020-05-29 15:28:53,408 [INFO] 🔨 Building lighthouse.tmrow.com..
2020-05-29 15:28:53,452 [DEBUG] docker build . --iidfile /tmp/tmptdmbykri -f /home/fbarl/Projects/tmrowco/tmrow/.brickdockerfile
2020-05-29 15:30:01,694 [DEBUG] rpc error: code = Unknown desc = executor failed running [/bin/sh -c yarn build]: exit code: 1
2020-05-29 15:30:01,699 [ERROR] docker build . --iidfile /tmp/tmptdmbykri -f /home/fbarl/Projects/tmrowco/tmrow/.brickdockerfile
rpc error: code = Unknown desc = executor failed running [/bin/sh -c yarn build]: exit code: 1

@madsnedergaard seems to get the full steps output when running on his machine so it might depend on the running environment, here's my Docker env:

fbarl@fbarl-XPS-15-9550 ~ $ docker version
Client:
 Version:           18.09.7
 API version:       1.39
 Go version:        go1.10.4
 Git commit:        2d0083d
 Built:             Fri Aug 16 14:19:38 2019
 OS/Arch:           linux/amd64
 Experimental:      false

Server:
 Engine:
  Version:          18.09.7
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.10.4
  Git commit:       2d0083d
  Built:            Thu Aug 15 15:12:41 2019
  OS/Arch:          linux/amd64
  Experimental:     false

p.s. I wonder if passing the digest stream straight to stdout in here could be a viable solution 😛

Use case: Allow brick to push the image to a registry
Command: brick push
What it should do: simply push the image to a registry using https://docker-py.readthedocs.io/en/stable/images.html#docker.models.images.ImageCollection.push

cc @FelixDQ

Steps to reproduce:

• brick build
• do a new commit
• brick build
• git revert that commit
• brick build
  Expected:
  The first brick build should have produced a cache entry, and thus the last brick build (after reverting) should re-use that cache and cause a no-op

We should match input paths and detect if they intersect a BUILD.yaml + associated output. In this case we should build the associated target first.

Following error I get with running brick build cmd. Beforehand I tried clearing everything in docker with docker system prune -a

Stack trace:

david@Davids-MBP backend % brick build        
2019-12-17 09:52:18,813 [INFO] 🔨 Preparing backend..
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/docker/api/client.py", line 256, in _raise_for_status
    response.raise_for_status()
  File "/usr/local/lib/python3.7/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http+docker://localhost/v1.35/images/sha256:4486daa9b9cf7e43bb8edc2987119249280e6dcf936dbd8b1469471102e306c7/tag?tag=df-%231356-electricity-input-dates&repo=backend_prepare&force=0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/brick", line 8, in <module>
    sys.exit(entrypoint())
  File "/usr/local/lib/python3.7/site-packages/brick/__main__.py", line 445, in entrypoint
    cli()  # pylint: disable=no-value-for-parameter
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/brick/__main__.py", line 266, in build
    digest = ctx.invoke(prepare, target=target)
  File "/usr/local/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/brick/__main__.py", line 249, in prepare
    dockerfile_contents=dockerfile_contents)
  File "/usr/local/lib/python3.7/site-packages/brick/__main__.py", line 143, in docker_build
    tag=version)
  File "/usr/local/lib/python3.7/site-packages/docker/models/images.py", line 122, in tag
    return self.client.api.tag(self.id, repository, tag=tag, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/docker/utils/decorators.py", line 19, in wrapped
    return f(self, resource_id, *args, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/docker/api/image.py", line 533, in tag
    self._raise_for_status(res)
  File "/usr/local/lib/python3.7/site-packages/docker/api/client.py", line 258, in _raise_for_status
    raise create_api_error_from_http_exception(e)
  File "/usr/local/lib/python3.7/site-packages/docker/errors.py", line 31, in create_api_error_from_http_exception
    raise cls(e, response=response, explanation=explanation)
docker.errors.APIError: 500 Server Error: Internal Server Error ("invalid tag format")

Output of docker py client:

{
  "ID": "QOKV:D5C3:5VOQ:JUQ7:DJ7M:ROSE:JZ3G:LYCS:NKUX:BZUU:WNHP:TOKZ",
  "Containers": 0,
  "ContainersRunning": 0,
  "ContainersPaused": 0,
  "ContainersStopped": 0,
  "Images": 1,
  "Driver": "overlay2",
  "DriverStatus": [
    [
      "Backing Filesystem",
      "extfs"
    ],
    [
      "Supports d_type",
      "true"
    ],
    [
      "Native Overlay Diff",
      "true"
    ]
  ],
  "SystemStatus": null,
  "Plugins": {
    "Volume": [
      "local"
    ],
    "Network": [
      "bridge",
      "host",
      "ipvlan",
      "macvlan",
      "null",
      "overlay"
    ],
    "Authorization": null,
    "Log": [
      "awslogs",
      "fluentd",
      "gcplogs",
      "gelf",
      "journald",
      "json-file",
      "local",
      "logentries",
      "splunk",
      "syslog"
    ]
  },
  "MemoryLimit": true,
  "SwapLimit": true,
  "KernelMemory": true,
  "KernelMemoryTCP": true,
  "CpuCfsPeriod": true,
  "CpuCfsQuota": true,
  "CPUShares": true,
  "CPUSet": true,
  "PidsLimit": true,
  "IPv4Forwarding": true,
  "BridgeNfIptables": true,
  "BridgeNfIp6tables": true,
  "Debug": true,
  "NFd": 29,
  "OomKillDisable": true,
  "NGoroutines": 45,
  "SystemTime": "2019-12-17T09:30:08.44687291Z",
  "LoggingDriver": "json-file",
  "CgroupDriver": "cgroupfs",
  "NEventsListener": 2,
  "KernelVersion": "4.9.184-linuxkit",
  "OperatingSystem": "Docker Desktop",
  "OSType": "linux",
  "Architecture": "x86_64",
  "IndexServerAddress": "https://index.docker.io/v1/",
  "RegistryConfig": {
    "AllowNondistributableArtifactsCIDRs": [],
    "AllowNondistributableArtifactsHostnames": [],
    "InsecureRegistryCIDRs": [
      "127.0.0.0/8"
    ],
    "IndexConfigs": {
      "docker.io": {
        "Name": "docker.io",
        "Mirrors": [],
        "Secure": true,
        "Official": true
      }
    },
    "Mirrors": []
  },
  "NCPU": 6,
  "MemTotal": 2095673344,
  "GenericResources": null,
  "DockerRootDir": "/var/lib/docker",
  "HttpProxy": "gateway.docker.internal:3128",
  "HttpsProxy": "gateway.docker.internal:3129",
  "NoProxy": "",
  "Name": "docker-desktop",
  "Labels": [],
  "ExperimentalBuild": false,
  "ServerVersion": "19.03.4",
  "ClusterStore": "",
  "ClusterAdvertise": "",
  "Runtimes": {
    "runc": {
      "path": "runc"
    }
  },
  "DefaultRuntime": "runc",
  "Swarm": {
    "NodeID": "",
    "NodeAddr": "",
    "LocalNodeState": "inactive",
    "ControlAvailable": false,
    "Error": "",
    "RemoteManagers": null
  },
  "LiveRestoreEnabled": false,
  "Isolation": "",
  "InitBinary": "docker-init",
  "ContainerdCommit": {
    "ID": "b34a5c8af56e510852c35414db4c1f4fa6172339",
    "Expected": "b34a5c8af56e510852c35414db4c1f4fa6172339"
  },
  "RuncCommit": {
    "ID": "3e425f80a8c931f88e6d94a8c831b9d5aa481657",
    "Expected": "3e425f80a8c931f88e6d94a8c831b9d5aa481657"
  },
  "InitCommit": {
    "ID": "fec3683",
    "Expected": "fec3683"
  },
  "SecurityOptions": [
    "name=seccomp,profile=default"
  ],
  "ProductLicense": "Community Engine",
  "Warnings": null
}

We could make sure the CLI autoupdates:
https://bitbucket.org/jorkar/autoupgrade/src/master/
Some more links:

• https://circleci.com/blog/continuously-deploying-python-packages-to-pypi-with-circleci
• https://packaging.python.org/tutorials/packaging-projects/

We could enable current builds by having multiple brickdockerfile files and an overview of dependencies.

Note that it will complicate the code and make debugging harder.

To reproduce:

1. add an BRICK_ env variable as part of a command in the build step
2. run the prepare step without the env variable (brick prepare)
3. brick will crash saying that the variable isn't defined

Expected behaviour:
Prepare step should run fine.

Ideally, we should only show "building X" when the step actually is being built (and not cached)

Similar to how we use a shared yarn cache, we could utilize the Poetry or pip cache.

Instead of relying on image name, we would maybe do something even simpler. The only concern would be binary dependencies, which we do not want to share across machine architecture.

Seems like there's an issue in the "collecting outputs" step. We occasionally see this error:

docker.errors.NotFound: 404 Client Error: Not Found ("No such container: 302cbd094843d307e53682b4d9a3d2e551a5aebb676c95256325117542b3afdd")

This could be related: docker/docker-py#1813

Looking through different brick BUILD.yaml files, I noticed that in some steps command is used while in some others there is a list of commands. I assumed the two are actually interchangeable and that both can be used across build, test, develop etc, only to end up trying to figure why test -> command config is always passing the tests - turns out the commands list being empty means no command have been run so it always passes.

Looking at the code, it seems that only develop uses a singular command while all the other steps require commands. I have a few suggestions how we could improve the behaviour:

1. Switch to commands syntax in develop and completely drop the command option
2. Support both command and commands options across all steps
3. Keep things as they are but document the examples better to make it clear develop steps are an exception in the sense that only there exactly one command is expected

See https://github.com/tmrowco/tmrow/issues/3236 for more context.

When running brick build for the Bloom app, I ended up with a public/ directory created by root user (which is the default Docker context) that I didn't have write access to from my normal non-root Linux account.

Consequently, trying to run yarn start after the build would crash with EACCES: permission denied as that command expects write access to public/. I see this is a minor bug / design flaw that I actually trip over quite often making it a nuisance.

Looking at the code, I can see 3 approaches for resolving this issue:

1. Make the docker_client.containers.run command run with the context of the caller user
2. Switch to get_archive command as the comment in the code suggests
3. Do a chmod on all the output dirs after they're created to make sure non-root users also have write privileges to them

• brick -r prune
  --
  372 | 2020-06-09 01:03:09,556 [INFO] Found 35 target(s)..
  373 | Traceback (most recent call last):
  374 | File "/usr/local/bin/brick", line 11, in
  375 | load_entry_point('brick==0.0.1', 'console_scripts', 'brick')()
  376 | File "/usr/local/lib/python3.6/dist-packages/brick/main.py", line 480, in entrypoint
  377 | cli() # pylint: disable=no-value-for-parameter
  378 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 764, in call
  379 | return self.main(*args, **kwargs)
  380 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 717, in main
  381 | rv = self.invoke(ctx)
  382 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 1137, in invoke
  383 | return _process_result(sub_ctx.command.invoke(sub_ctx))
  384 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 956, in invoke
  385 | return ctx.invoke(self.callback, **ctx.params)
  386 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 555, in invoke
  387 | return callback(*args, **kwargs)
  388 | File "/usr/local/lib/python3.6/dist-packages/click/decorators.py", line 17, in new_func
  389 | return f(get_current_context(), *args, **kwargs)
  390 | File "/usr/local/lib/python3.6/dist-packages/brick/main.py", line 457, in prune
  391 | if check_recursive(ctx, target, prune):
  392 | File "/usr/local/lib/python3.6/dist-packages/brick/main.py", line 139, in check_recursive
  393 | ctx.invoke(fun, target=recursive_target, skip_previous_steps=ctx.parent.params.get('skip_previous_steps'))
  394 | File "/usr/local/lib/python3.6/dist-packages/click/core.py", line 555, in invoke
  395 | return callback(*args, **kwargs)
  396 | File "/usr/local/lib/python3.6/dist-packages/click/decorators.py", line 17, in new_func
  397 | return f(get_current_context(), *args, **kwargs)
  398 | TypeError: prune() got an unexpected keyword argument 'skip_previous_steps'

cc @madsnedergaard

Requested:

• Support to discover all subfolders using -r flag