search

irods / contrib

A pooled collection of community-contributed code that works alongside iRODS
BSD 3-Clause "New" or "Revised" License
13 stars 19 forks source link

Update elk-stack container for audit plugin refactor and new Dockerfile syntax #31

Closed SwooshyCueb closed 1 year ago

SwooshyCueb commented 2 years ago

README still needs to be updated. Container must be built with BuildKit now. Usually possible with an environment variable or docker buildx

SwooshyCueb commented 2 years ago

Based on what I've read, the RUN --mount command can help improve performance. Are those commands (along with COPY --chmod) the reasons why docker buildx is required?

Yes

The --mount options seems very promising the more I read about it. Are there any other motivating factors behind these changes?

My primary motivation was to reduce intermediate images from RUN chmod and to reduce write cycles (and therefore SSD wear) from re-downloading packages all the time.

Have you seen any performance improvements?

Building the image is faster (at least on my system) by several orders of magnitude.

korydraughn commented 2 years ago

Very nice. Several orders of magnitude is good.

Do you sense any downsides to using buildx? Is it safe to assume everyone running docker has access to buildx?

SwooshyCueb commented 2 years ago

Given that Docker's website says (or said, I haven't checked recently but it's still the conventional wisdom) specifically not to use older distro-provided packages for any serious work, and given that docker has shipped with BuildKit for a few years now anyway, I think it's safe to assume anyone who needs this functionality will have it readily available, if not via docker buildx then via the DOCKER_BUILDKIT environment variable.

If there are any downsides to using BuildKit over the original image builder, I haven't found them.

SwooshyCueb commented 1 year ago

Seems good to me. Are the changes you're making in irods/irods_rule_engine_plugin_audit_amqp#105 required for this to work?

No. It can be used with the pre-105 version of the plugin by specifying the workarounds as described in the readme.

SwooshyCueb commented 1 year ago

I think we're ready to merge this PR