Alon Bar-Lev has posted comments on this change.
Change subject: packaging: Add backup and restore utility
......................................................................
Patch Set 18: (15 inline comments)
I did not go over the logic... syntax and other stuff.
....................................................
File Makefile
Line 362: ln -sf "$(DATA_DIR)/setup/bin/ovirt-engine-setup"
"$(DESTDIR)$(BIN_DIR)/engine-upgrade"
Line 363: ln -sf "$(DATA_DIR)/setup/bin/ovirt-engine-remove"
"$(DESTDIR)$(BIN_DIR)/engine-cleanup"
Line 364: ln -sf "$(DATA_DIR)/bin/engine-config.sh"
"$(DESTDIR)$(BIN_DIR)/engine-config"
Line 365: ln -sf "$(DATA_DIR)/bin/engine-manage-domains.sh"
"$(DESTDIR)$(BIN_DIR)/engine-manage-domains"
Line 366: ln -sf "$(DATA_DIR)/bin/engine-backup.sh"
"$(DESTDIR)$(BIN_DIR)/engine-backup"
I am unsure these utilities should be at /usr
And if they do we should start putting them in sbin and not bin.
Line 367:
Line 368: install -d -m 755
"$(DESTDIR)$(PKG_SYSCONF_DIR)/notifier/notifier.conf.d"
Line 369: install -d -m 755 "$(DESTDIR)$(PKG_SYSCONF_DIR)/engine.conf.d"
Line 370: install -d -m 755
"$(DESTDIR)$(PKG_SYSCONF_DIR)/ovirt-websocket-proxy.conf.d"
....................................................
File packaging/bin/engine-backup.sh
Line 16: # limitations under the License.
Line 17: #
Line 18:
Line 19: # Load the prolog:
Line 20: . "$(dirname "$(readlink -f "$0")")"/engine-prolog.sh
I guess best is to add PACKAGE_NAME, PACKAGE_VERSION to engine-prolog.sh.in so
you can fetch it.
Line 21:
Line 22: # Globals
Line 23: BACKUP_FOLDERS="/etc/ovirt-engine
Line 24: /etc/pki/ovirt-engine
Line 23: BACKUP_FOLDERS="/etc/ovirt-engine
Line 24: /etc/pki/ovirt-engine
Line 25: /etc/ovirt-engine-setup.conf.d
Line 26: /var/lib/ovirt-engine"
Line 27: VERSION_FILE="/usr/share/ovirt-engine/conf/version"
this file is going to be obsoleted, please don't use it.
Line 28: MYPGPASS=""
Line 29: TEMP_FOLDER=""
Line 30: FILE=""
Line 31: DATE="$(date +%Y_%m_%d_%H_%M_%S)"
Line 27: VERSION_FILE="/usr/share/ovirt-engine/conf/version"
Line 28: MYPGPASS=""
Line 29: TEMP_FOLDER=""
Line 30: FILE=""
Line 31: DATE="$(date +%Y_%m_%d_%H_%M_%S)"
Please use sane standard format, plain %Y%m%d%H%M%S
Line 32: BACKUP_TAR="/tmp/engine-backup_${DATE}.tar.bz"
Line 33: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 34: MD5_FILE_NAME="md5sum"
Line 35: LOG="/tmp/engine-backup_${DATE}.log"
Line 28: MYPGPASS=""
Line 29: TEMP_FOLDER=""
Line 30: FILE=""
Line 31: DATE="$(date +%Y_%m_%d_%H_%M_%S)"
Line 32: BACKUP_TAR="/tmp/engine-backup_${DATE}.tar.bz"
no underscore please use '-', not sure why it is in /tmp and not specified by
user, or why the tar name and location is not specified by user.
I recommend to receive a parameter from user where he wants to create the
output, we should not do this for him.
Line 33: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 34: MD5_FILE_NAME="md5sum"
Line 35: LOG="/tmp/engine-backup_${DATE}.log"
Line 36: DB="db"
Line 30: FILE=""
Line 31: DATE="$(date +%Y_%m_%d_%H_%M_%S)"
Line 32: BACKUP_TAR="/tmp/engine-backup_${DATE}.tar.bz"
Line 33: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 34: MD5_FILE_NAME="md5sum"
no reason to store this in constant if not allowing user to override it.
Line 35: LOG="/tmp/engine-backup_${DATE}.log"
Line 36: DB="db"
Line 37: FILES="files"
Line 38: VERSION="version"
Line 31: DATE="$(date +%Y_%m_%d_%H_%M_%S)"
Line 32: BACKUP_TAR="/tmp/engine-backup_${DATE}.tar.bz"
Line 33: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 34: MD5_FILE_NAME="md5sum"
Line 35: LOG="/tmp/engine-backup_${DATE}.log"
again, please have --log= parameter and don't guess locations.
Line 36: DB="db"
Line 37: FILES="files"
Line 38: VERSION="version"
Line 39:
Line 32: BACKUP_TAR="/tmp/engine-backup_${DATE}.tar.bz"
Line 33: DB_BACKUP_FILE_NAME="engine_backup.db"
Line 34: MD5_FILE_NAME="md5sum"
Line 35: LOG="/tmp/engine-backup_${DATE}.log"
Line 36: DB="db"
again, there is no reason for constants that contains their content.
Line 37: FILES="files"
Line 38: VERSION="version"
Line 39:
Line 40: cleanup() {
Line 49: USAGE:
Line 50: engine-backup [--mode=MODE] [--scope=SCOPE] [--file=FILE]
Line 51: MODE is one of the following:
Line 52: backup backup system into FILE
Line 53: restore restore system from FILE
do not use tabs within terminal output.
Line 54: SCOPE is one of the following:
Line 55: all complete backup/restore
Line 56: db database only
Line 57: __EOF__
Line 104: fi
Line 105: }
Line 106:
Line 107: dobackup() {
Line 108: echo "Backing up..."
please have a function for user output, it can be echo now... and in [near]
future it will be print to terminal and log.
Line 109: generatePgPass
Line 110:
Line 111: # Create temporary folder
Line 112: local tardir="${TEMP_FOLDER}/tar"
Line 133: createmd5() {
Line 134: local tardir="$1"
Line 135: local md5file="$2"
Line 136: local cwd="$(pwd)"
Line 137: cd "${tardir}"
do not cd in program... options:
1. use subshell
( cd ... && command )
2. use lighter commands:
find "${tardir}" -printf '%P\n'
Line 138: find . -type f | while read file; do
Line 139: md5sum "${file}" >> "${md5file}" || die "Cannot create
checksum for '${file}'"
Line 140: done || die "Find execution failed"
Line 141: cd "${cwd}"
Line 144: verifymd5() {
Line 145: local tardir="$1"
Line 146: local md5file="$2"
Line 147: local cwd="$(pwd)"
Line 148: cd "${tardir}"
this should be in subshell
( cd "${tardir} && md5sum ... ) || die
Line 149: md5sum -c "${md5file}" --status --strict || die "Checksum
verification failed"
Line 150: cd "${cwd}"
Line 151: }
Line 152:
Line 247: MYPGPASS="${TEMP_FOLDER}/.pgpass"
Line 248: cat > "${MYPGPASS}" << __EOF__
Line 249:
${ENGINE_DB_HOST}:${ENGINE_DB_PORT}:${ENGINE_DB_DATABASE}:${ENGINE_DB_USER}:${ENGINE_DB_PASSWORD}
Line 250: __EOF__
Line 251: chmod 0600 "${MYPGPASS}"
usually it is better to touch, then chmod, then write.
Line 252: }
Line 253:
Line 254: welcome() {
Line 255: cat << __EOF__
Line 260: 3. engine user and database has the same credentials as the backup
Line 261: 4. Firewall is configured properly
Line 262: __EOF__
Line 263: while true; do
Line 264: read -p "Do you wish to continue? (y/n) " yn
this is command-line, it should not be interactive.
most users will run it out of cron or other automation.
this should be removed.
if you really like to make sure that user is knowing what he is doing add
parameter --i-know-what-i-am-doing.
Line 265: case "${yn}" in
Line 266: [Yy]* ) return 0;;
Line 267: [Nn]* ) exit;;
Line 268: * ) echo "Please answer yes or no.";;
Line 275: # Do this in function so we do not lose $@
Line 276: parseArgs "$@"
Line 277: verifyArgs
Line 278:
Line 279: TEMP_FOLDER="$(mktemp -d)"
please add prefix to template so we can detect our tmpdir for debug
Line 280: generatePgPass
Line 281: do${MODE}
--
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: 18
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Kiril Nesenko <knese...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Ohad Basan <oba...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
