Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Docker/Compose #3

Merged
merged 6 commits into from
Jan 29, 2024
Merged

Refactor Docker/Compose #3

merged 6 commits into from
Jan 29, 2024

Conversation

Murazaki
Copy link

@Murazaki Murazaki commented Jan 23, 2024

From @jippi PR in upstream : pixelfed#4844

I had to squash for convenience (110 commits ?) and rebase on latest.

(If there's any concern on using latest staging please reply and tell me why)

@Murazaki
Copy link
Author

@jippi any reason for building with bullseye and 8.1 ?

@jippi
Copy link

jippi commented Jan 25, 2024

it was the baseline for existing Dockerfile, so I just kept it

@Murazaki
Copy link
Author

Murazaki commented Jan 28, 2024

Testing on a standard migration case, my observations:

  • the acquire-lock can block both the app and worker, and can be left locked on kill
  • some ok situations blocks the scripts
worker-1  | [entrypoint / 11-first-time-setup.sh] - 👷 Running [php artisan passport:keys] as [www-data]
worker-1  | [entrypoint / 11-first-time-setup.sh] - (stdout) Encryption keys already exist. Use the --force option to overwrite them.
worker-1  | [entrypoint / 11-first-time-setup.sh] - ERROR - ❌ Error!
  • error on redis update to 7-alpine (password not set error on phpredis side) env variable issue, fixed

Cc @jippi

@Murazaki
Copy link
Author

Murazaki commented Jan 29, 2024

@jippi trying something on lock file but still not satisfactory :

# @description Best effort file lock to ensure *something* is not running in multiple containers.
#   The script uses "trap" to clean up after itself if the script crashes
# @arg $1 string The lock identifier
function acquire-lock() {
    local name="${1:-$script_name}"
    local file="${docker_locks_path}/${name}"
    local lock_fd

    ensure-directory-exists "$(dirname "${file}")"

    exec {lock_fd}>"$file"

    log-info "🔑 Trying to acquire lock: ${file}: "
    while !(flock -n -x "$lock_fd"); do
        log-info "🔒 Waiting on lock ${file}"

        staggered-sleep
    done

    # not needed but does verify ownership
    stream-prefix-command-output touch "${file}"

    log-info "🔐 Lock acquired [${file}]"

    on-trap "release-lock ${name} ${lock_fd}" EXIT INT QUIT TERM
}

# @description Release a lock aquired by [acquire-lock]
# @arg $1 string The lock identifier
function release-lock() {
    local name="${1:-$script_name}"
    local file="${docker_locks_path}/${name}"
    local lock_fd="$2"

    log-info "🔓 Releasing lock [${file}]"

    exec {lock_fd}>&-
    # Not sure if we should delete then
    stream-prefix-command-output rm -fv "${file}"
}

@Murazaki Murazaki merged commit 41170a8 into dev Jan 29, 2024
5 of 6 checks passed
@Murazaki
Copy link
Author

file lock broke shellcheck, but the CI passed. oh well.

@jippi
Copy link

jippi commented Jan 29, 2024

flock does not work at all/well between containers due to how it works

I plan to add a "timeout" to "wait" X time (maybe 60s or something) if it does deadlock (I've only seen this happen when ctrl+c'ing containers that have a lock during startup).

Also considered using Redis as a lock hub, but not loving that a lot

@jippi
Copy link

jippi commented Jan 29, 2024

Also, any issues in 11-first-time-setup.sh is documented in the migration guide :)

@Murazaki
Copy link
Author

Thanks. BTW docs is pushed here too : pixelfed-glitch.github.io/docs

@jippi
Copy link

jippi commented Jan 29, 2024

Cool - I'm only working on the upstream part of it to pixelfed/pixelfed atm :)

@Murazaki
Copy link
Author

flock does not work at all/well between containers due to how it works

I plan to add a "timeout" to "wait" X time (maybe 60s or something) if it does deadlock (I've only seen this happen when ctrl+c'ing containers that have a lock during startup).

Also considered using Redis as a lock hub, but not loving that a lot

This is kinda strange because it seemed like flock was working pretty well from a docker instance to another here, and I don't see conflicts. It is also recommended and tested by other docker devs :

https://stackoverflow.com/a/57189715
nextcloud/docker#1903

@Murazaki Murazaki deleted the jippi-fork branch January 30, 2024 11:27
@Murazaki Murazaki mentioned this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants