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.

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

Jump to bottom

Move in progress detection to separate file with pid #145

Merged
merged 3 commits into from
Oct 9, 2015
Merged

Move in progress detection to separate file with pid #145

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc97d5a
Move in progress detection to separate file with pid
dbussink Sep 28, 2015
4d6e261
Add test for cleanup process
dbussink Sep 28, 2015
d301fd3
Update wording of when old version backup progress is detected
dbussink Oct 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 34 additions & 15 deletions bin/ghe-backup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,37 +28,56 @@ touch "incomplete"
GHE_MAINTENANCE_MODE_ENABLED=false

# To prevent multiple backup runs happening at the same time, we create a
# in-progress symlink pointing to the snapshot directory. This will fail if
# another backup is already in progress, giving us a form of locking.
# in-progress file with the timestamp and pid of the backup process,
# giving us a form of locking.
#
# Set up a trap to remove the in-progress symlink if we exit for any reason but
# verify that we own the in-progress symlink before doing so.
# Set up a trap to remove the in-progress file if we exit for any reason but
# verify that we are the same process before doing so.
#
# The cleanup trap also handles disabling maintenance mode on the appliance if
# it was automatically enabled.
cleanup () {
if [ "$(readlink ../in-progress)" = "$GHE_SNAPSHOT_TIMESTAMP" ]; then
unlink ../in-progress
if [ -f ../in-progress ]; then
progress=$(cat ../in-progress)
snapshot=$(echo "$progress" | cut -d ' ' -f 1)
pid=$(echo "$progress" | cut -d ' ' -f 2)
if [ "$snapshot" = "$GHE_SNAPSHOT_TIMESTAMP" -a "$$" = $pid ]; then
unlink ../in-progress
fi
fi

if $GHE_MAINTENANCE_MODE_ENABLED; then
ghe-maintenance-mode-disable "$GHE_HOSTNAME"
fi
if $GHE_MAINTENANCE_MODE_ENABLED; then
ghe-maintenance-mode-disable "$GHE_HOSTNAME"
fi
}

# Setup exit traps
trap 'cleanup' EXIT
trap 'exit $?' INT # ^C always terminate

# Mark the snapshot as in-progress by creating the symlink. If this fails, it
# means another ghe-backup run is already in progress and we should exit.
# NOTE: The -n argument to ln is non-POSIX but widely supported.
if ! ln -sn "$GHE_SNAPSHOT_TIMESTAMP" ../in-progress 2>/dev/null; then
snapshot="$(readlink ../in-progress)"
echo "Error: backup of $GHE_HOSTNAME already in progress in snapshot $snapshot. Aborting." 1>&2
if [ -h ../in-progress ]; then
echo "Error: in progress backup from previous version detected." 1>&2
Copy link

Choose a reason for hiding this comment

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

I think we can improve the error message. Was there a reason to drop $GHE_HOSTNAME from it?

How about:

Error: detected a backup of $GHE_HOSTNAME already in progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment seems outdated based on the additional feedback, so going with the last mentioned suggestion.

echo "If there is no backup in progress anymore, please remove" 1>&2
echo "the $GHE_DATA_DIR/in-progress symlink. This is only needed once." 1>&2
Copy link

Choose a reason for hiding this comment

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

It's not clear what is only needed once. Is it removing the file? And it's not a symlink any more.

Copy link

Choose a reason for hiding this comment

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

Ah, I missed that this was testing for a symlink, so "previous version" and "only needed once" make more sense to me now, and why it's tricky to write a good error message.

echo "Error: detected a backup already in progress from a previous version of ghe-backup." 1>&2
echo "If there is no backup in progress anymore, please remove" 1>&2
echo "the $GHE_DATA_DIR/in-progress symlink." 1>&2

I don't think it's necessary to say it's only needed once if we're specific about the version of ghe-backup.

exit 1
fi

if [ -f ../in-progress ]; then
progress=$(cat ../in-progress)
snapshot=$(echo "$progress" | cut -d ' ' -f 1)
pid=$(echo "$progress" | cut -d ' ' -f 2)
if ! ps -p $pid -o command= | grep ghe-backup; then
# We can safely remove in-progress, ghe-prune-snapshots
# will clean up the failed backup.
unlink ../in-progress
else
echo "Error: backup process $pid of $GHE_HOSTNAME already in progress in snapshot $snapshot. Aborting." 1>&2
exit 1
fi
fi

echo "$GHE_SNAPSHOT_TIMESTAMP $$" > ../in-progress

echo "Starting backup of $GHE_HOSTNAME in snapshot $GHE_SNAPSHOT_TIMESTAMP"

# Perform a host connection check and establish the remote appliance version.
Expand Down
14 changes: 13 additions & 1 deletion test/test-ghe-backup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ begin_test "ghe-backup tarball strategy"
)
end_test

begin_test "ghe-backup fails fast when other run in progress"
begin_test "ghe-backup fails fast when old style run in progress"
(
set -e

Expand All @@ -241,6 +241,18 @@ begin_test "ghe-backup fails fast when other run in progress"
)
end_test

begin_test "ghe-backup cleans up stale in-progress file"
(
set -e

echo "20150928T153353 99999" > "$GHE_DATA_DIR/in-progress"
ghe-backup

[ ! -f "$GHE_DATA_DIR/in-progress" ]
)
end_test


begin_test "ghe-backup without manage-password file"
(
set -e
Expand Down