Alon Bar-Lev has posted comments on this change.
Change subject: [WORK-IN-PROGRESS] packaging: Add backup and restore utility
......................................................................
Patch Set 5: (19 inline comments)
Hi!
Initial comments.
Thanks!
....................................................
File packaging/bin/engine-backup.sh
Line 16: # limitations under the License.
Line 17: #
Line 18:
Line 19: # Globals
Line 20: PGDUMP="/usr/bin/pg_dump"
not sure why you are using full path.
Line 21: PSQL="/usr/bin/psql"
Line 22: TAR="/usr/bin/tar"
Line 23: TAR_COMPRESS_FLAGS="czf"
Line 24: TAR_DECOMPRESS_FLAGS="xf"
Line 18:
Line 19: # Globals
Line 20: PGDUMP="/usr/bin/pg_dump"
Line 21: PSQL="/usr/bin/psql"
Line 22: TAR="/usr/bin/tar"
for standard utilities?
Line 23: TAR_COMPRESS_FLAGS="czf"
Line 24: TAR_DECOMPRESS_FLAGS="xf"
Line 25: DBCONFIG="/etc/ovirt-engine/engine.conf.d/10-setup-database.conf"
Line 26: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
Line 19: # Globals
Line 20: PGDUMP="/usr/bin/pg_dump"
Line 21: PSQL="/usr/bin/psql"
Line 22: TAR="/usr/bin/tar"
Line 23: TAR_COMPRESS_FLAGS="czf"
standard flags as constants not highly acceptable.
Line 24: TAR_DECOMPRESS_FLAGS="xf"
Line 25: DBCONFIG="/etc/ovirt-engine/engine.conf.d/10-setup-database.conf"
Line 26: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
Line 27: MYPGPASS=""
Line 21: PSQL="/usr/bin/psql"
Line 22: TAR="/usr/bin/tar"
Line 23: TAR_COMPRESS_FLAGS="czf"
Line 24: TAR_DECOMPRESS_FLAGS="xf"
Line 25: DBCONFIG="/etc/ovirt-engine/engine.conf.d/10-setup-database.conf"
this is incorrect, please use the epilogue script to read configuration into
environment.
Line 26: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
Line 27: MYPGPASS=""
Line 28: TARBUILD_FOLDER=""
Line 29: TARBALL="/tmp/engine-backup_$(uname -n)_$(date +%Y_%m_%d_%H_%M_%S).tgz"
Line 25: DBCONFIG="/etc/ovirt-engine/engine.conf.d/10-setup-database.conf"
Line 26: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
Line 27: MYPGPASS=""
Line 28: TARBUILD_FOLDER=""
Line 29: TARBALL="/tmp/engine-backup_$(uname -n)_$(date +%Y_%m_%d_%H_%M_%S).tgz"
Please don't put underscores... plain timestamp, please use .tar.gz and not tgz.
engine-backup-$(uname -n)-$(date +%Y%m%d%H%M%S).tar.gz
However, I am not sure uname is good, may present wrong name.
Line 30: BACKUPS_DIR="/var/lib/ovirt-engine/backups/"
Line 31: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 32:
Line 33: cleanup() {
Line 33: cleanup() {
Line 34: [ -n "${MYPGPASS}" ] && rm -f "${MYPGPASS}"
Line 35:
Line 36: # TODO: Safely run rm -rf?
Line 37: [ -n "${TARBUILD_FOLDER}" ] && rm -rf "${TARBUILD_FOLDER}"
I would have single tmp directory for script, within the pgpass and the tarball
root.
This way you always clean one, even if in future you need more temp files.
Line 38: }
Line 39:
Line 40: trap cleanup 0
Line 41:
Line 40: trap cleanup 0
Line 41:
Line 42:
Line 43: die() {
Line 44: local m="$1"
Decide tabs or spaces.
I strongly prefer tabs.
Line 45: echo "FATAL: ${m}" >&2
Line 46: exit 1
Line 47: }
Line 48:
Line 49: usage() {
Line 50: cat << __EOF__
Line 51: engine-backup: backup and restore ovirt-engine
Line 52: USAGE:
Line 53: engine-bacup [-b | --backup] [-r | --restore] [-d | --dbonly]
[-f | --file=/path/to/bacup/or/restore/tarball]
--command=backup or --command=restore?
--database-only instead -f --dbonly? better... --scope=all --scope=database,
this will enable you to add more in future without changing command.
Please don't support short parameters.... it is ugly and waste logic.
Line 54: Where:
Line 55: -b, --backup perform backup
Line 56: -r, --restore perform restore
Line 57: -d, --dbonly handle only DB, ignore other configuration
Line 64: BACKUP=
Line 65: RESTORE=
Line 66: DBONLY=
Line 67:
Line 68: parseArgs() {
please don't support short parameters.
Line 69: while [ -n "$1" ]; do
Line 70: local x="$1"
Line 71: local v="${x#*=}"
Line 72: shift
Line 110: }
Line 111:
Line 112: verifyArgs() {
Line 113: # Verify backup OR restore specified
Line 114: if [ -z "${BACKUP}" ] ; then
why not have mode=backup or ode=restore?
Line 115: [ -n "${RESTORE}" ] || die "-b or -r is missing"
Line 116: fi
Line 117:
Line 118: # Verify a TARBALL has been given
Line 122: }
Line 123:
Line 124: doBackup() {
Line 125: # Create temporart folder
Line 126: TARBUILD_FOLDER=$(mktemp -d)
create the temp at initialization, you need it anyway.
Line 127:
Line 128: # Iterate folders and copy them
Line 129: for folder in ${BACKUP_FOLDERS}; do
Line 130: dirname=$(dirname "${folder}")
Line 125: # Create temporart folder
Line 126: TARBUILD_FOLDER=$(mktemp -d)
Line 127:
Line 128: # Iterate folders and copy them
Line 129: for folder in ${BACKUP_FOLDERS}; do
local folder above
if BACKUP_FOLDERS have space it won't work...
better to have:
BACKUP_FOLDERS="folder1
folder2"
cat "${BACKUP_FOLDERS}" | while read folder; do
done
Line 130: dirname=$(dirname "${folder}")
Line 131: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
Line 132: cp -R "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Line 133: done
Line 128: # Iterate folders and copy them
Line 129: for folder in ${BACKUP_FOLDERS}; do
Line 130: dirname=$(dirname "${folder}")
Line 131: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
Line 132: cp -R "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Wont this work?
cp -a "${folder}" "${TARBALL_FOLDER}"
Line 133: done
Line 134:
Line 135: # Backup database
Line 136: generatePgPass
Line 132: cp -R "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Line 133: done
Line 134:
Line 135: # Backup database
Line 136: generatePgPass
Do this at initialization, you need it anyway.
Line 137: backupDB "${TARBUILD_FOLDER}"/"${BACKUPS_DIR}"
Line 138:
Line 139: # Pack everything
Line 140: createTarball
Line 133: done
Line 134:
Line 135: # Backup database
Line 136: generatePgPass
Line 137: backupDB "${TARBUILD_FOLDER}"/"${BACKUPS_DIR}"
should be:
backupDB "${TARBUILD_FOLDER}/${BACKUPS_DIR}"
Line 138:
Line 139: # Pack everything
Line 140: createTarball
Line 141: }
Line 145: "${TAR_COMPRESS_FLAGS}" \
Line 146: "${TARBALL}" \
Line 147: -C "${TARBUILD_FOLDER}" \
Line 148: .
Line 149: )
No need for these extra constants...
createTarball() {
local dir="$1"
local file="$2"
tar -C "${dir}" -cjvf "${file}" .
}
Line 150:
Line 151: }
Line 152:
Line 153: backupDB() {
Line 164: -h "${ENGINE_DB_HOST}" \
Line 165: -p "${ENGINE_DB_PORT}" \
Line 166: -f "${outfolder}"/"${DB_BACKUP_FILE_NAME}" \
Line 167: "${ENGINE_DB_DATABASE}"
Line 168: )
Why the embedded () ?
backupDB() {
local outfolder="$1"
mkdir -p "${outfolder}"
PGPASSFILE=... pgdump ....
}
Line 169: }
Line 170:
Line 171: doRestore() {
Line 172: extractTarball
Line 200: generatePgPass() {
Line 201: if [ -e "${DBCONFIG}" ]; then
Line 202: . "${DBCONFIG}"
Line 203: else
Line 204: echo "ERROR: ${DBCONFIG} doesn't exist"
use die and reverse
[ -e "${DBCONFIG}" ] || die "xxx"
. "${DBCONFIG}
Line 205: exit 1
Line 206: fi
Line 207:
Line 208: MYPGPASS="$(mktemp)"
Line 214:
Line 215: ## Main
Line 216:
Line 217: # do this in function so we do not lose $@
Line 218: parseArgs "$@"
I think you need $@ to avoid dropping spaces.
I usually out here the parser and verify, see the other script.
Line 219: verifyArgs
Line 220:
Line 221: # TODO: Handle DBONLY option
Line 222: # TODO: ovirt-engine service handling
--
To view, visit http://gerrit.ovirt.org/15276
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f6c386a0f48ccd520978193639120999e00cf2a
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Kiril Nesenko <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Ohad Basan <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
