Alon Bar-Lev has posted comments on this change.
Change subject: packaging: Add backup and restore utility
......................................................................
Patch Set 6: (28 inline comments)
....................................................
File packaging/bin/engine-backup.sh
Line 1: #!/bin/sh
tabs?
Line 2: #
Line 3: # ovirt-engine-backup - oVirt engine backup and restore utility
Line 4: # Copyright (C) 2013 Red Hat, Inc.
Line 5: #
Line 19: # Load the prolog:
Line 20: . "$(dirname "$(readlink -f "$0")")"/engine-prolog.sh
Line 21:
Line 22: # Globals
Line 23: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
should be:
BACKUP_FOLDERS="/etc/ovirt-engine
/etc/pki/ovirt-engine"
Line 24: MYPGPASS=""
Line 25: TEMP_FOLDER=""
Line 26: TARBUILD_FOLDER=""
Line 27: TARBALL="/tmp/engine-backup-$(date +%Y_%m_%d_%H_%M_%S).tar.gz"
Line 23: BACKUP_FOLDERS="/etc/ovirt-engine /etc/pki/ovirt-engine"
Line 24: MYPGPASS=""
Line 25: TEMP_FOLDER=""
Line 26: TARBUILD_FOLDER=""
Line 27: TARBALL="/tmp/engine-backup-$(date +%Y_%m_%d_%H_%M_%S).tar.gz"
please remove the _
anyway, why don't we set the file mandatory?
Line 28: BACKUPS_DIR="/var/lib/ovirt-engine/backups/"
Line 29: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 30:
Line 31: cleanup() {
Line 24: MYPGPASS=""
Line 25: TEMP_FOLDER=""
Line 26: TARBUILD_FOLDER=""
Line 27: TARBALL="/tmp/engine-backup-$(date +%Y_%m_%d_%H_%M_%S).tar.gz"
Line 28: BACKUPS_DIR="/var/lib/ovirt-engine/backups/"
no need for that if user specify where backup is.
Line 29: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 30:
Line 31: cleanup() {
Line 32: [ -n "${TARBUILD_FOLDER}" ] && rm -rf "${TEMP_FOLDER}"
Line 33: }
Line 34:
Line 35: trap cleanup 0
Line 36:
Line 37: die() {
you have die in the prologue.
Line 38: local m="$1"
Line 39: echo "FATAL: ${m}" >&2
Line 40: exit 1
Line 41: }
Line 43: usage() {
Line 44: cat << __EOF__
Line 45: engine-backup: backup and restore ovirt-engine
Line 46: USAGE:
Line 47: engine-bacup [--mode=<backup|restore>] [--scope=<all|dbonly>]
[--tarball=/path/to/bacup/or/restore/tarball]
--tarball -> --file= ?
Line 48: Where:
Line 49: --mode= the required mode (backup or restore)
Line 50: --scope= the elements to backup
Line 51: --tarball= the backup tarball to restore
Line 63: local v="${x#*=}"
Line 64: shift
Line 65: case "${x}" in
Line 66: --mode=*)
Line 67: MODE="${v}"
should be:
--mode=*)
case "${mode}" in
backup) BACKUP=1 ;;
restore) RESTORE=1 ;;
*) die("Invalid mde") ;;
esac
;;
Line 68: ;;
Line 69: --scope=*)
Line 70: SCOPE="${v}"
Line 71: ;;
Line 65: case "${x}" in
Line 66: --mode=*)
Line 67: MODE="${v}"
Line 68: ;;
Line 69: --scope=*)
same.
Line 70: SCOPE="${v}"
Line 71: ;;
Line 72: -h|-help|--help)
Line 73: usage
Line 68: ;;
Line 69: --scope=*)
Line 70: SCOPE="${v}"
Line 71: ;;
Line 72: -h|-help|--help)
help should be last.
please accept only --help.
Line 73: usage
Line 74: exit 0
Line 75: ;;
Line 76: --tarball=*)
Line 74: exit 0
Line 75: ;;
Line 76: --tarball=*)
Line 77: TARBALL="${v}"
Line 78: ;;
at last you should put:
*)
usage
exit 1
;;
Line 79: esac
Line 80: done
Line 81: }
Line 82:
Line 81: }
Line 82:
Line 83: verifyArgs() {
Line 84: # Verify backup OR restore specified
Line 85: [ -n "${MODE}" ] || die "--mode=<backup|restore is missing"
< ?
Line 86:
Line 87: if [ "${MODE}" == "backup" ] ; then
Line 88: BACKUP=1
Line 89: elif [ "${MODE}" == "restore" ] ; then
Line 87: if [ "${MODE}" == "backup" ] ; then
Line 88: BACKUP=1
Line 89: elif [ "${MODE}" == "restore" ] ; then
Line 90: [ -e "${TARBALL}" ] || die "--mode=restore requires
--tarball=<path to backup tarball>"
Line 91: RESTORE=1
why do you need to split BACKUP and RESTORE and not keep MODE?
Line 92: else
Line 93: die "mode ${MODE} is not supported"
Line 94: fi
Line 95:
Line 97: }
Line 98:
Line 99: doBackup() {
Line 100: # Create temporart folder
Line 101: TARBUILD_FOLDER="${TEMP_FOLDER}/tar"
local?
lower case?
Line 102: mkdir -p "${TARBUILD_FOLDER}"
Line 103:
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
Line 98:
Line 99: doBackup() {
Line 100: # Create temporart folder
Line 101: TARBUILD_FOLDER="${TEMP_FOLDER}/tar"
Line 102: mkdir -p "${TARBUILD_FOLDER}"
always delete temp before usage?
Line 103:
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
Line 106: dirname=$(dirname "${folder}")
Line 101: TARBUILD_FOLDER="${TEMP_FOLDER}/tar"
Line 102: mkdir -p "${TARBUILD_FOLDER}"
Line 103:
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
should be:
echo "${BACKUP_FOLDERS} | while read folder; do
Line 106: dirname=$(dirname "${folder}")
Line 107: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
Line 108: cp -a "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Line 109: done
Line 102: mkdir -p "${TARBUILD_FOLDER}"
Line 103:
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
Line 106: dirname=$(dirname "${folder}")
should be dirname="$(...)"
Line 107: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
Line 108: cp -a "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Line 109: done
Line 110:
Line 103:
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
Line 106: dirname=$(dirname "${folder}")
Line 107: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
should be:
mkdir -p "${TARBUILD_FOLDER}/${dirname}"
Line 108: cp -a "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
Line 109: done
Line 110:
Line 111: # Backup database
Line 104: # Iterate folders and copy them
Line 105: for folder in ${BACKUP_FOLDERS}; do
Line 106: dirname=$(dirname "${folder}")
Line 107: mkdir -p "${TARBUILD_FOLDER}"/"${dirname}"
Line 108: cp -a "${folder}" "${TARBUILD_FOLDER}"/"${dirname}"
same
Line 109: done
Line 110:
Line 111: # Backup database
Line 112: backupDB "${TARBUILD_FOLDER}/${BACKUPS_DIR}"
Line 114: # Pack everything
Line 115: createTarball "${TARBUILD_FOLDER}" "${TARBALL}"
Line 116: }
Line 117:
Line 118: createTarball() {
I don't understand why keep a function that is called once and is one line.
Line 119: local dir="$1"
Line 120: local file="$2"
Line 121: tar -C "${dir}" -cjf "${file}" .
Line 122: }
Line 117:
Line 118: createTarball() {
Line 119: local dir="$1"
Line 120: local file="$2"
Line 121: tar -C "${dir}" -cjf "${file}" .
|| die "tar failed"
Line 122: }
Line 123:
Line 124: backupDB() {
Line 125: local outfolder="$1"
Line 124: backupDB() {
Line 125: local outfolder="$1"
Line 126: mkdir -p "${outfolder}"
Line 127: PGPASSFILE="${MYPGPASS}" \
Line 128: pg_dump \
indent...
pg_dump \
parameter \
parameter
Line 129: -E "UTF8" \
Line 130: --disable-dollar-quoting \
Line 131: --disable-triggers \
Line 132: --format=p \
Line 134: -U "${ENGINE_DB_USER}" \
Line 135: -h "${ENGINE_DB_HOST}" \
Line 136: -p "${ENGINE_DB_PORT}" \
Line 137: -f "${outfolder}"/"${DB_BACKUP_FILE_NAME}" \
Line 138: "${ENGINE_DB_DATABASE}"
|| die "backup failed"
Line 139: }
Line 140:
Line 141: doRestore() {
Line 142: extractTarball "${TARBALL}"
Line 140:
Line 141: doRestore() {
Line 142: extractTarball "${TARBALL}"
Line 143: generatePgPass
Line 144: restoreDB "${BACKUPS_DIR}"/"${DB_BACKUP_FILE_NAME}"
"${x}/${y}"
Line 145: }
Line 146:
Line 147: extractTarball() {
Line 148: local file="$1"
Line 143: generatePgPass
Line 144: restoreDB "${BACKUPS_DIR}"/"${DB_BACKUP_FILE_NAME}"
Line 145: }
Line 146:
Line 147: extractTarball() {
No need one line functions.
Line 148: local file="$1"
Line 149: tar -C "/" -xf "${file}"
Line 150: }
Line 151:
Line 163: -d "${ENGINE_DB_DATABASE}"
Line 164: }
Line 165:
Line 166: generatePgPass() {
Line 167: MYPGPASS="${TEMP_FOLDER}/.pgpass"
touch and chmod before writing?
Line 168: cat > "${MYPGPASS}" << __EOF__
Line 169:
${ENGINE_DB_HOST}:${ENGINE_DB_PORT}:${ENGINE_DB_DATABASE}:${ENGINE_DB_USER}:${ENGINE_DB_PASSWORD}
Line 170: __EOF__
Line 171: chmod 0600 "${MYPGPASS}"
Line 170: __EOF__
Line 171: chmod 0600 "${MYPGPASS}"
Line 172: }
Line 173:
Line 174: createTempFolder() {
no need one line functions...
Line 175: TEMP_FOLDER=$(mktemp -d)
Line 176: }
Line 177:
Line 178: init() {
Line 188:
Line 189: # TODO: Handle DBONLY option
Line 190: # TODO: ovirt-engine service handling
Line 191: init
Line 192: if [ -n "${BACKUP}" ]; then
use mode...
case "${MODE}" in
backup) doBackup ;;
esac
Line 193: doBackup
Line 194: echo "Backup completed successfully."
Line 195: echo "Backup is available at "${TARBALL}""
Line 196: fi
Line 191: init
Line 192: if [ -n "${BACKUP}" ]; then
Line 193: doBackup
Line 194: echo "Backup completed successfully."
Line 195: echo "Backup is available at "${TARBALL}""
no need, please force user to specify file, do not guess.
if user wishes to redirect this to whatever place he can do that.
Line 196: fi
Line 197:
Line 198: if [ -n "${RESTORE}" ] ; then
Line 199: doRestore
--
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: 6
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
