Merge lp:~gandelman-a/ubuntu/oneiric/cobbler/lp850880-850866 into lp:ubuntu/oneiric/cobbler

Proposed by Adam Gandelman
Status: Merged
Merge reported by: St├ęphane Graber
Merged at revision: not available
Proposed branch: lp:~gandelman-a/ubuntu/oneiric/cobbler/lp850880-850866
Merge into: lp:ubuntu/oneiric/cobbler
Diff against target: 250 lines (+129/-43)
2 files modified
debian/changelog (+11/-0)
debian/cobbler-ubuntu-import (+118/-43)
To merge this branch: bzr merge lp:~gandelman-a/ubuntu/oneiric/cobbler/lp850880-850866
Reviewer Review Type Date Requested Status
Andres Rodriguez Needs Fixing
Scott Moser (community) Needs Fixing
Ubuntu branches Pending
Review via email: mp+78904@code.launchpad.net

Description of the change

This extends and adds some needed functionality:

* -u/--update can be used to download a newer ISO from the archive, import it into cobbler as a new distro. After its imported with a temp. name, sub-profiles of the outdated profile are reassigned to the updated version. The outdated distro is removed and the newer version renamed to match. This whole song and dance is necessary to preserve profiles and systems that are dependent on the root distro within the cobbler resource tree.

* both new imports and update checks are done first against a releases -update pocket and, failing that (404), falls back to the release pocket. This should allow new imports to import the most recent mini.isos, and later runs to check against whats been published since last run. A trivial change to orchestra-import-isos can make use of this and --update to keep cached ISOs up to date between cron'd runs.

* -v existed previously, but the corresponding VERBOSITY variable was never used. Added debug() which is useful for debugging info during some of the more involved stuff

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

Some comments:
 * I don't like the use of globals in update_available and get_isos. I dont have a simple solution for you, and i wouldnt nack it completely based on that, but globals (or lexical scoped variables) are just confusing. If you have to use them, please use upper case names to indicate that they're global.
 * use more quotes:
   Things like '[ -n $md5sum_local ]' should really be '[ -n "$md5sum_local" ]', as the variable md5sum_local could have either shell wildcards or spaces in it which change behavior unexpectedly.
 * there are some white space indentation rather than tab (search for '[ ][ ][ ]' and you'll see).
   Also, you can set vi to read the modelines (the last line of the file) with 'set modeline modelines=3'

Also, I'd like a general "just do the right thing" flag.
So that I could do something like:
  cobbler-ubuntu-import --do-what-i-mean natty-i386 natty-amd64 oneiric-amd64 oneiric-i386

And it would import or update if necessary, and exit success unless an operation failed. It would even make sense to me that that should be the default behavior.

The last comment is that it would be nice if 'cobbler import' had a '--replace' flag. We might want to do that and submit it upstream that did your rename dance inside. It would be able to do more appropriate locking and such to avoid two things attempting the call at the same time.

Thanks for your work, I really hope not to sound negative.

review: Needs Fixing
53. By Adam Gandelman

Better quoting of shell variables

54. By Adam Gandelman

Also remove new, temp. profile after distro has been renamed

55. By Adam Gandelman

Capitalize global variables, lower case locals

56. By Adam Gandelman

Fix up some whitespace

Revision history for this message
Adam Gandelman (gandelman-a) wrote :

Hi Scott-

Just pushed some changes which address your 3 points. I agree global variables suck, but to do anything to properly get rid of them at this point would be a major rewrite of the script itself. Perhaps this is something we should address next cycle, maybe thinking about a better way to go about managing a local ISO cache.

I'd prefer to get these changes worked out and tested before changing the default behavior of the script, but I agree that it should probably do all this by default. And +1 to trying to get the same support in cobbler upstream. In the meantime, the --check and --update stuff should be enough.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

From conversation with Scott, this branch needs fixing:

smoser> * debug() should go to stderr, not stdout

I'll look into fixing it to getting it uploaded.

review: Needs Fixing
57. By Adam Gandelman

debug(): Redirect to stderr instead of stdout

Revision history for this message
James Page (james-page) wrote :

Merged in precise - please can this branch be marked as merged unless we are going to use this branch for the SRU.

---------------

cobbler (2.1.0+git20110602-0ubuntu27) precise; urgency=low

  [ Adam Gandelman ]
  * debian/cobbler-ubuntu-import:
      - Check update pockets for releases on download and update check.
        (LP: #850880 )
      - Allow '-u' to upgrade existing profiles to a newer version of an ISO.
        (LP: #850886)
      - '-v' to allow debug msgs.

  [ Andres Rodriguez ]
  * debian/patches:
    - 42_fix_repomirror_create_sync.patch: Updated to correctly create the
      mirror. (LP: #872926)
  * debian/cobbler-web.postrm: Remove symlinks that were created on
    postinst. (LP: #872892)
 -- Andres Rodriguez <email address hidden> Mon, 24 Oct 2011 19:29:26 -0400

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2011-09-29 23:58:08 +0000
3+++ debian/changelog 2011-10-12 20:47:41 +0000
4@@ -1,3 +1,14 @@
5+cobbler (2.1.0+git20110602-0ubuntu27) UNRELEASED; urgency=low
6+
7+ * debian/cobbler-ubuntu-import:
8+ - Check update pockets for releases on download and update check.
9+ (LP: #850880 )
10+ - Allow '-u' to upgrade existing profiles to a newer version of an ISO.
11+ (LP: #850886)
12+ - '-v' to allow debug msgs.
13+
14+ -- Adam Gandelman <adamg@canonical.com> Mon, 10 Oct 2011 18:44:00 -0700
15+
16 cobbler (2.1.0+git20110602-0ubuntu26) oneiric; urgency=low
17
18 * debian/cobbler-ubuntu-import: Add --check-update and --remove
19
20=== modified file 'debian/cobbler-ubuntu-import'
21--- debian/cobbler-ubuntu-import 2011-09-16 05:10:52 +0000
22+++ debian/cobbler-ubuntu-import 2011-10-12 20:47:41 +0000
23@@ -10,6 +10,7 @@
24 errorp() { printf "$@" 1>&2; }
25 fail() { [ $# -eq 0 ] || error "$@"; exit 1; }
26 failp() { [ $# -eq 0 ] || errorp "$@"; exit 1; }
27+debug() { [ $VERBOSITY -gt 0 ] && echo "[DEBUG] $@" 1>&2; }
28
29 Usage() {
30 cat <<EOF
31@@ -26,6 +27,8 @@
32 mirror. exits 0 if an update is needed or iso does
33 not exist in $ISO_DIR
34 -r | --remove remove cobbler profile/distro and cached ISO
35+ -u | --update update iso, import, replace outdated distro and profile
36+ -v | --verbose increase verbosity
37 Note, arch can be i386, x86_64, or amd64. However,
38 the registered distro and profile will use 'x86_64' rather than 'amd64'.
39 This is to keep consistent with cobbler naming.
40@@ -37,7 +40,7 @@
41
42 bad_Usage() { Usage 1>&2; [ $# -eq 0 ] || error "$@"; exit 1; }
43 cleanup() {
44- [ -z "${TEMP_D}" -o ! -d "${TEMP_D}" ] || rm -Rf "${TEMP_D}"
45+ [ -z "${TEMP_D}" -o ! -d "${TEMP_D}" ] || rm -Rf "${TEMP_D}"
46 }
47
48 loop_mount() {
49@@ -49,10 +52,91 @@
50 chown root:disk /dev/loop$loops
51 fi
52 # Do the loop mount
53+ debug "Creating loopback mount from $1 at $2"
54 mount -o loop "$1" "$2"
55 }
56
57-short_opts="Dhm:o:vcr"
58+get_iso() {
59+ # first check release's update pocket for an iso.
60+ # grab from release pocket if that does not exist.
61+ if wget -O "$TEMP_D/$ISO" "$U_UPDATE"; then
62+ debug "Downloaded ISO from updates pocket: $U_UPDATE"
63+ elif wget -O "$TEMP_D/$ISO" "$U"; then
64+ debug "Updating from release pocket: $U"
65+ else
66+ fail "failed download for $REL-$ARCH"
67+ fi
68+ { $delete || mv "$TEMP_D/$ISO" "$ISO_DIR/$ISO"; } ||
69+ fail "failed to move iso $ISO_DIR/$ISO"
70+}
71+
72+import_iso() {
73+ # import iso into cobbler as $DISTRO
74+ local mounted
75+ local distro="$1"
76+ if cobbler distro list | grep -qs " $distro"; then
77+ fail "distro '$distro' already exists. cannot import."
78+ fi
79+ { [ -f "$TEMP_D/$ISO" ] || ln -sf "$ISO_DIR/$ISO" "$TEMP_D/$ISO"; } ||
80+ fail "failed to create symlink to existing $ISO_DIR/$ISO"
81+ mounted=0 && loop_mount "$TEMP_D/$ISO" "$TEMP_D/mnt" && mounted=1 &&
82+ cobbler import --name="$distro" --path="$TEMP_D/mnt" \
83+ --breed=ubuntu --os-version="$REL" --arch="$ARCH" &&
84+ umount "${TEMP_D}/mnt" && mounted=0 || {
85+ [ $mounted -eq 0 ] || umount "$TEMP_D/mnt";
86+ fail "failed to import $REL-$ARCH";
87+ }
88+ echo "imported $distro"
89+}
90+
91+reassign_distro() {
92+ # swap out backing distro of profile with a newer, imported
93+ # distro. after, remove the original and rename the new distro
94+ # to match the previous distro name associated with profile
95+ local new_distro="$1"
96+ local profile="$2"
97+ debug "Reassigning $profile to $new_distro"
98+ if ! cobbler profile list | grep -qs " $profile"; then
99+ fail "cannot reassign non-existent profile '$profile'"
100+ fi
101+ if ! cobbler distro list | grep -qs " $profile"; then
102+ fail "distro matching profile '$profile' does not exist"
103+ fi
104+ debug "Renaming old profile '$profile' to 'last-$profile'"
105+ cobbler distro rename --name="$profile" --newname="last-$profile" ||
106+ fail "could not rename distro '$profile' to 'last-$profile'"
107+ debug "Assigning profile $profile to distro $new_distro"
108+ cobbler profile edit --name="$profile" --distro="$new_distro" ||
109+ fail "could not assign profile '$profile' to distro '$new_distro'"
110+ debug "Removing distro last-$profile"
111+ cobbler distro remove --name="last-$profile" ||
112+ fail "could not remove stale distro 'last-$profile'"
113+ debug "Renaming distro $new_distro to $profile"
114+ cobbler distro rename --name="$new_distro" --newname="$profile" ||
115+ fail "could not rename distro '$new_distro' to '$profile'"
116+ debug "Cleaning up temporary profile '$new_distro'"
117+ cobbler profile remove --name="$new_distro" ||
118+ fail "could not remove temp profile '$new_distro'"
119+}
120+
121+function update_available() {
122+ # check MD5SUM first from updates pocket if it exists, then release pocket.
123+ # compare netboot mini iso with what is local.
124+ local md5sum_local
125+ local md5sum_remote
126+ [ ! -f "$ISO_DIR/$ISO" ] && return 0
127+ md5sum_local=$(md5sum "$ISO_DIR/$ISO") && md5sum_local=${md5sum_local%% *} &&
128+ [ -n "$md5sum_local" ] || fail "failed to checksum $ISO_DIR/$ISO: $md5sum_local"
129+ debug "local md5sum: $md5sum_local ($ISO_DIR/$ISO)"
130+ wget -qO "$TEMP_D/MD5SUMS" "$MD5_UPDATE" || wget -qO "$TEMP_D/MD5SUMS" "$MD5" ||
131+ fail "failed to download MD5SUMS for $REL-$ARCH."
132+ md5sum_remote=$(awk '$2 == "./netboot/mini.iso" { print $1 }' $TEMP_D/MD5SUMS) &&
133+ [ -n "$md5sum_remote" ] || fail "failed to parse checksum from $TEMP_D/MD5SUMS: $md5sum_remote"
134+ debug "remote md5sum: $md5sum_remote"
135+ [ "$md5sum_local" != "$md5sum_remote" ]
136+}
137+
138+short_opts="Dhm:o:vcru"
139 long_opts="delete-iso,help,mirror:,verbose,update-check,remove"
140 getopt_out=$(getopt --name "${0##*/}" \
141 --options "${short_opts}" --long "${long_opts}" -- "$@") &&
142@@ -63,6 +147,7 @@
143 check_update=false
144 remove=false
145 mirror=${DEFAULT_MIRROR}
146+do_update=false
147
148 while [ $# -ne 0 ]; do
149 cur=${1}; next=${2};
150@@ -73,6 +158,7 @@
151 -v|--verbose) VERBOSITY=$((${VERBOSITY}+1));;
152 -c|--update-check) check_update=true;;
153 -r|--remove) remove=true;;
154+ -u|--update) do_update=true;;
155 --) shift; break;;
156 esac
157 shift;
158@@ -82,62 +168,51 @@
159 [ $# -ne 0 ] || bad_Usage "must provide arguments"
160
161 TEMP_D=$(mktemp -d "${TMPDIR:-/tmp}/${0##*/}.XXXXXX") ||
162- fail "failed to make tempdir"
163+ fail "failed to make tempdir"
164 trap cleanup EXIT
165
166 # program starts here
167 if $delete; then
168- { [ -d "$ISO_DIR" ] || mkdir -p "$ISO_DIR"; } || fail "failed to create $ISO_DIR"
169+ { [ -d "$ISO_DIR" ] || mkdir -p "$ISO_DIR"; } || fail "failed to create $ISO_DIR"
170 fi
171 mkdir "$TEMP_D/mnt" || fail "failed to make tempdir/mnt"
172
173 for tok in "$@"; do
174- rel=${tok%-*}; arch=${tok#*-}
175- [ "$arch" = "amd64" ] &&
176+ REL=${tok%-*}; ARCH=${tok#*-}
177+ [ "$ARCH" = "amd64" ] &&
178 { error "Warning: using x86_64 for arch rather than amd64"; arch="x86_64"; }
179- ubuntu_arch=$arch; [ "$arch" = "x86_64" ] && ubuntu_arch=amd64
180- iso=$rel-$arch-mini.iso
181- u=$mirror/dists/$rel/main/installer-$ubuntu_arch/current/images/netboot/mini.iso
182- md5=$mirror/dists/$rel/main/installer-$ubuntu_arch/current/images/MD5SUMS
183+ ubuntu_arch=$ARCH; [ "$ARCH" = "x86_64" ] && ubuntu_arch=amd64
184+ ISO=$REL-$ARCH-mini.iso
185+ U=$mirror/dists/$REL/main/installer-$ubuntu_arch/current/images/netboot/mini.iso
186+ MD5=$mirror/dists/$REL/main/installer-$ubuntu_arch/current/images/MD5SUMS
187+ U_UPDATE=$mirror/dists/$REL-updates/main/installer-$ubuntu_arch/current/images/netboot/mini.iso
188+ MD5_UPDATE=$mirror/dists/$REL-updates/main/installer-$ubuntu_arch/current/images/MD5SUMS
189 if $remove; then
190- if [ -f "$ISO_DIR/$iso" ]; then
191- rm -rf "$ISO_DIR/$iso" ||
192- fail "Could not delete $ISO_DIR/$iso"
193+ if [ -f "$ISO_DIR/$ISO" ]; then
194+ debug "Removing $ISO_DIR/$ISO"
195+ rm -rf "$ISO_DIR/$ISO" ||
196+ fail "Could not delete $ISO_DIR/$ISO"
197 fi
198- if cobbler distro list | grep -qs " $rel-$arch"; then
199- cobbler distro remove --name=$rel-$arch ||
200- fail "Could not remove cobbler profile for $rel-$arch"
201+ if cobbler distro list | grep -qs " $REL-$ARCH"; then
202+ debug "Removing cobbler distro '$REL-$ARCH'"
203+ cobbler distro remove --name="$REL-$ARCH" ||
204+ fail "Could not remove cobbler profile for $REL-$ARCH"
205 fi
206 exit 0
207 fi
208 if $check_update; then
209- # compare md5 of the local iso to whats on the mirror.
210- [ ! -f "$ISO_DIR/$iso" ] && exit 0
211- md5=$mirror/dists/$rel/main/installer-$ubuntu_arch/current/images/MD5SUMS
212- md5sum_local=$(md5sum "$ISO_DIR/$iso") && md5sum_local=${md5sum_local% *} \
213- && [ -n $md5sum_local ] || fail "failed to checksum $ISO_DIR/$iso"
214- wget -qO $TEMP_D/MD5SUMS $md5 || fail "failed to download MD5SUMS for $rel-$arch: $u"
215- md5sum_remote=$(awk '$2 == "./netboot/mini.iso" { print $1 }' $TEMP_D/MD5SUMS) &&
216- [ -n $md5sum_remote ] || fail "failed to parse checksum from $TEMP_D/MD5SUM"
217- [ $md5sum_local != $md5sum_remote ] && exit 0
218- error "$ISO_DIR/$iso up to date." && exit 3
219- fi
220- if [ ! -f "$ISO_DIR/$iso" ]; then
221- wget -O "$TEMP_D/$iso" "$u" && [ -s "$TEMP_D/$iso" ] ||
222- fail "failed download for $rel-$arch: $u"
223- { $delete || mv "$TEMP_D/$iso" "$ISO_DIR/$iso"; } ||
224- fail "failed to write to move iso to $ISO_DIR/$iso"
225- fi
226- { [ -f $TEMP_D/$iso ] || ln -sf "$ISO_DIR/$iso" "$TEMP_D/$iso"; } ||
227- fail "failed to create symlink to existing $ISO_DIR/$iso"
228- mounted=0 && loop_mount "$TEMP_D/$iso" "$TEMP_D/mnt" && mounted=1 &&
229- cobbler import --name=$rel-$arch --path="$TEMP_D/mnt" \
230- --breed=ubuntu --os-version=$rel --arch=$arch &&
231- umount "${TEMP_D}/mnt" && mounted=0 || {
232- [ $mounted -eq 0 ] || umount "$TEMP_D/mnt";
233- fail "failed to import $rel-$arch";
234- }
235- echo "imported $rel-$arch"
236+ update_available && echo "Update for $REL-$ARCH available" && exit 0
237+ echo "$ISO_DIR/$ISO up to date." && exit 3
238+ fi
239+ if $do_update; then
240+ update_available || fail "No update available"
241+ # get iso from either pocket and import it with a tmp name.
242+ get_iso && import_iso "tmp-$REL-$ARCH"
243+ reassign_distro "tmp-$REL-$ARCH" "$REL-$ARCH"
244+ exit 0
245+ fi
246+ [[ ! -f "$ISO_DIR/$ISO" ]] && get_iso
247+ import_iso "$REL-$ARCH" && echo "imported $REL-$ARCH"
248 done
249
250 # vi: ts=4 noexpandtab

Subscribers

People subscribed via source and target branches