Ubuntu system image

Upgrade path calculation incorrect

Bug #1412698 reported by Michael Vogt
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Snappy
Fix Released
Critical
Barry Warsaw
Ubuntu system image
Fix Released
High
Barry Warsaw
Ubuntu system image 3.0

Bug Description

The upgrade path calculation is incorrect (as reported by asac). I can reproduce this too:
- create image
- force upgrade to ensure both sys-a,b are populated
- set version of current image to version -=1 (e.g. 244 instead of 245 if 245 is latest)
- set version of current image to version -= 2
- run "system-image-dbus -v"
- run "sudo snappy update"
- watch the log message of system-image-dbus show only a single version to upgrade instead of two (also downlods only a single delta instead of two)

Asac debugged this last night and we suspect the bug consists of two parts:
- partition.py does not point to the correct other partition system-image file due to a bug
- system-image caches the update calculcation and therefore does not recalculcate on reloadconfiguration

I attach two proof of concept patches that outline the fix. Both need work.

Tags: client

Related branches

lp://staging/~barry/ubuntu-system-image/snappy-lp1412698
Michael Vogt: Pending requested
Diff: 62 lines (+36/-0)
3 files modified
debian/changelog (+8/-0)
systemimage/dbus.py (+8/-0)
systemimage/tests/test_dbus.py (+20/-0)
lp://staging/~barry/ubuntu-system-image/citrain30beta
lp://staging/~barry/ubuntu-system-image/citrain30
Ubuntu CI managed package branches: Pending requested
Diff: 15774 lines (+8086/-2651)
138 files modified
MANIFEST.in (+1/-1)
NEWS.rst (+72/-1)
PKG-INFO (+1/-1)
cli-manpage.rst (+31/-18)
coverage-curl.ini (+21/-0)
coverage-udm.ini (+21/-0)
coverage.ini (+0/-15)
dbus-manpage.rst (+50/-28)
debian/changelog (+77/-0)
debian/control (+3/-2)
debian/rules (+15/-10)
debian/tests/control (+7/-7)
debian/tests/dryrun (+1/-1)
debian/tests/prep.py (+9/-2)
debian/tests/smoketest (+2/-2)
debian/tests/smoketest-noreboot (+2/-2)
debian/tests/unittests (+1/-1)
ini-manpage.rst (+33/-30)
setup.cfg (+1/-1)
setup.py (+1/-1)
system_image.egg-info/PKG-INFO (+1/-1)
system_image.egg-info/SOURCES.txt (+105/-59)
systemimage/api.py (+20/-10)
systemimage/apply.py (+32/-9)
systemimage/bag.py (+7/-1)
systemimage/candidates.py (+1/-1)
systemimage/channel.py (+1/-1)
systemimage/config.py (+174/-108)
systemimage/curl.py (+275/-0)
systemimage/data/client.ini (+0/-35)
systemimage/dbus.py (+100/-95)
systemimage/device.py (+1/-1)
systemimage/docs/conf.py (+1/-1)
systemimage/download.py (+119/-208)
systemimage/gpg.py (+6/-1)
systemimage/helpers.py (+53/-43)
systemimage/image.py (+5/-1)
systemimage/index.py (+2/-5)
systemimage/keyring.py (+5/-5)
systemimage/logging.py (+4/-2)
systemimage/main.py (+117/-47)
systemimage/reactor.py (+7/-4)
systemimage/scores.py (+32/-15)
systemimage/service.py (+15/-18)
systemimage/settings.py (+1/-1)
systemimage/state.py (+32/-23)
systemimage/testing/controller.py (+95/-45)
systemimage/testing/dbus.py (+22/-10)
systemimage/testing/demo.py (+6/-6)
systemimage/testing/helpers.py (+174/-62)
systemimage/testing/nose.py (+7/-2)
systemimage/testing/service.py (+0/-50)
systemimage/tests/data/00.ini (+1/-2)
systemimage/tests/data/01.ini (+2/-3)
systemimage/tests/data/api.channels_01.json (+13/-0)
systemimage/tests/data/api.index_01.json (+36/-0)
systemimage/tests/data/api.index_02.json (+251/-0)
systemimage/tests/data/api.index_03.json (+37/-0)
systemimage/tests/data/candidates.index_01.json (+6/-0)
systemimage/tests/data/candidates.index_02.json (+23/-0)
systemimage/tests/data/candidates.index_08.json (+244/-0)
systemimage/tests/data/candidates.index_10.json (+36/-0)
systemimage/tests/data/candidates.index_11.json (+37/-0)
systemimage/tests/data/candidates.index_13.json (+244/-0)
systemimage/tests/data/channel.channels_01.json (+23/-0)
systemimage/tests/data/channel.channels_02.json (+38/-0)
systemimage/tests/data/channel.channels_03.json (+70/-0)
systemimage/tests/data/channel.channels_04.json (+56/-0)
systemimage/tests/data/channel.channels_05.json (+23/-0)
systemimage/tests/data/channel_06.ini (+0/-8)
systemimage/tests/data/channel_07.ini (+0/-8)
systemimage/tests/data/config.config_01.ini (+34/-0)
systemimage/tests/data/config.config_02.ini (+34/-0)
systemimage/tests/data/config.config_03.ini (+3/-4)
systemimage/tests/data/config.config_04.ini (+36/-0)
systemimage/tests/data/config.config_05.ini (+3/-4)
systemimage/tests/data/config.config_06.ini (+3/-4)
systemimage/tests/data/config.config_07.ini (+1/-2)
systemimage/tests/data/config.config_08.ini (+1/-2)
systemimage/tests/data/config.config_09.ini (+0/-3)
systemimage/tests/data/config.config_10.ini (+27/-0)
systemimage/tests/data/config.config_11.ini (+2/-0)
systemimage/tests/data/config_04.ini (+0/-36)
systemimage/tests/data/config_09.ini (+0/-27)
systemimage/tests/data/config_10.ini (+0/-35)
systemimage/tests/data/dbus.index_03.json (+36/-0)
systemimage/tests/data/dbus.index_06.json (+37/-0)
systemimage/tests/data/download.index_01.json (+6/-0)
systemimage/tests/data/gpg.channels_01.json (+23/-0)
systemimage/tests/data/helpers.config_01.ini (+7/-0)
systemimage/tests/data/helpers.config_02.ini (+6/-0)
systemimage/tests/data/index.channels_01.json (+9/-0)
systemimage/tests/data/index.channels_02.json (+13/-0)
systemimage/tests/data/index.channels_05.json (+9/-0)
systemimage/tests/data/index.index_01.json (+251/-0)
systemimage/tests/data/index.index_04.json (+244/-0)
systemimage/tests/data/main.channels_01.json (+13/-0)
systemimage/tests/data/main.channels_03.json (+13/-0)
systemimage/tests/data/main.config_01.ini (+5/-6)
systemimage/tests/data/main.config_05.ini (+7/-0)
systemimage/tests/data/main.config_07.ini (+1/-1)
systemimage/tests/data/main.index_04.json (+36/-0)
systemimage/tests/data/main.index_05.json (+36/-0)
systemimage/tests/data/scores.index_01.json (+245/-0)
systemimage/tests/data/scores.index_05.json (+245/-0)
systemimage/tests/data/scores.index_06.json (+253/-0)
systemimage/tests/data/scores.index_07.json (+252/-0)
systemimage/tests/data/state.channels_01.json (+64/-0)
systemimage/tests/data/state.channels_02.json (+13/-0)
systemimage/tests/data/state.config_01.ini (+2/-0)
systemimage/tests/data/state.config_02.ini (+6/-0)
systemimage/tests/data/state.index_01.json (+244/-0)
systemimage/tests/data/state.index_02.json (+245/-0)
systemimage/tests/data/state.index_03.json (+36/-0)
systemimage/tests/data/state.index_04.json (+37/-0)
systemimage/tests/data/state.index_07.json (+2/-3)
systemimage/tests/test_api.py (+71/-93)
systemimage/tests/test_bag.py (+1/-1)
systemimage/tests/test_candidates.py (+37/-45)
systemimage/tests/test_channel.py (+19/-19)
systemimage/tests/test_config.py (+202/-178)
systemimage/tests/test_dbus.py (+365/-167)
systemimage/tests/test_download.py (+229/-115)
systemimage/tests/test_gpg.py (+129/-19)
systemimage/tests/test_helpers.py (+183/-171)
systemimage/tests/test_image.py (+1/-1)
systemimage/tests/test_index.py (+32/-68)
systemimage/tests/test_keyring.py (+1/-1)
systemimage/tests/test_main.py (+725/-432)
systemimage/tests/test_scores.py (+127/-15)
systemimage/tests/test_settings.py (+4/-6)
systemimage/tests/test_state.py (+244/-142)
systemimage/tests/test_winner.py (+23/-19)
systemimage/udm.py (+212/-0)
systemimage/version.txt (+1/-1)
tools/demo.ini (+0/-1)
tools/runme.sh (+10/-0)
tox.ini (+21/-16)
lp://staging/~ci-train-bot/ubuntu-system-image/system-image-ubuntu-wily-proposed
lp://staging/ubuntu/wily/system-image
lp://staging/ubuntu/wily-proposed/system-image
Revision history for this message
Michael Vogt (mvo) wrote :

This changes the code to use the actual mount_target of the other parition. The call parition.get_mount_target() gives a path that is not actually used in the partition.mount_other_rootfs() earlier. So the following code that checks if other_config exists will never see it as this mount_point is empty.

Changed in snappy-ubuntu:
importance: Undecided → Critical
Revision history for this message
Michael Vogt (mvo) wrote :

This patch changes ReloadConfiguration to also flush the "self._update state.

For some unknown reason I had to self._checking.release() first or it would hang. This needs to be understood first before the patch can be applied.

Revision history for this message
Barry Warsaw (barry) wrote : Re: [Bug 1412698] Re: Upgrade path calculation incorrect

On Jan 20, 2015, at 09:29 AM, Michael Vogt wrote:

>This patch changes ReloadConfiguration to also flush the "self._update
>state.
>
>For some unknown reason I had to self._checking.release() first or it would
>hang. This needs to be understood first before the patch can be applied.

Both make sense. I'll work on this fix.

One thing you could do in the meantime is call .Exit() and then
re-D-Bus-activate. That defeats the purpose of .ReloadConfiguration() but it
would work.

Barry Warsaw (barry)
Changed in snappy-ubuntu:
status: New → In Progress
assignee: nobody → Barry Warsaw (barry)
Revision history for this message
Barry Warsaw (barry) wrote :

CheckForUpdate() acquires the checking lock. This gets released after the update is downloaded or when CancelUpdate gets called.

I just realized that if manual downloads are enabled, the checking lock might not get released after the check completes. That could be the bug you're encountering here, since I believe snappy always disables automatic downloads.

Barry Warsaw (barry)
tags: added: client
Changed in ubuntu-system-image:
status: New → In Progress
importance: Undecided → High
assignee: nobody → Barry Warsaw (barry)
milestone: none → 3.0
status: In Progress → Invalid
Barry Warsaw (barry)
Changed in ubuntu-system-image:
status: Invalid → Triaged
Revision history for this message
Barry Warsaw (barry) wrote :

Why #4 is a problem here but not in the standard system-image.

In standard si, CheckForUpdate tries to acquire the checking lock, but it does so non-blocking. If the lock cannot be acquired, but there is cached update information already available, it will just send an UpdateAvailableStatus with the cached information. If no cached information is available, it is assumed the check is still in progress, and when *that* check completes, an UAS signal is emitted and the update information is cached. So for all intents and purposes, it doesn't matter if the checking lock is acquired.

However, when snappy's version with mvo's patch is used, it changes the internal assumptions. Let's say a CFU is called, and it completes, sending a UAS signal. However, as with the si trunk, the checking lock is not released, *and* snappy always uses manual downloads, so the release after the download completes never happens. When ReloadConfiguration invalidates the cache (by setting self._update to None), we now have a situation where the checking lock is acquired, no download or CancelUpdate is called to release the lock, *and* there is no cached update information. So from here on, no UAS signal will be emitted.

mvo should confirm, but I believe when he says in comment #2 that "I had to self._checking.release() first or it would hang", the "it" means the snappy client would hang because it would be waiting on a UAS signal that would never arrive. It's *not* the case that si-dbus would hang, at least I predict as such. :)

This then explains why mvo had to also release the checking lock after invalidating the cache. Once that's done, then next CFU will see the lock get re-acquired and will re-initiate the check, eventually leading to a UAS.

Ideally, the checking lock should probably get released after the UAS signal is emitted, but that's too risky of a change here. I'll work on that for si 3.0 though, thus the additional bug task. For the snappy version, I think mvo's patch is exactly right.

Barry Warsaw (barry)
Changed in ubuntu-system-image:
status: Triaged → Fix Committed
Michael Vogt (mvo)
Changed in snappy-ubuntu:
status: In Progress → Fix Released
Barry Warsaw (barry)
Changed in ubuntu-system-image:
status: Fix Committed → Fix Released
Michael Terry (mterry)
affects: snappy-ubuntu → snappy
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.