Linaro Image Tools

Merge lp://staging/~gesha/linaro-image-tools/717129 into lp://staging/linaro-image-tools/11.11

717129
Merge into trunk

Proposed by Georgy Redkozubov on 2011-12-13

Status:

Merged

Approved by:

Mattias Backman on 2011-12-22

Approved revision:

476

Merged at revision:

479

Proposed branch:

lp://staging/~gesha/linaro-image-tools/717129

Merge into:

lp://staging/linaro-image-tools/11.11

Diff against target:

171 lines (+107/-8)

2 files modified

linaro_image_tools/media_create/partitions.py (+39/-8)
linaro_image_tools/media_create/tests/test_media_create.py (+68/-0)

To merge this branch:

bzr merge lp://staging/~gesha/linaro-image-tools/717129

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mattias Backman (community)			Approve on 2011-12-22
Georgy Redkozubov			Pending
Review via email: mp+85478@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-12-08.

Description of the change

This branch adds the ability to check for appearance of partitions to the UDisks service for the device being partitioned.
media_create looks in a loop for the devices if they are not found it sleeps for 1 second, then looks again, increases sleep time by 1 second, checks and sleeps again until maximum number of sleep attempts is reached or devices appear. Now MAX_TTS is 10 so the maximum sleep time is 55 seconds.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-12-08: Posted in a previous version of this proposal

Download full text (4.4 KiB)

Hi Georgy,

I have a few comments below but what worries me is that the new code is
untested and if in the future somebody accidentally breaks it we might end up
causing unnecessary sleeps.

There's also the fact that we don't seem to have anybody who can reproduce the
issue anymore to confirm that these changes actually fix the issue.

On Thu, 2011-12-08 at 12:29 +0000, Georgy Redkozubov wrote:
[...]
> This branch adds the ability to check for appearance of partitions in /dev for the device being partitioned.
> media_create looks in a loop for the devices if they are not found it sleeps for 1 second, then looks again, increases sleep time by 1 second, checks and sleeps again until maximum number of sleep attempts is reached or devices appear. Now MAX_TTS is 10 so the maximum sleep time is 55 seconds.
> I couldn't reproduce the failures to find more nice fix. Please review and try.
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_image_tools/media_create/partitions.py'
> --- linaro_image_tools/media_create/partitions.py 2011-07-28 01:24:21 +0000
> +++ linaro_image_tools/media_create/partitions.py 2011-12-08 12:29:00 +0000
> @@ -24,6 +24,7 @@
> import re
> import subprocess
> import time
> +import sys
>
> import dbus
> from parted import (
> @@ -42,6 +43,7 @@
> CYLINDER_SIZE = HEADS * SECTORS * SECTOR_SIZE
> DBUS_PROPERTIES = 'org.freedesktop.DBus.Properties'
> UDISKS = "org.freedesktop.UDisks"
> +MAX_TTS = 10 # max number of attempts to sleep (total sleep time in seconds = 1+2+...+MAX_TTS)

You could move the comment to a line by itself to keep the lines under 80
characters.

>
>
> def setup_android_partitions(board_config, media, image_size, bootfs_label,
> @@ -142,6 +144,7 @@
> should_align_boot_part=should_align_boot_part)
>
> if media.is_block_device:
> +#!!!! wait_partition_to_settle(media)

We can remove this now, right?

> bootfs, rootfs = get_boot_and_root_partitions_for_media(
> media, board_config)
> # It looks like KDE somehow automounts the partitions after you
> @@ -428,6 +431,15 @@
> """
> # This could be simpler but UDisks doesn't make it easy for us:
> # https://bugs.freedesktop.org/show_bug.cgi?id=33113.
> + tts = 1
> + while not glob.glob("%s?*" % device) and (tts <= MAX_TTS):
> + print "Sleeping for %s second(s) to wait for the partitions available" % tts
> + time.sleep(tts)
> + tts += 1

This looks very similar to wait_partition_to_settle(). Can't we reuse that
here? Maybe we could if we refactored it.

Hi Georgy,

I have a few comments below but what worries me is that the new code is
untested and if in the future somebody accidentally breaks it we might end up
causing unnecessary sleeps.

There's also the fact that we don't seem to have anybody who can reproduce the
issue anymore to confirm that these changes actually fix the issue.

On Thu, 2011-12-08 at 12:29 +0000, Georgy Redkozubov wrote:
[...]
> This branch adds the ability to check for appearance of partitions in /dev for the device being partitioned.
> media_create looks in a loop for the devices if they are not found it sleeps for 1 second, then looks again, increases sleep time by 1 second, checks and sleeps again until maximum number of sleep attempts is reached or devices appear. Now MAX_TTS is 10 so the maximum sleep time is 55 seconds.
> I couldn't reproduce the failures to find more nice fix. Please review and try.
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_image_tools/media_create/partitions.py'
> --- linaro_image_tools/media_create/partitions.py	2011-07-28 01:24:21 +0000
> +++ linaro_image_tools/media_create/partitions.py	2011-12-08 12:29:00 +0000
> @@ -24,6 +24,7 @@
>  import re
>  import subprocess
>  import time
> +import sys
>  
>  import dbus
>  from parted import (
> @@ -42,6 +43,7 @@
>  CYLINDER_SIZE = HEADS * SECTORS * SECTOR_SIZE
>  DBUS_PROPERTIES = 'org.freedesktop.DBus.Properties'
>  UDISKS = "org.freedesktop.UDisks"
> +MAX_TTS = 10 # max number of attempts to sleep (total sleep time in seconds = 1+2+...+MAX_TTS)

You could move the comment to a line by itself to keep the lines under 80
characters.

>  
> 
>  def setup_android_partitions(board_config, media, image_size, bootfs_label,
> @@ -142,6 +144,7 @@
>              should_align_boot_part=should_align_boot_part)
>  
>      if media.is_block_device:
> +#!!!!        wait_partition_to_settle(media)

We can remove this now, right?

>          bootfs, rootfs = get_boot_and_root_partitions_for_media(
>              media, board_config)
>          # It looks like KDE somehow automounts the partitions after you
> @@ -428,6 +431,15 @@
>      """
>      # This could be simpler but UDisks doesn't make it easy for us:
>      # https://bugs.freedesktop.org/show_bug.cgi?id=33113.
> +    tts = 1
> +    while not glob.glob("%s?*" % device) and (tts <= MAX_TTS):
> +        print "Sleeping for %s second(s) to wait for the partitions available" % tts
> +        time.sleep(tts)
> +        tts += 1

This looks very similar to wait_partition_to_settle(). Can't we reuse that
here? Maybe we could if we refactored it.

> +    if tts > MAX_TTS:
> +        print "Error: Couldn't read partitions for a reasonable time."
> +        sys.exit(1)
> +
>      for dev_file in glob.glob("%s?*" % device):
>          device_path = _get_udisks_device_path(dev_file)
>          udisks_dev = dbus.SystemBus().get_object(UDISKS, device_path)
> @@ -516,6 +528,8 @@
>              ['parted', '-s', media.path, 'mklabel', 'msdos'], as_root=True)
>          proc.wait()
>  
> +    wait_partition_to_settle(media)
> +
>      sfdisk_cmd = board_config.get_sfdisk_cmd(
>          should_align_boot_part=should_align_boot_part)
>  
> @@ -523,11 +537,28 @@
>  
>      # Sync and sleep to wait for the partition to settle.
>      cmd_runner.run(['sync']).wait()
> -    # Sleeping just 1 second seems to be enough here, but if we start getting
> -    # errors because the disk is not partitioned then we should revisit this.
> -    # XXX: This sleep can probably die now; need to do more tests before doing
> -    # so, though.
> -    time.sleep(1)
> +    wait_partition_to_settle(media)
> +
> +
> +def wait_partition_to_settle(media):
> +    """Sleep in a loop to wait partition to settle
> +
> +    :param media: A setup_partitions.Media object to partition.
> +    """
> +    tts = 1
> +    while (tts > 0) and (tts <= MAX_TTS):
> +        try:
> +            print "Sleeping for %s second(s) to wait for the partition to settle" % tts
> +            time.sleep(tts)
> +            proc = cmd_runner.run(['sfdisk', '-l', media.path], as_root=True, stdout=open('/dev/null', 'w'))
> +            proc.wait()
> +            tts = 0
> +        except cmd_runner.SubcommandNonZeroReturnValue:
> +            print "Partition table is not available."
> +            tts += 1
> +    if tts:
> +        print "Error: Couldn't read partition table for a reasonable time."
> +        sys.exit(1)

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-12-09: Posted in a previous version of this proposal

I think there's a chance those changes won't actually fix the issue. I say that because on bug 902227 it was reported a very similar error but the user got the same error when running l-a-m-c with this branch. The interesting thing is that apparently l-m-c works fine for them, even on trunk.

I also realized that using sfdisk in wait_partition_to_settle() won't help anything as having the partition table available to sfdisk doesn't seem to guarantee that it is available to UDisks as well. I've pushed another attempt at fixing bug 717129 to lp:~salgado/linaro-image-tools/717129. With some luck it might fix bug 902227 as well

Revision history for this message

Georgy Redkozubov (gesha) wrote on 2011-12-13: Posted in a previous version of this proposal

Guilherme,

I've incorporated your fix into my branch.
sfdisk is used because we are going to create partitions with it in next step.

> I think there's a chance those changes won't actually fix the issue. I say
> that because on bug 902227 it was reported a very similar error but the user
> got the same error when running l-a-m-c with this branch. The interesting
> thing is that apparently l-m-c works fine for them, even on trunk.
>
> I also realized that using sfdisk in wait_partition_to_settle() won't help
> anything as having the partition table available to sfdisk doesn't seem to
> guarantee that it is available to UDisks as well. I've pushed another attempt
> at fixing bug 717129 to lp:~salgado/linaro-image-tools/717129. With some luck
> it might fix bug 902227 as well

review: Needs Resubmitting

Revision history for this message

Данило Шеган (danilo) wrote on 2011-12-20:

A suggested way to test this: get the entire try block split into an overrideable method that test can replace (or at least parts that we believe are affected by the problem), and ensure that we do some number of loops. Eg. 5 loops would be good enough (to ensure we don't have a race-condition in the test with the timeout of 10s), and that's something a method overridden by test could do (i.e. fail 4 times, then succeed). It would be nice to have a test that ensures failures are properly caught as well (eg. test method which fails at least 11 times).

Revision history for this message

Mattias Backman (mabac) wrote on 2011-12-20:

> A suggested way to test this: get the entire try block split into an
> overrideable method that test can replace (or at least parts that we believe
> are affected by the problem), and ensure that we do some number of loops. Eg.
> 5 loops would be good enough (to ensure we don't have a race-condition in the
> test with the timeout of 10s), and that's something a method overridden by
> test could do (i.e. fail 4 times, then succeed). It would be nice to have a
> test that ensures failures are properly caught as well (eg. test method which
> fails at least 11 times).

I agree that this is a good way to get this tested to a reasonable level. It's going to be tough to cover all possible versions of Reality that might hit this code though, so just simulating the expected cases will have to do. You could mock udisks_dev.Get() to do that but I think it will be much easier to go with Danilo's suggestion and break out the entire try block and mock that with something that can generate the wanted exception.

lp://staging/~gesha/linaro-image-tools/717129 updated on 2011-12-22