Merge lp://qastaging/~sil2100/ubuntu-release-upgrader/snap-size-estimation into lp://qastaging/ubuntu-release-upgrader

From the stylistic nitpicks for instance, I really had a strong urge to just call _prepare_snap_replacement_data() in both _calculateSnapSizeRequirements() and _replaceDebsWithSnaps(), and simply make it to exit early if self._snap_list is already non-empty. But then it would be non-optimal, since we know _replaceDebsWithSnaps will never be called without _calculateSnapSizeRequirements running first. But eh, I somehow couldn't make this look pretty, sorry...

Looking at DistUpgradeQuirks.py's run function we can see that there are currently 6 different quirk handlers that are supported. Is there a reason you didn't use an existing one and created "PreCalcDistUpgrade"? If there is a good reason then I think PreCalcDistUpgrade should be documented with the other ones.

I think it'd also be good if DistUpgradeCache.py were to log that the not enough free space error in /var was due to the fact that extra_snap_space was size X.

Thanks for working on this! You'll find some specific comments in line.

Thanks for the review! I will address all the issues you have outlined, with comments to a few of them (not including those inline as the diff will change and it's always a bit more annoying to switch to the old revision to see the comments):

1) Regarding PreCalcDistUpgrade: actually I see that I can just do the same in PostInitialUpdate, not sure why those weeks ago I didn't find that one appropriate. I just know any of the later ones would not do, as it has to happen early in the upgrade process. One plus side of having a PreCalcDistUpgrade was that it all felt a bit more consistent, as the size calculation was done right before the dist-upgrade was 'calculated'. I changed it to be done as part of PostInitialUpdate now which is also fine, but the only downside is that now very early users might be seeing 'Calculating snap size requirements', which might seem a bit out-of-place. On the other hand, that doesn't actually matter much I'd say!

2) Regarding changing the 'installing ...' strings as you requested - won't that break translations? I mean, I'm usually very reluctant in changing translatable strings without a valid reason. Guess this particular change won't be backported to bionic though, so maybe it's actually safe. Anyway, I guess I'd recommend changing those in a separate MP/commit.

3) Regarding Snap-Device-Series: for now this is a constant that I think can just stay hard-coded. One might think it's related to the core base (16, 18 etc.) - that would be logical, right? But actually it's not. I was confused about that as well, but the snapd team told me this series is actually unrelated to the snap base but is instead like a snap-specific standard version which should be only bumped when the standard changes, which for now is still constant. So this argument is '16' for both core16, core18 and possibly the upcoming core20.

4) Regarding the count of 5 in the test for replacing debs with snaps: I guess it feels correct, because basically currently as well as previously when determining which debs need to be exchanged with snaps, at the stage of when the quirk is running there was never any logic to check if we're replacing a real deb or not. So for instance 'gtk-common-themes', which is not a real deb package, in the case when its snap was not installed on the system, would anyway mark it for installation and put it into forced_obsoletes. The logic here didn't change. What the unit test checks is only if the 'refresh' snaps are refreshed and if the 'install' snaps are installed (and, per the existing logic, added to the forced_obsoletes).
I can only assume that the upgrader actually checks if the packages in forced_obsoletes are actual packages that need acting on somewhere later on.

5) Regarding the new dependencies and adding those to debian/control: I think no new dependencies need to be added. Both the json and urllib modules are part of the python standard library, so they basically come from the libpython3.*-stdlib etc. packages, and those are explicit dependencies of python3.*. So I guess we should be safe here, right?

Was also looking into making DistUpgradeCache mentioning the not enough space being because of extra_snap_space, but from what I see the way it's written right now feels very generic and not sure if I should change that. But I'll look into it a bit more.

> Thanks for the review! I will address all the issues you have outlined, with
> comments to a few of them (not including those inline as the diff will change
> and it's always a bit more annoying to switch to the old revision to see the
> comments):
>
> 1) Regarding PreCalcDistUpgrade: actually I see that I can just do the same in
> PostInitialUpdate, not sure why those weeks ago I didn't find that one
> appropriate. I just know any of the later ones would not do, as it has to
> happen early in the upgrade process. One plus side of having a
> PreCalcDistUpgrade was that it all felt a bit more consistent, as the size
> calculation was done right before the dist-upgrade was 'calculated'. I changed
> it to be done as part of PostInitialUpdate now which is also fine, but the
> only downside is that now very early users might be seeing 'Calculating snap
> size requirements', which might seem a bit out-of-place. On the other hand,
> that doesn't actually matter much I'd say!

Great, thanks.

> 2) Regarding changing the 'installing ...' strings as you requested - won't
> that break translations? I mean, I'm usually very reluctant in changing
> translatable strings without a valid reason. Guess this particular change
> won't be backported to bionic though, so maybe it's actually safe. Anyway, I
> guess I'd recommend changing those in a separate MP/commit.

That's fair and not a big deal really.

> 3) Regarding Snap-Device-Series: for now this is a constant that I think can
> just stay hard-coded. One might think it's related to the core base (16, 18
> etc.) - that would be logical, right? But actually it's not. I was confused
> about that as well, but the snapd team told me this series is actually
> unrelated to the snap base but is instead like a snap-specific standard
> version which should be only bumped when the standard changes, which for now
> is still constant. So this argument is '16' for both core16, core18 and
> possibly the upcoming core20.

Alright, thanks for explaining this!

> 4) Regarding the count of 5 in the test for replacing debs with snaps: I guess
> it feels correct, because basically currently as well as previously when
> determining which debs need to be exchanged with snaps, at the stage of when
> the quirk is running there was never any logic to check if we're replacing a
> real deb or not. So for instance 'gtk-common-themes', which is not a real deb
> package, in the case when its snap was not installed on the system, would
> anyway mark it for installation and put it into forced_obsoletes. The logic
> here didn't change. What the unit test checks is only if the 'refresh' snaps
> are refreshed and if the 'install' snaps are installed (and, per the existing
> logic, added to the forced_obsoletes).
> I can only assume that the upgrader actually checks if the packages in
> forced_obsoletes are actual packages that need acting on somewhere later on.

That makes sense, maybe its worth adding a comment about why it is 5 though?

> 5) Regarding the new dependencies and adding those to debian/control: I think
> no new dependencies need to be added. Both the json and urllib modules are
> part of the python standard library, so the...

This look good to me now, but as I mentioned on irc I'd expect the size calculation to fail for the core18 snap because there is no stable/ubuntu-19.10 channel for core18. It'd be good to test that this is really failing before fixing bug 1841225.

I did some testing on a VM and real upgrade scenarios and with this branch and the core18-stable follow up one, everything seems to work as expected. Let me merge both in this case.