growpart mishandles image filenames that end in a number

cloud-utils growpart script uses either sgdisk or sfdisk as a resizer... unfortunately it does not expect disk formats other than:

resize_sfdisk_gpt() {
 resize_sfdisk gpt
}

resize_sfdisk_dos() {
 resize_sfdisk dos
}

where $1 is the type of the disk format.

that is why you're getting this error. I'll flag this as "Triaged" and "Wishlist", and put in server team queue for further analysis.

Thanks for reporting this!

The error occurs with a gpt formatted image, which is supported by sfdisk.

This logic in growpart is broken for image files:

if [ -b "${DISK}p${PART}" -a "${DISK%[0-9]}" != "${DISK}" ]; then

If an image filename ends with a number, sfdisk identifies each partition like:

${DISK}p${PART_NUMBER}

Each partition is _not_ a block device, though, so they fail the `-b "${DISK}p${PART}"` check.

Removing that specific check fixes this issue; however, I don't know what ramification that has for other use cases.

As a workaround, we are renaming the image file to end with a non-numerical suffix, growing the partition, then renaming the image back to its original name.

I see what you mean now David, sorry for the wrong assumption, let me re-check this for you.

I agree that this is a bug (fix proposed), but fwiw, your naming of a *disk* image with an extension 'ext4' is a bit confusing to anyone that might see that file. Probably better '.disk' or '.img'.

Yes Scott, I do agree, and that is why I thought this was related to a loopback filesystem only, having a GPT/MSDOS partition table in a file called .ext4 is awkward to me as well. Thank you very much for your path, I'm reviewing it right now.

(k)inaddy@kvirtclone:~$ sudo fdisk /fakedisk

Welcome to fdisk (util-linux 2.33.1).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.

Command (m for help): p
Disk /fakedisk: 128 MiB, 134217728 bytes, 262144 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x9953d6a9

Device Boot Start End Sectors Size Id Type
/fakedisk1 2048 262143 260096 127M 83 Linux

Command (m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p):

Using default response p.
Partition number (1-4, default 1):
First sector (2048-262143, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-262143, default 262143): +100mb

Created a new partition 1 of type 'Linux' and of size 95 MiB.

(k)inaddy@kvirtclone:~$ sudo qemu-nbd -c /dev/nbd8 -f raw /fakedisk

(k)inaddy@kvirtclone:~$ ls /dev/nbd8*
/dev/nbd8 /dev/nbd8p1

(k)inaddy@kvirtclone:~$ sudo growpart /dev/nbd8 1
CHANGED: partition=1 start=2048 old: size=194560 end=196608 new: size=260063 end=262111

(k)inaddy@kvirtclone:~$ sudo mv /fakedisk /fakedisk.ext4

(k)inaddy@kvirtclone:~$ sudo fdisk /fakedisk.ext4

(m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p): p
Partition number (1-4, default 1):
First sector (2048-262143, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-262143, default 262143): +100mb

Created a new partition 1 of type 'Linux' and of size 95 MiB.

Command (m for help): p
Disk /fakedisk.ext4: 128 MiB, 134217728 bytes, 262144 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x9953d6a9

Device Boot Start End Sectors Size Id Type
/fakedisk.ext4p1 2048 196607 194560 95M 83 Linux

Command (m for help): w
The partition table has been altered.
Syncing disks.

(k)inaddy@kvirtclone:~$ sudo growpart /fakedisk.ext4 1
FAILED: failed to get start and end for /fakedisk.ext41 in /fakedisk.ext4

### BEFORE YOUR PATCH ^^^^^^^^

### AFTER YOUR PATCH vvvvvvvvv

(k)inaddy@kvirtclone:~/work/sources/ubuntu/cloud-utils$ sudo dpkg -i ./*.deb
(Reading database ... 129124 files and directories currently installed.)
Preparing to unpack .../cloud-guest-utils_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-guest-utils (0.30-52-g97fddc7b-0ubuntu1) over (0.30-51-g7adb670f-0ubuntu1) ...
Preparing to unpack .../cloud-image-utils_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-image-utils (0.30-52-g97fddc7b-0ubuntu1) over (0.30-51-g7adb670f-0ubuntu1) ...
Preparing to unpack .../cloud-utils-euca_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-utils-euca (0.30-52-g97fddc7b-0ubuntu1) over (0...

Scott shouldn't this be first merge into upstream first ? Then I can merge eoan with upstream and SRU the fix for other versions as DEP3.

@Rafael, yes. Fixed upstream now.

I'm going to also do an upload to eoan using 'new-upstream-snapshot'
[1] https://github.com/CanonicalLtd/uss-tableflip/blob/master/scripts/new-upstream-snapshot

In order to make that workflow work, i did have to get tags into git from the old bzr repo and sign those tags.

Thanks a lot!

Thank you both for addressing the issue :-)

This bug was fixed in the package cloud-utils - 0.31-3-gfadd07fe-0ubuntu1

---------------
cloud-utils (0.31-3-gfadd07fe-0ubuntu1) eoan; urgency=medium

  * New upstream snapshot.
    - growpart: Fix bug when file image ends in a digit. (LP: #1835124)
    - Fix spelling error in ec2metadata (--reserveration-id).
      (LP: #1810857)
    - tools: rename export-tarball to make-tarball.

 -- Scott Moser <email address hidden> Wed, 10 Jul 2019 14:09:37 -0400

SRU information required in this bug please.

Sorry Robie,

I missed the template. Now its good to go I believe.

Hello David, or anyone else affected,

Accepted cloud-utils into disco-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/cloud-utils/0.31-0ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-disco to verification-done-disco. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-disco. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

All autopkgtests for the newly accepted cloud-utils (0.31-0ubuntu1.1) for disco have finished running.
The following regressions have been reported in tests triggered by the package:

cloud-utils/0.31-0ubuntu1.1 (amd64, i386, ppc64el, arm64, s390x)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/disco/update_excuses.html#cloud-utils

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

We ran into this bug initially on xenial when doing image builds. Manually installing the cloud-guest-utils 0.31-0ubuntu1.1 deb in that environment resolves the issue.

The excuses page linked in comment 15 lists a test failure that is a regression of the change for this bug.

I've filed bug 1842682 and put up a pull request to fix that logic error.

Alright, I'll backport LP: #1842682 SRU together with this one and propose it in the existing merge for review.

MR:

https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/cloud-utils/+git/cloud-utils/+ref/lp1835124

rebases previous merge including your fix.

Feel free to sponsor if review is +1.

Let me know if there are any issues.

Thx Scott!

For whoever interests, the version fixing the issue with autopkgtests (that blocked migration of 0.31-0ubuntu1.1) is 0.31-0ubuntu1.2. So there will be a new fix to be verified in this bug.

I confirm version 0.31-0ubuntu1.1 indeed fixes the issue.

Hello David, or anyone else affected,

Accepted cloud-utils into disco-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/cloud-utils/0.31-0ubuntu1.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-disco to verification-done-disco. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-disco. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

verified:

----

(k)rafaeldtinoco@cloudutils:~$ sudo fdisk /fakedisk.ext4

Welcome to fdisk (util-linux 2.33.1).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.

Command (m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p):

Using default response p.
Partition number (1-4, default 1):
First sector (2048-204799, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-204799, default 204799): +50MB

Created a new partition 1 of type 'Linux' and of size 48 MiB.

Command (m for help): w
The partition table has been altered.
Syncing disks.

(k)rafaeldtinoco@cloudutils:~$ sudo growpart /fakedisk.ext4 1
CHANGED: partition=1 start=2048 old: size=98304 end=100352 new: size=202719 end=204767

----

all good.

This bug was fixed in the package cloud-utils - 0.31-0ubuntu1.2

---------------
cloud-utils (0.31-0ubuntu1.2) disco; urgency=medium

  * test-growpart: fix logic error resulting in test failure.
    [Scott Moser] (LP: #1842682)

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 <email address hidden> Fri, 06 Sep 2019 10:47:23 -0300

The verification of the Stable Release Update for cloud-utils has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.