Skip to content

Enhance rotatelogs-compress.sh script, fix for symlinked log directories #385

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.

Sign up for GitHub

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

Open
pastakhov wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enhance rotatelogs-compress.sh script, fix for symlinked log directories #385

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 47 additions & 15 deletions _sources/scripts/rotatelogs-compress.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,48 @@
#!/bin/bash
# Enable extended globbing for +([0-9]) pattern.
shopt -s extglob

# Returns common prefix of two strings
common_prefix() {
local n=0
while [[ "${1:n:1}" == "${2:n:1}" ]]; do
# Compare strings char-by-char and stop at first mismatch/end.
while [[ -n "${1:n:1}" && "${1:n:1}" == "${2:n:1}" ]]; do
((n++))
done
echo "${1:0:n}"
}

new_log_file="${1}"
file_to_compress="${2}"
commonFilePrefix=""
if [ -n "$new_log_file" ] && [ -n "$file_to_compress" ] && [ "$new_log_file" != "$file_to_compress" ]; then
new_log_file_basename=$(basename "$new_log_file")
file_to_compress_basename=$(basename "$file_to_compress")
commonFilePrefix=$(common_prefix "$new_log_file_basename" "$file_to_compress_basename" | sed 's/[0-9]*$//')
new_log_file_basename=$(basename -- "$new_log_file")
file_to_compress_basename=$(basename -- "$file_to_compress")
commonFilePrefix=$(common_prefix "$new_log_file_basename" "$file_to_compress_basename")
# Trim trailing digits
commonFilePrefix="${commonFilePrefix%%+([0-9])}"
fi

# Fallback: derive prefix from file_to_compress basename to avoid empty prefix ("*") in find filters.
if [ -z "$commonFilePrefix" ] && [ -n "$file_to_compress" ]; then

@vedmaka vedmaka Feb 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we fail if the common prefix can not be determined?

It seems there must always be a common prefix as the /rotatelogs-compress.sh hook always gets two arguments as input, and the pattern we use here is a common prefix:

Taqasta/_sources/configs/mediawiki.conf

Line 39 in 2525572

CustomLog "|/usr/bin/rotatelogs -c -f -l -p /rotatelogs-compress.sh -L /var/log/apache2/access_log.current /var/log/apache2/access_log_%Y%m%d 86400" docker

So absence of a common prefix must be an indicator of something not working/configured as expected

file_to_compress_basename=$(basename -- "$file_to_compress")
# Trim trailing digits
commonFilePrefix="${file_to_compress_basename%%+([0-9])}"
fi

compress_exit_code=0

if [[ "${file_to_compress}" ]]; then
# wait random number of seconds before compressing to avoid to compress log files simultaneously (especially for wiki farms)
if [ "$LOG_FILES_COMPRESS_DELAY" -eq 0 ]; then
DELAY=0
else
# Use random delay only when env value is a valid positive integer.
if [[ "${LOG_FILES_COMPRESS_DELAY:-0}" =~ ^[0-9]+$ ]] && [ "${LOG_FILES_COMPRESS_DELAY:-0}" -gt 0 ]; then

@vedmaka vedmaka Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic (both old and new) around LOG_FILES_COMPRESS_DELAY looks confusing to me:

Use random delay only when env value is a valid positive integer.

The default value for the LOG_FILES_COMPRESS_DELAY is 3600

Taqasta/templates/final.Dockerfile

Line 42 in e67e99c

LOG_FILES_COMPRESS_DELAY=3600 \

But the value we set to the ENV does not really make any sense as we use mod division by $RANDOM, instead we run a random delay whether the LOG_FILES_COMPRESS_DELAY is not equal 0, and that's quite confusing

--

I suggest removing the random part altogether and simply use the value of LOG_FILES_COMPRESS_DELAY for the delay

DELAY=$RANDOM
((DELAY %= "$LOG_FILES_COMPRESS_DELAY"))
((DELAY %= LOG_FILES_COMPRESS_DELAY))
else
if [ -n "${LOG_FILES_COMPRESS_DELAY:-}" ] && ! [[ "${LOG_FILES_COMPRESS_DELAY}" =~ ^[0-9]+$ ]]; then
echo "LOG_FILES_COMPRESS_DELAY is not a non-negative integer (${LOG_FILES_COMPRESS_DELAY:-}), using 0."
fi
DELAY=0
fi
echo "Wait for $DELAY seconds before compressing ${file_to_compress}"
sleep "$DELAY"
Expand All @@ -44,14 +62,28 @@ if [[ "${file_to_compress}" ]]; then
echo "File ${file_to_compress} does not exist".
fi

# remove old log files
if [ -n "$LOG_FILES_REMOVE_OLDER_THAN_DAYS" ] && [ "$LOG_FILES_REMOVE_OLDER_THAN_DAYS" != false ]; then
LOG_DIRECTORY=$(dirname "${file_to_compress}")
find "$LOG_DIRECTORY" -type f -mtime "+$LOG_FILES_REMOVE_OLDER_THAN_DAYS" -iname "$commonFilePrefix*" ! -iname ".*" ! -iname "*.current" -exec rm -f {} \;
fi
LOG_DIRECTORY="$(dirname -- "$file_to_compress")"
# Keep exactly one trailing slash (needed for symlink directory handling).
LOG_DIRECTORY="${LOG_DIRECTORY%/}/"

@vedmaka vedmaka Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think dirname -- "$file" may ever return a trailing slash, so this transformation seems unnecessary


# Empty prefix would expand to "*" and affect unrelated files.
if [ -n "$commonFilePrefix" ]; then

@vedmaka vedmaka Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a scenario the prefix could be empty here? Also, see my comment above about failing if the common prefix could not be determined. If we'd like to account for a possible empty prefix (even after we tried a fallback) this check better to be around the same place where we do the fallback rather than here

# remove old log files
if [ -n "${LOG_FILES_REMOVE_OLDER_THAN_DAYS:-}" ] && [ "${LOG_FILES_REMOVE_OLDER_THAN_DAYS}" != false ]; then
# Validate mtime value before passing it to find.
if [[ "${LOG_FILES_REMOVE_OLDER_THAN_DAYS}" =~ ^[0-9]+$ ]]; then

@vedmaka vedmaka Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving all input checks like that to the beginning of the file, and failing the script run if the inputs are invalid, sending the errors to stderr

find "$LOG_DIRECTORY" -type f -mtime "+$LOG_FILES_REMOVE_OLDER_THAN_DAYS" -iname "$commonFilePrefix*" ! -iname ".*" ! -iname "*.current" -exec rm -f {} \;
else
echo "LOG_FILES_REMOVE_OLDER_THAN_DAYS is not a non-negative integer (${LOG_FILES_REMOVE_OLDER_THAN_DAYS}), skipping old log removal."
fi
fi

# compress uncompressed old log files
find "$LOG_DIRECTORY" -type f -mtime "+2" -iname "$commonFilePrefix*" ! -iname ".*" ! -iname "*.current" ! -iname "*.gz" ! -iname "*.zip" -exec tar --gzip --create --remove-files --absolute-names --transform 's/.*\///g' --file {}.tar.gz {} \;
# compress uncompressed old log files
find "$LOG_DIRECTORY" -type f -mtime "+2" -iname "$commonFilePrefix*" ! -iname ".*" ! -iname "*.current" ! -iname "*.gz" ! -iname "*.zip" ! -iname "*.tar" -exec tar --gzip --create --remove-files --absolute-names --transform 's/.*\///g' --file {}.tar.gz {} \;
else
echo "commonFilePrefix is empty, skipping cleanup/compression find operations."
fi
fi

shopt -u extglob
exit ${compress_exit_code}
Loading