Please consider merging branch reboot
of repo https://git.spwhitton.name/propellor
- Factor out reboot code in
Propellor.Property.SiteSpecific.DigitalOcean
intoPropellor.Property.Reboot
- Add
Propellor.Property.Reboot.toKernelNewerThan
. - Add
Propellor.Property.SiteSpecific.Exoscale
While I've merged this, I am unsure if Reboot.toKernelNewerThan should stop propellor from ensuring any subsequent properties.
That works if we have:
But not if the two properties are flipped. So, doesn't it follow that Sbuild.built is a buggy property?
If Sbuild.built needs a particular kernel version running, it could requires toKernelNewerThan. Then any use of Sbuild.built would make sure the right kernel is running, rebooting into it if necessary.
And, if toKernelNewerThan failed due to the right kernel version not being installed, Sbuild.built would be prevented from running. In which case, it would be fine for propellor to continue on with ensuring other unrelated properties.
readVersionMaybe was buggy; for "4.1.2" it yielded
Just (Version {versionBranch = [4], versionTags = []})
which is lacking anything but the major.I fixed it by taking the maximum of the list of all possible parses.
Thanks for taking a look at my branch, and especially for fixing my inadequately-tested
readVersionMaybe
.Sbuild.built
does not require a particular version of the kernel. It is just that the file that it generates in/etc/schroot/chroot.d
can vary depending on the kernel version running at the time thatSbuild.built
is first ensured. In particular, if the running kernel does not support overlayfs (as jessie's kernel doesn't), the lineunion-type=overlay
will be omitted from the file in/etc/schroot/chroot.d
. This rendersSchroot.overlaysInTmpfs
useless.I think it should be up to the user to apply a property like
to individual hosts, because it depends on whether they actually care about using an overlay chroot. Perhaps on an old machine they don't intend to use overlays. In my config, I do something like this:
The idea is that if I reinstall my machine from a jessie installation CD, propellor will upgrade to testing and boot to the new kernel before it builds the chroot, so I get the
union-type=overlay
line in my config.I could prepare a patch to add this information to the haddock of Sbuild.hs?
When
requires
is used as in your first example, Reboot.toKernelNewerThan does not need to throw an exception. It could just return FailedChange and then Sbuild.builtFor wouldn't get run.Your second example, as written is actually buggy. If Apt.upgraded fails for some reason, then Reboot.toKernelNewerThan never gets run, and then Sbuilt.builtFor can still run with the wrong kernel version.
The second example could instead be written thus:
Then if any part of the upgrade fails the following properties don't run thanks to
combineProperties
. And here too Reboot.toKernelNewerThan does not need to thow an exception.So, I'm not seeing any good use cases for it throwing an exception in these examples.
It might also be worth making the Sbuild properties know whether overlays are desired. Then they could make sure to set up the config file with the needed lines, even if the wrong kernel is running.
I assume schroot fails to work in that configuration, so the properties for it would fail and then the user would notice they need to add a property to get a new enough kernel version..
It could be specified with another parameter to the Sbuild properties. Or, you could add a pure Info property
useOverlays
and have the Sbuild properties check if the Info has that set. This would also let Schroot.overlaysInTmpfs require useOverlays and auto-enable them.Most of the implementation of that:
I like the idea of a
useOverlays
info property. It is better, and more in the spirit of propellor, to have the choice explicit, rather than implicitly relying on the behaviour of certain shell commands in certain conditions (relying on sbuild-createchroot(1) to create the config file in /etc/schroot/chroot.d is the thing I like least about Sbuild.hs, though it's necessary for achieving the goal of having a totally standard Debian sbuild setup).Before I implement this, I have a couple of questions:
In the case where
Reboot.toKernelNewerThan
finds a satisfactory kernel to reboot to, what do you think about the choice of rebooting immediately or at the end of the current propellor run? If every property that needs the newer kernelrequires
it, it would mean that other properties that don't need the newer kernel get ensured sooner. Not sure whether this is actually an advantage, but it might encourage usingrequires
instead of relying on implicit ordering.You suggest relying on schroot(1) and sbuild-createchroot(1) failing if
union-type=overlay
is present in the config file but the kernel doesn't support overlays. I'd prefer to go further and have the sbuild properties conditionallyrequires
Reboot.toKernelNewerThan
if the info property is set. That way, we can be confident that we'll never get an inconsistent state out of the sbuild properties. Does this sound sensible?If Reboot.toKernelNewerThan doesn't reboot right away, then when a property
requires
it, the property's code is not guaranteed to run under the new kernel. So, an immediate reboot seems to make sense.Making the sbuild properties automatically include Reboot.toKernelNewerThan seems reasonable.
Please consider merging my new
reboot
branch which addresses the discussion we've had.I also included some other improvements to
Sbuild.hs
, a bug fix inCcache.hs
and some GHC 7.6 compatibility fixes. With one exception,[1] I think that the changes are sufficiently self-explanatory thatgit diff master..spwhitton/reboot
will be enough for you to review the branch. If not, I will happily split the commits into several branches.[1] I changed the haddocks on some functions in Sbuild.hs so that they will be properly hyperlinked, and did some other documentation rearrangements.
FĂ©lix sent some patches today fixing compiling Propellor.Exception on old ghc, which overlap with part of your patch. You addressed the same problem in different ways. Since I already merged his (more extensive I think) fixes for that, your branch will need to be updated.
The only thing I caught during review is that the documentation for useOverlays says that the property has to be added before Sbuild.builtFor, but actually info-setting properties set info before any properties run, so can safely appear after properties that use the info they set!
(I'm not sure if overlaysInTmpfs can safely come after Sbuild.builtFor, but if it cannot it's not due to setting useOverlays.)
Also, it would be good to have some lines to add to the changelog about the sbuild changes.
Ah, very nice
I reverted my GHC 6 commits and the merge with your master branch is now clean.
Some changelog text you can use:
aliases=UNRELEASED,sid...
lines to more than one schroot config. It will not remove any such lines that the previous version of propellor added, though.