Merge ~rafaeldtinoco/ubuntu/+source/cloud-utils:lp1835124 into ubuntu/+source/cloud-utils:ubuntu/disco-devel

[Rafael David Tinoco]

SRU-ing:

https://bugs.launchpad.net/ubuntu/+source/cloud-utils/+bug/1835124
https://bugs.launchpad.net/ubuntu/+source/cloud-utils/+bug/1836593
https://bugs.launchpad.net/ubuntu/+source/cloud-utils/+bug/1810857

For Ubuntu Disco.

[Rafael David Tinoco (rafaeldtinoco)]

Scott,

I thought these 3 fixes, being straightforward, could be in Disco, at least, as well.

[Scott Moser (smoser)]

@Rafael,

Summary: This is fine, and I have some comments inline below.

Details.
The process you've used here is perfectly acceptable ('B' below). Please forgive my long winded response.

There are basically two paths that ubuntu server team has taken for packaging source management, and I think each are valid.
 A. upstream maintained packaging branches (lp:cloud-utils branches)
  )
 B. git-ubuntu: Ubuntu maintained packaging branches (lp:ubuntu/cloud-utils).

For cloud-init and curtin, the server team has been using 'A' and has tools to
ease that management documented at
https://github.com/CanonicalLtd/uss-tableflip/blob/master/doc/ubuntu_release_process.md
. Please read that doc. 'A' has to allow for uploads that are
done via 'B', so there is a way to "import" uploads to upstream packaging
branches that were done via 'B'. For 'B', an process like 'A' is just unknown
and results in a new imported version by the importer, just as if you'd done an
upload "by hand" to the archive.

Within 'A' there There are basically two options listed there for doing SRU patches. I generally prefer "new upstream snapshot" path as it makes maintenance much simpler. I think that doing a "new upstream snapshot" back into ubuntu/disco should be acceptable to SRU team, changes should be reviewed on merit of change rather than arbitrary numbers or whether or not something was released as a source tarball.

The snapshot backport works *really* well as it has all the metadata from
upstream git to make it "just work" and the tool 'new-upstream-snapshot' knows
enough about Ubuntu packaging to get version numbers and such right.

I think this process works well for packages who consider Ubuntu a "first class citizen". The difference is basically that in this case, upstream (lp:cloud-utils) maintains its *own* packaging branches and does package development on those branches. The ubuntu source (lp:ubuntu/cloud-utils) is just treated as artifacts.

What I suggest is this:
 * cloud-utils use 'A' for ubuntu/devel. The process is just really straight
   forward and the kinks worked out in cloud-init and curtin. If there are uploads outside of cloud-utils then we need to import them into cloud-utils
ubuntu/devel branch.
 * cloud-utils use 'B' for SRUs (as you've done here). At this point that means cloud-utils upstream
   will just be unaware entirely of anything ubuntu line of development other
   than the ubuntu/devel.

[Scott Moser (smoser)]

For your 'debian/patches' files, I think I'd prefer to just use 'git format patch -1 HASH' of the commit you're cherry-picking.

In my understanding, that is a valid dep-3 format.

It also makes it easier, and then you don't have to worry about people nit picking your choice to use bit.ly ;)

[Rafael David Tinoco (rafaeldtinoco)]

Scott,

Tks a lot for this clarification, really useful. I'll read uss-tableflip. I confess that's new to me and it makes total sense everything you said.

About DEP-3 format, sure. First time I use bit.ly, the http patch pointer was huge =). Let me re-push this to you then.

[Scott Moser (smoser)]

this looks good.

The only comment I have is on the names of your patches/ files.
I think i might have just left them as git wrote them. Again... doing that means you don't have to defend "why did you do that", as you can just blame the author of the patch when they wrote the subject line.

You can fix that, or not.

Do you need me to sponsor upload this?

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

Well, since the Subject: inside patches are intact, so you can always find upstream patch if needed, and you don't mind that much, I'd rather point the LP being fixed in the patch name. Of course, like always, those type of things are peculiar to each pkg maintainer and/or sponsor and we usually comply on their request.

If you're good, yes, please, could you sponsor it for me ?

Thanks a lot!

Rafael

[Scott Moser (smoser)]

@Rafael,

I just uploaded b26a5aa7736d4e64c19f66fa563a12808b1c4ebd as upload/0.31-0ubuntu1.1.

Email says:
 [ubuntu/disco-proposed] cloud-utils 0.31-0ubuntu1.1 (Waiting for approval)

I do not know what is the process or if there is one
for me to request a push of a tag for to usd-import-team.

[Andreas Hasenack (ahasenack)]

I verified that what was uploaded matches b26a5aa7736d4e64c19f66fa563a12808b1c4ebd, and am pushing an upload tag for it:

$ git push pkg upload/0.31-0ubuntu1.1
Enumerating objects: 26, done.
Counting objects: 100% (26/26), done.
Delta compression using up to 2 threads
Compressing objects: 100% (23/23), done.
Writing objects: 100% (23/23), 4.57 KiB | 1.14 MiB/s, done.
Total 23 (delta 13), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/cloud-utils
 * [new tag] upload/0.31-0ubuntu1.1 -> upload/0.31-0ubuntu1.1

[Rafael David Tinoco (rafaeldtinoco)]

Alright, I changed public bug to contain proper SRU template. This only needs sponsoring and the upload for me to verify the SRU before the migration. Thx for the review!

diff --git a/debian/changelog b/debian/changelog
index 0054615..80d02dd 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+cloud-utils (0.31-0ubuntu1.1) disco; urgency=medium
+
+  * fix race condition in test-growpart teardown seen on ppc64el (LP: #1836593)
+  * growpart: fix bug when file image ends in a digit (LP: #1835124)
+  * fix spelling error in ec2metadata (LP: #1810857)
+
+ -- Rafael David Tinoco <[e-mail]>  Wed, 17 Jul 2019 11:14:34 -0300
+
 cloud-utils (0.31-0ubuntu1) disco; urgency=medium
 
   * debian/tests/control: add test growpart-start-matches-size.
diff --git a/debian/patches/lp1810857-fix-spelling-error-in-ec2metadata-reservation-id.patch b/debian/patches/lp1810857-fix-spelling-error-in-ec2metadata-reservation-id.patch
new file mode 100644
index 0000000..e736e82
--- /dev/null
+++ b/debian/patches/lp1810857-fix-spelling-error-in-ec2metadata-reservation-id.patch
@@ -0,0 +1,33 @@
+From 7adb670f6a3ac76cedc97ceb5e2c801253864614 Mon Sep 17 00:00:00 2001
+From: Scott Moser <[e-mail]>
+Date: Tue, 8 Jan 2019 09:28:54 -0500
+Subject: [PATCH] Fix spelling error in ec2metadata (--reserveration-id).
+
+This just makes ec2metadata correctly take:
+  --reservation-id
+rather than the broken
+  --reserveration-id
+
+Thanks to Daniel Serodio for noticing and reporting the bug.
+
+LP: #1810857
+---
+ bin/ec2metadata | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/bin/ec2metadata b/bin/ec2metadata
+index 9b4b06d..01a42d7 100755
+--- a/bin/ec2metadata
++++ b/bin/ec2metadata
+@@ -86,7 +86,7 @@ METAOPTS = ['ami-id', 'ami-launch-index', 'ami-manifest-path',
+             'instance-action', 'instance-id', 'instance-type',
+             'local-hostname', 'local-ipv4', 'kernel-id', 'mac',
+             'profile', 'product-codes', 'public-hostname', 'public-ipv4',
+-            'public-keys', 'ramdisk-id', 'reserveration-id', 'security-groups',
++            'public-keys', 'ramdisk-id', 'reservation-id', 'security-groups',
+             'user-data']
+ 
+ binstdout = os.fdopen(sys.stdout.fileno(), 'wb')
+-- 
+2.22.0
+
diff --git a/debian/patches/lp1835124-growpart-fix-bug-when-file-image-ends-in-a-digit.patch b/debian/patches/lp1835124-growpart-fix-bug-when-file-image-ends-in-a-digit.patch
new file mode 100644
index 0000000..8b95fdd
--- /dev/null
+++ b/debian/patches/lp1835124-growpart-fix-bug-when-file-image-ends-in-a-digit.patch
@@ -0,0 +1,53 @@
+From fadd07fed6600764b375ed706e14696a6259c5c7 Mon Sep 17 00:00:00 2001
+From: Scott Moser <[e-mail]>
+Date: Wed, 10 Jul 2019 12:20:23 -0400
+Subject: [PATCH] growpart: Fix bug when file image ends in a digit.
+
+If the input to growpart was a file that ended in a digit (my.disk0)
+then growing partition would fail because sfdisk output will
+uses a 'p' to delimit the filename and partition name.
+
+The fix here is safe/dumb at the cost of a bit of verbosity.
+
+LP: #1835124
+---
+ bin/growpart | 23 +++++++++++++++--------
+ 1 file changed, 15 insertions(+), 8 deletions(-)
+
+diff --git a/bin/growpart b/bin/growpart
+index 446f63b..33d27f1 100755
+--- a/bin/growpart
++++ b/bin/growpart
+@@ -264,14 +264,21 @@ resize_sfdisk() {
+ 		fail "sed failed on dump output"
+ 
+ 	dpart="${DISK}${PART}" # disk and partition number
+-	if [ -b "${DISK}p${PART}" -a "${DISK%[0-9]}" != "${DISK}" ]; then
+-		# for block devices that end in a number (/dev/nbd0)
+-		# the partition is "<name>p<partition_number>" (/dev/nbd0p1)
+-		dpart="${DISK}p${PART}"
+-	elif [ "${DISK#/dev/loop[0-9]}" != "${DISK}" ]; then
+-		# for /dev/loop devices, sfdisk output will be <name>p<number>
+-		# format also, even though there is not a device there.
+-		dpart="${DISK}p${PART}"
++	if [ -b "$DISK" ]; then
++		if [ -b "${DISK}p${PART}" -a "${DISK%[0-9]}" != "${DISK}" ]; then
++			# for block devices that end in a number (/dev/nbd0)
++			# the partition is "<name>p<partition_number>" (/dev/nbd0p1)
++			dpart="${DISK}p${PART}"
++		elif [ "${DISK#/dev/loop[0-9]}" != "${DISK}" ]; then
++			# for /dev/loop devices, sfdisk output will be <name>p<number>
++			# format also, even though there is not a device there.
++			dpart="${DISK}p${PART}"
++		fi
++	else
++		case "$DISK" in
++			# sfdisk for files ending in digit to <disk>p<num>.
++			*[0-9]) dpart="${DISK}p${PART}";;
++		esac
+ 	fi
+ 
+ 	pt_start=$(awk '$1 == pt { print $4 }' "pt=${dpart}" <"${dump_mod}") &&
+-- 
+2.22.0
+
diff --git a/debian/patches/lp1836593-fix-race-condition-in-test-growpart.patch b/debian/patches/lp1836593-fix-race-condition-in-test-growpart.patch
new file mode 100644
index 0000000..04cd429
--- /dev/null
+++ b/debian/patches/lp1836593-fix-race-condition-in-test-growpart.patch
@@ -0,0 +1,71 @@
+From 51ef36d2281a38a65111995718e8ab85178438e5 Mon Sep 17 00:00:00 2001
+From: Scott Moser <[e-mail]>
+Date: Mon, 15 Jul 2019 10:24:47 -0400
+Subject: [PATCH] Fix race condition in test-growpart teardown seen on ppc64el.
+
+The test case 'test-growpart' seemed to have a race condition that
+would sometimes be seen on the Ubuntu autopkg builder infrastructure.
+The failure was only seen on ppc64el.
+
+The fix here is to use 'udevadm settle' liberally in teardown.
+I have admittedly not thought this completely through as to what
+*could* cause this scenario, and 'udevadm settle' often has the same
+effect as 'sleep 1'.  That said, this is just in testcase teardown
+so there is no harm in making it work even if this is not the fully
+"proper" fix.
+
+Also here is removal of code that was not used in test-growpart-fsimage.
+There, MP and LODEV were never set, so the removed cleanup would
+never run.
+
+LP: #1836593
+---
+ test/test-growpart         | 8 +++++++-
+ test/test-growpart-fsimage | 6 ------
+ 2 files changed, 7 insertions(+), 7 deletions(-)
+
+diff --git a/test/test-growpart b/test/test-growpart
+index e00b639..6df4b89 100755
+--- a/test/test-growpart
++++ b/test/test-growpart
+@@ -25,13 +25,19 @@ clearparts() {
+ 		echo "delpart $LODEV $part"
+ 		delpart $LODEV $part
+ 	done
++	udevadm settle
+ }
+ cleanup() {
+-	[ -z "$MP" ] || { echo "unmount $MP"; umount "$MP"; }
++	if [ -z "$MP" ]; then
++		echo "unmount $MP";
++		umount "$MP";
++		udevadm settle
++	fi
+ 	if [ -n "$LODEV" ]; then
+ 		clearparts "$LODEV"
+ 		echo "losetup --detach $LODEV";
+ 		losetup --detach "$LODEV";
++		udevadm settle
+ 	fi
+ 	[ ! -d "${TEMP_D}" ] || rm -Rf "${TEMP_D}"
+ }
+diff --git a/test/test-growpart-fsimage b/test/test-growpart-fsimage
+index 39e123c..7d35e73 100755
+--- a/test/test-growpart-fsimage
++++ b/test/test-growpart-fsimage
+@@ -12,12 +12,6 @@ rq() {
+ }
+ 
+ cleanup() {
+-	[ -z "$MP" ] || { echo "unmount $MP"; umount "$MP"; }
+-	if [ -n "$LODEV" ]; then
+-		clearparts "$LODEV"
+-		echo "losetup --detach $LODEV";
+-		losetup --detach "$LODEV";
+-	fi
+ 	[ ! -d "${TEMP_D}" ] || rm -Rf "${TEMP_D}"
+ }
+ TEMP_D=$(mktemp -d ${TMPDIR:-/tmp}/${0##*/}.XXXXXX)
+-- 
+2.22.0
+
diff --git a/debian/patches/series b/debian/patches/series
new file mode 100644
index 0000000..d6f2064
--- /dev/null
+++ b/debian/patches/series
@@ -0,0 +1,3 @@
+lp1810857-fix-spelling-error-in-ec2metadata-reservation-id.patch
+lp1835124-growpart-fix-bug-when-file-image-ends-in-a-digit.patch
+lp1836593-fix-race-condition-in-test-growpart.patch