Is `BACKUP_FILENAME_EXPAND` still working for multiple conf files in `/etc/dockervolumebackup/conf.d`

Question

Is `BACKUP_FILENAME_EXPAND` still working for multiple conf files in `/etc/dockervolumebackup/conf.d`

CXwudi opened this issue 10 months ago · 12 comments

Describe the bug
I have 4 different daily backups, each with a name like BACKUP_FILENAME="$BACKUP_PRUNING_PREFIX-%Y-%m-%dT%H-%M-%S.{{ .Extension }}"

Previously this works fine as long as I have BACKUP_FILENAME_EXPAND set to true.

Starting in recent, it no longer works, even with v2.37.3. The backup filename is like -2024-02-16T09-00-00.tar.gz

To Reproduce

First create a compose.redis.yml file like the following:

version: "3"

services:
  redis:
    image: redis:alpine
    container_name: rsshub-redis
    restart: unless-stopped
    volumes:
      - redis-data:/data
    labels:
      - docker-volume-backup.stop-during-backup=rsshub-redis-data

volumes:
  redis-data:
    name: rsshub-redis-data

Then launch that Redis with docker compose -f redis.compose.yml up -d

Then create another compose.yml file in another directory for this docker-voume-backup container

version: '3'

services:
  volume-backup:
    image: offen/docker-volume-backup:latest
    container_name: docker-volume-backup
    env_file:
      - .env 
    volumes:
      - /var/run/docker.sock:/var/run/docker.sock
      - ./config:/etc/dockervolumebackup/conf.d
      - ./backup-files:/archive
      # volumes
      - rsshub-redis-data:/backup/rsshub-redis-data:ro
    restart: always

volumes:
  rsshub-redis-data:
    name: rsshub-redis-data
    external: true

In the same folder, create an .env file as following:

BACKUP_ARCHIVE=/archive
BACKUP_FILENAME_EXPAND=true

In the config directory shown in the compose.yml file, create a new rsshub-redis-data.daily.env confi file with the following content:

BACKUP_CRON_EXPRESSION="0 9 * * *"
BACKUP_SOURCES=/backup/rsshub-redis-data
BACKUP_PRUNING_PREFIX=rsshub-redis-data-daily-backup
BACKUP_FILENAME="$BACKUP_PRUNING_PREFIX-%Y-%m-%dT%H-%M-%S.{{ .Extension }}"
BACKUP_LATEST_SYMLINK="$BACKUP_PRUNING_PREFIX-latest.tar.gz"
BACKUP_RETENTION_DAYS=7
BACKUP_STOP_DURING_BACKUP_LABEL=rsshub-redis-data

You can also modify BACKUP_CRON_EXPRESSION to be one or two minutes away from now

Launch the docker-voume-backup container by running docker compose up -d

Then you should see a wired filename like -2024-....tar.gz in the ./backup-files` folder

Expected behavior
the env var BACKUP_PRUNING_PREFIX from conf files in /etc/dockervolumebackup/conf.d should be properly propagated into the same conf file.

Version (please complete the following information):

Image Version: v2.37.3
Docker Version: 24.0.6
Docker Compose Version (if applicable): v2.21.0

Additional context
v2.36.x works well, so this issue is probably another part of the unfix regression from v2.37.x

Answer 1 · 2024-02-18T15:22:03.000Z

Could you put the contents of the zip file in the issue body?

As for your issue the behavior should be supported by godotenv as per the docs, but apparently there's something wrong. I'll look into it once I find the time.

Answer 2 · 2024-02-18T15:31:16.000Z

I updated the issue with contents from the zip file. Let me know if there is any more to do

Answer 3 · 2024-02-19T18:26:18.000Z

This is pretty tricky. The situation you describe does not work as expected in v2.36.x versions already: each conf.d file was sourced before running a schedule, which already expanded environment variables before the command provided by this image even ran. I.e. in your setup, BACKUP_FILENAME_EXPAND should have no effect, the behavior you intend to have should happen either way you set the value here.

What happens in v2.37.x is that instead of sourceing the env files, os.ExpandEnv is called on the contents of the files before passing them to godotenv. Unfortunately os.ExpandEnv will drop empty previously unset variables which in your case eats the token, preventing godotenv from even expanding it.

An easy fix for your setup would be to not replace empty values, but this is still not the exact same behavior as before. I'll need to think about this for a bit,

Answer 4 · 2024-02-19T20:06:07.000Z

I see, I guess that's why BACKUP_LATEST_SYMLINK="$BACKUP_PRUNING_PREFIX-latest.tar.gz" can also function in v2.36.x, although (I suppose) BACKUP_FILENAME_EXPAND was intended to only work on BACKUP_FILENAME? This leads me to believe that the expansion was previously handled by the OS sourcing?

This brings me to a question: for each conf.d file, does the old behaviour simply source the file into the OS environment variables, create a cron job, and then move on to the next conf.d file and repeat the process? I ask this because, in my configuration, I have multiple conf.d files with overlapping environment variables, and I'm surprised that the environment variables in each file are properly isolated.

Answer 5 · 2024-02-19T20:10:43.000Z

Each file in conf.d would be sourced right before running a schedule which makes them not leak anything to any other invocations, yes. This is the relevant part from the v2.36.x entrypoint script:

  for file in /etc/dockervolumebackup/conf.d/*; do
    source $file
    BACKUP_CRON_EXPRESSION="${BACKUP_CRON_EXPRESSION:-@daily}"
    echo "Appending cron.d entry with expression $BACKUP_CRON_EXPRESSION and configuration file $file"
    (crontab -l; echo "$BACKUP_CRON_EXPRESSION /bin/sh -c 'set -a; source $file; set +a && backup' 2>&1") | crontab -
  done

This behavior is also kept intact in v2.37.x, the only "problem" now is emulating the behavior of source $file when it comes to variable expansion which is very subtle in many places.

Answer 6 · 2024-02-19T20:40:01.000Z

I start believing there is no real way of emulating the established behavor without:

shelling out (I'd prefer not to)
using the sh library https://github.com/mvdan/sh and executing the files

I'll try to use the latter and see where it takes me.

Answer 7 · 2024-02-19T20:51:00.000Z

Yes, I saw your PR and I agree that reinventing the wheel is generally not a good idea.

However, if the situation is becoming more complex, I am perfectly fine with just adding an option such as EXPAND_PRUNING_PREFIX_TO_FILENAME, which would simply expand the pruning prefix to the backup filename, similar to BACKUP_FILENAME="${BACKUP_PRUNING_PREFIX}${BACKUP_FILENAME}". I just feel sad to see that you want to use a well maintained library like godotenv but end up not using it due to a very corner requirement/behaviour

Answer 8 · 2024-02-19T21:00:21.000Z

I just feel sad to see that you want to use a good library like godotenv but end up not using it due to a very corner requirement/behaviour

I feel that :) but I would think as long as this tool is expected to be continued as a v2 it will need to keep supporting the existing behavior, no matter how it's implemented.

If a v3 is ever cut I would think there's a lot of room for improvement in the entire area (e.g. default to expanding env vars everywhere instead of piece-meal settings like BACKUP_FILENAME_EXPAND), but that requires quite some work and will also open up more opportunities that I currently have no time for. So I would argue it's better to stick to supporting your corner case as it already was supported before right now, and if there ever is a v3 this can be revisited.

You're not the only person with this requirement btw: #321

Answer 9 · 2024-02-20T04:57:31.000Z

Hi, I'm back with an update on the workaround for this issue. For anyone seeing this issue, the idea of the workaround is quite straightforward: prefer to use environment variable substitution at the OS level. In this case, the OS level refers to the Linux OS within the Docker container/image.

More specifically, you should create an additional environment file like default.env, where you can define your common environment variable strings. These strings can then be utilized across multiple cron jobs:

# default.env
# volume names
RSSHUB_REDIS_DATA_NAME=rsshub-redis-data

# cron expression
DAILY_CRON_EXPRESSION="0 9 * * *"

# backup filename prefix
RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX=${RSSHUB_REDIS_DATA_NAME}-daily-backup

# backup retention
DAILY_BACKUP_RETENTION_IN_DAYS=7

Next, you'll need to update the Docker Compose file to include this new default.env file:

services:
  volume-backup:
    image: offen/docker-volume-backup
    container_name: docker-volume-backup
    env_file:
      - default.env # it's recommended to place this file first in the env_file list
      - .env
    # ... more

Then, within any conf.d file, you can employ the variables declared in the default.env file:

BACKUP_CRON_EXPRESSION=${DAILY_CRON_EXPRESSION}
BACKUP_SOURCES=/backup/${RSSHUB_REDIS_DATA_NAME}
BACKUP_PRUNING_PREFIX=${RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX}
BACKUP_FILENAME="${RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX}-%Y-%m-%dT%H-%M-%S.{{ .Extension }}"
BACKUP_LATEST_SYMLINK="${RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX}-latest.tar.gz"
BACKUP_RETENTION_DAYS=${DAILY_BACKUP_RETENTION_IN_DAYS}
BACKUP_STOP_DURING_BACKUP_LABEL=${RSSHUB_REDIS_DATA_NAME}

I have verified that this method works for both v2.37.x and v2.36.x versions. This is because the substitution of RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX occurs at the OS level, effectively RSSHUB_REDIS_DATA_BACKUP_FILENAME_DAILY_PREFIX=rsshub-redis-data-daily-backup is inserted into the container's environment variables before the code initiates. Then, any implementation that supports substituting ${} with OS-level environment variables (which are inserted from env_file in Docker Compose) will function properly. This is supported by both the shell source command and godotenv. And eventually this method set BACKUP_FILENAME properly.

Now, @m90, do you believe it's still necessary to address this issue or to add a "How To" guide with the content mentioned here in the documentation?

Answer 10 · 2024-02-20T08:36:47.000Z

Thanks for sharing your workaround.

I do agree just using godotenv might be the better option in theory, but I made the mistake of using source a long time ago already, so I feel I should keep supportin this in any v2 version. From what I understand this should also not conflict with your workaround, so you might just keep using that.

Answer 11 · 2024-02-22T16:59:30.000Z

This should now be working as before in v2.38.0, but it's also not affecting your workaround, so there should be no reason not to keep it if that is what you want to do.

Answer 12 · 2024-02-23T05:25:08.000Z

Although I already switched to use the workaround, still I am really appreciated for your efforts on fixing it. I may consider reuse BACKUP_FILENAME="$BACKUP_PRUNING_PREFIX-... since it is quite convenience