[Merge] lp:~sil2100/ubuntu-release-upgrader/snap-size-estimation into lp:ubuntu-release-upgrader
Brian Murray
brian at ubuntu.com
Mon Aug 12 20:30:19 UTC 2019
Review: Needs Fixing
Thanks for working on this! You'll find some specific comments in line.
Diff comments:
>
> === modified file 'DistUpgrade/DistUpgradeQuirks.py'
> --- DistUpgrade/DistUpgradeQuirks.py 2019-06-24 18:16:09 +0000
> +++ DistUpgrade/DistUpgradeQuirks.py 2019-08-09 13:15:45 +0000
> @@ -439,51 +449,66 @@
> if not res:
> self.controller.abort()
>
> + def _calculateSnapSizeRequirements(self):
> + import json
> + import urllib.request
Don't forget to update the debian/control file with these deps if they are missing. Actually that would require an SRU to the release from which somebody is upgrading so 18.04 and 19.04.
> + from urllib.error import URLError
> +
> + # first fetch the list of snap-deb replacements that will be needed
> + # and store them for future reference, along with other data we'll
> + # need in the process
> + self._prepare_snap_replacement_data()
> + # now perform direct API calls to the store, requesting size
> + # information for each of the snaps needing installation
> + self._view.updateStatus(_("Calculating snap size requirements"))
> + for snap, snap_object in self._snap_list.items():
> + if snap_object['command'] != 'install':
> + continue
> + action = {
> + "instance-key": "upgrade-size-check",
> + "action": "download",
> + "snap-id": snap_object['snap-id'],
> + "channel": "stable/ubuntu-%s" % self._to_version,
> + }
> + data = {
> + "context": [],
> + "actions": [action],
> + }
> + req = urllib.request.Request(
> + url='https://api.snapcraft.io/v2/snaps/refresh',
> + data=bytes(json.dumps(data), encoding='utf-8'))
> + req.add_header('Snap-Device-Series', '16')
Should the '16' be hard coded i.e. might the number need to change later on?
> + req.add_header('Content-type', 'application/json')
> + req.add_header('Snap-Device-Architecture', self.arch)
> + try:
> + response = urllib.request.urlopen(req).read()
> + info = json.loads(response)
> + size = int(info['results'][0]['snap']['download']['size'])
> + except (KeyError, URLError, ValueError):
> + logging.debug("Failed fetching size of snap %s" % snap)
> + continue
> + # XXX: Should we substract the deb size?
> + self.extra_snap_space += size
No, because they do not get removed until after the upgrade is completed so there will need to be space for the deb and snap at the same time.
> +
> def _replaceDebsWithSnaps(self):
> - di = distro_info.UbuntuDistroInfo()
> - try:
> - fromVersion = \
> - di.version('%s' % self.controller.fromDist).split()[0]
> - toVersion = di.version('%s' % self.controller.toDist).split()[0]
> - # Ubuntu 18.04's python3-distro-info does not have version
> - except AttributeError:
> - fromVersion = next((r.version for r in di.get_all("object")
> - if r.series == self.controller.fromDist),
> - self.controller.fromDist).split()[0]
> - toVersion = next((r.version for r in di.get_all("object")
> - if r.series == self.controller.toDist),
> - self.controller.toDist).split()[0]
> """ install a snap and mark its corresponding package for removal """
> # gtk-common-themes isn't a package name but is this risky?
> - snaps = ['core18', 'gnome-3-28-1804', 'gtk-common-themes',
> - 'gnome-calculator', 'gnome-characters', 'gnome-logs',
> - 'gnome-system-monitor']
> - self._view.updateStatus(_("Checking for installed snaps"))
> - for snap in snaps:
> - # check to see if the snap is already installed
> - snap_info = subprocess.Popen(["snap", "info", snap],
> - universal_newlines=True,
> - stdout=subprocess.PIPE).communicate()
> - self._view.processEvents()
> - if re.search("^installed: ", snap_info[0], re.MULTILINE):
> - logging.debug("Snap %s is installed" % snap)
> - # its not tracking the release channel so don't refresh
> - if not re.search("^tracking:.*ubuntu-%s" % fromVersion,
> - snap_info[0], re.MULTILINE):
> - logging.debug("Snap %s is not tracking the release channel"
> - % snap)
> - continue
> - command = 'refresh'
> + self._view.updateStatus(_("Processing snap replacements"))
> + # _snap_list should be populated by the earlier
> + # _calculateSnapSizeRequirements call.
> + for snap, snap_object in self._snap_list.items():
> + command = snap_object['command']
> + if command == 'refresh':
> self._view.updateStatus(_("refreshing snap %s" % snap))
> else:
> - command = 'install'
> self._view.updateStatus(_("installing snap %s" % snap))
I don't know why "installing" and "refreshing" isn't capitalized here but it seems inconsistent. Could you fix that too?
> try:
> self._view.processEvents()
> - proc = subprocess.run(["snap", command, "--channel",
> - "stable/ubuntu-%s" % toVersion, snap],
> - stdout=subprocess.PIPE,
> - check=True)
> + proc = subprocess.run(
> + ["snap", command, "--channel",
> + "stable/ubuntu-%s" % self._to_version, snap],
> + stdout=subprocess.PIPE,
> + check=True)
> self._view.processEvents()
> except subprocess.CalledProcessError:
> logging.debug("%s of snap %s failed" % (command, snap))
>
> === modified file 'tests/test_quirks.py'
> --- tests/test_quirks.py 2018-08-31 22:46:45 +0000
> +++ tests/test_quirks.py 2019-08-09 13:15:45 +0000
> @@ -229,6 +328,108 @@
> self.assertEqual(pkgname, "linux-generic-lts-quantal")
>
>
> +class TestSnapQuirks(unittest.TestCase):
> +
> + @mock.patch("subprocess.Popen", MockPopenSnap)
> + def test_prepare_snap_replacement_data(self):
> + # Prepare the state for testing
> + controller = mock.Mock()
> + controller.fromDist = 'disco'
> + controller.toDist = 'eoan'
> + config = mock.Mock()
> + q = DistUpgradeQuirks(controller, config)
> + # Call method under test
> + q._prepare_snap_replacement_data()
> + self.assertEqual(q._from_version, '19.04')
> + self.assertEqual(q._to_version, '19.10')
> + # Check if the right snaps have been detected as installed and
> + # needing refresh and which ones need installation
> + self.assertDictEqual(
> + q._snap_list,
> + {'core18': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-3-28-1804': {'command': 'install', 'snap-id': '1234'},
> + 'gtk-common-themes': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-calculator': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-characters': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-logs': {'command': 'refresh'},
> + 'gnome-system-monitor': {'command': 'refresh'}})
> +
> + @mock.patch("DistUpgrade.DistUpgradeQuirks.get_arch")
> + @mock.patch("urllib.request.urlopen")
> + def test_calculate_snap_size_requirements(self, urlopen, arch):
> + # Prepare the state for testing
> + arch.return_value = 'amd64'
> + controller = mock.Mock()
> + config = mock.Mock()
> + q = DistUpgradeQuirks(controller, config)
> + # We mock out _prepare_snap_replacement_data(), as this is tested
> + # separately.
> + q._prepare_snap_replacement_data = mock.Mock()
> + q._snap_list = {
> + 'test-snap': {'command': 'install', 'snap-id': '2'},
> + 'gnome-calculator': {'command': 'install', 'snap-id': '1'},
> + 'gnome-system-monitor': {'command': 'refresh'}
> + }
> + q._to_version = "19.10"
> + # Mock out urlopen in such a way that we get a mocked response based
> + # on the parameters given but also allow us to check call arguments
> + # etc.
> + urlopen.side_effect = mock_urlopen_snap
> + # Call method under test
> + q._calculateSnapSizeRequirements()
> + # Check if the size was calculated correctly
> + self.assertEqual(q.extra_snap_space, 6218880)
> + # Check if we only sent queries for the two command: install snaps
> + self.assertEqual(urlopen.call_count, 2)
> + # Make sure each call had the right headers and parameters
> + for call in urlopen.call_args_list:
> + req = call[0][0]
> + self.assertIn(b"stable/ubuntu-19.10", req.data)
> + self.assertDictEqual(
> + req.headers,
> + {'Snap-device-series': '16',
> + 'Content-type': 'application/json',
> + 'Snap-device-architecture': 'amd64'})
> +
> + @mock.patch("subprocess.run")
> + def test_replace_debs_with_snaps(self, run_mock):
> + controller = mock.Mock()
> + config = mock.Mock()
> + q = DistUpgradeQuirks(controller, config)
> + q._snap_list = {
> + 'core18': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-3-28-1804': {'command': 'install', 'snap-id': '1234'},
> + 'gtk-common-themes': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-calculator': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-characters': {'command': 'install', 'snap-id': '1234'},
> + 'gnome-logs': {'command': 'refresh'},
> + 'gnome-system-monitor': {'command': 'refresh'}
> + }
> + q._to_version = "19.10"
> + q._replaceDebsWithSnaps()
> + # Make sure all snaps have been handled
> + self.assertEqual(run_mock.call_count, 7)
> + snaps_refreshed = set()
> + snaps_installed = set()
> + # Check if all the snaps that needed to be installed were installed
> + # and those that needed a refresh - refreshed
> + for call in run_mock.call_args_list:
> + args = call[0][0]
> + if args[1] == 'install':
> + snaps_installed.add(args[4])
> + else:
> + snaps_refreshed.add(args[4])
> + self.assertSetEqual(
> + snaps_refreshed,
> + {'gnome-logs', 'gnome-system-monitor'})
> + self.assertSetEqual(
> + snaps_installed,
> + {'core18', 'gnome-3-28-1804', 'gtk-common-themes',
> + 'gnome-calculator', 'gnome-characters'})
> + # Make sure we marked the replacing ones for removal
s/replacing/replaced/.
> + self.assertEqual(controller.forced_obsoletes.append.call_count, 5)
Is 5 the right number here? Not every snap being installed is replacing a deb.
> +
> +
> if __name__ == "__main__":
> import logging
> logging.basicConfig(level=logging.DEBUG)
--
https://code.launchpad.net/~sil2100/ubuntu-release-upgrader/snap-size-estimation/+merge/371120
Your team Ubuntu Core Development Team is subscribed to branch lp:ubuntu-release-upgrader.
More information about the Ubuntu-reviews
mailing list