Check if there are updates to install before invoking `freebsd-update install' on upgrade. #1027
Conversation
iocage_lib/ioc_upgrade.py
Outdated
| rb'''\t\t\texit 1''' | ||
| exit_code_sub = \ | ||
| rb'''\t\t\techo "Run '$0 fetch' first."\n'''\ | ||
| rb'''\t\t\t#exit 1 #iocage''' |
There was a problem hiding this comment.
Hmm this is a tough problem, because if any output on their end changes, this all breaks. Parsing userland output is suboptimal, but I can't think of a better solution either. Ideally they don't continually change this script for other programs, without thinking about other consumers of it.
There was a problem hiding this comment.
Well, they changed it once in 2017 and now changed it back :). I assume this will be relatively stable (and they way this is implemented it won't break). Ideally we could add an environment variable to freebsd-update to control this behaviour (I might create a patch let's see), but this won't help versions in existence (like 11.3-RELEASE). I was a bit surprised iocage fetches it from github though and not from a direct freebsd resource (given that patches are fetched from there, it would make sense to not introduce an additional resource). Outside of the scope of this patch, but still.
There was a problem hiding this comment.
p.s. This is not parsing userland output, but modifying freebsd-update to not exit with this code. Parsing userland output would've been more unstable and in this case really hard (like parse it in python while still having full interactivity). And as this is basically just a search/replace, if the code changes in a compatible way, it will simply be a no-op.
There was a problem hiding this comment.
Something in that script we can call in the environment would be more ideal I think. As far as fetching from github, it was simply because I couldn't find a mirror of freebsd-update that would get the minor patches as the major lived.
I'm open to changing that behavior of course! So with your suggestion I think we should do the following:
- Use an env variable or flag to freebsd-update to not exit 1 on these things
- Only run this sub on 11.3-RELEASE and under.
That should help us with stability in regards to this behavior as those shouldn't change at all.
There was a problem hiding this comment.
p.s. This is not parsing userland output, but modifying freebsd-update to not exit with this code. Parsing userland output would've been more unstable and in this case really hard (like parse it in python while still having full interactivity). And as this is basically just a replace, if the code changes in a compatible way, it will simply be a no-op.
Yeah I noticed I made a mistake and this is actually the scripts content but I didn't want to edit it and make any reply look out of context :D
There was a problem hiding this comment.
I created a patch to freebsd-update and opened a bug:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239997
If approved I would modify this pull request to make use of it.
There was a problem hiding this comment.
Thanks for all the effort @grembo 👍
There was a problem hiding this comment.
@skarekrow After discussing the issue with @cperciva, I took his advice and implemented a different fix, that doesn't require patching freebsd-update on download and is backwards compatible. It looks for the symlink created by freebsd-update to determine if there is anything left to install (this is a stable enough interface to freebsd-update at this point). This also has the advantage of replacing the for loop hardcoded to three iterations with a while condition and also removing error messages from the log. I'll still look into patching freebsd-update to add a function called freebsd-update updatesready, but more as a new feature and not as a pre-condition to fix iocage. I feel like getting this in (also in the ports tree as a patch to 1.1) should be a priority, as it's actually breaking people's setup on upgrade to 11.3 at the moment.
There was a problem hiding this comment.
Agreed, this is something that has been a pain point for a lot of folks recently.
| uuid=self.uuid, | ||
| unjailed=True | ||
| ) | ||
| except iocage_lib.ioc_exceptions.CommandFailed: |
There was a problem hiding this comment.
My memory suggests we actually did snapshot the upgraded jails at one time, I can't seem to find where we do that anymore. But my suggestion here is instead of removing this code, that we actually just snapshot the jail if it's not a basejail (pluginv2 also) type. We want the upgrade to be reversible in case of failures after all.
There was a problem hiding this comment.
Jails are running during the upgrade, so if anything stateful happens during the upgrade (e.g. file upload, database change, or even just a log file entry) an automatic rollback would be disastrous for the user. I assume this is the reason why automatic snapshots/rollbacks exist for basejails (stateless) and not for jails (stateful).
As a side note, iocage is starting a jail silently (and without interacting with the user) before upgrading it. As a new user who stopped the jail before running upgrade and then finding it running during the process, this came as a bit of a surprise.
There was a problem hiding this comment.
The jail should be stopped by iocage, snapshotted, started, upgraded (anything during this time should be considered expendable, as if the upgrade fails, the user would likely prefer the state their jail was at before any changes happened) and then if success destroy snapshot.
As for it being a surprise, I understand. We need the mounts, so it's the best we got. I suppose some documentation or a print would help a user understand that clearer.
There was a problem hiding this comment.
the user would likely prefer the state their jail was at before any changes happened) and then if success destroy snapshot.
I wouldn't make that assumption, as this really depends on the use case. If the jail does anything stateful that has any sort of meaning (e.g. communicating with the outside world), I absolutely prefer to not have the state that was accumulated during an upgrade destroyed. Also if my jail setup is a bit more complicated (think ZFS mountpoints from outside the main jails containers) it would end up in quite a mess. Given that upgrades can easily take 15 minutes and longer, and that the jail is started with all services running for update (correct me if I'm wrong here), that is a lot of state to be lost. So, unless we have an enforced setup, in which the jail itself is known to be stateless - like an ideal container, where all logging and all storage lives outside of the jail - automatic rollback is asking for disaster and should be an optional feature.
There was a problem hiding this comment.
Forgive me as it's been a while since I've played around with upgrade, as @sonicaj has taken more of the direct hands on with the project lately.
I think that's a great point. Thinking about this, rollback should be an optional flag on upgrade, but in addition we should start the jail process and not the rc process, so we can ensure these things aren't happening during upgrade. You're right, this process can take quite some time depending on a variety of factors.
grembo
changed the title
There was a problem hiding this comment.
Thank you for the contribution @grembo. It's much appreciated. I had a quick query, have you tested this across different releases i.e 11.2->12 / 11.3->12 / 11.1 -> 11.2 etc ?
I just want to be sure that this behavior does not disrupt other upgrade paths ( it looks like it shouldn't but confirming it with you - also you do mention that it is backwards compatible but just being sure ;) )
iocage_lib/ioc_upgrade.py
Outdated
| silent=self.silent | ||
| ) | ||
| while os.path.islink(self.freebsd_install_link): | ||
| self.__upgrade_install__(tmp.name) |
There was a problem hiding this comment.
Is it possible that this never ends ?
There was a problem hiding this comment.
No, there is no code path in freebsd-update install that would exit on 0 and leave ${BDHASH}-install behind.
There was a problem hiding this comment.
There is, but only after freebsd-update upgrade when it wants you to reboot after it installs a new kernel / before it deletes old shared libraries.
There was a problem hiding this comment.
There is, but only after
freebsd-update upgradewhen it wants you to reboot after it installs a new kernel / before it deletes old shared libraries.
Touché - I clearly wasn't precise enough, sorry. What I meant to say is, that freebsd-update install either moves the installation forward or exits != 0. It doesn't exit on 0 and gets stuck in whatever it was doing beforehand. You can trust its exit code - it won't say "ok" if there is a problem.
Edit: At first I thought of having a counter in this code to prevent a runaway, but looking at freebsd-update it felt that all it did was making the code harder to read.
The (simplified) logic is basically:
- Check params - if there's a problem, exit 1 (unless ${BDHASH}-install doesn't exist and fetch was called, in which case it exits 0 - but this code path in unreachable in our case)
- install_files:
- If !kernel_done: install kernel, exit 1 on error. touch kernel_done; exit 0
- install other files - exit 1 on any error
- rename ${BDHASH}-install to ${BDHASH}-rollback
There was a problem hiding this comment.
Quite silly of me, as actually the whole point of this patch is to look if ${BDHASH}-install was left behind while freebsd-update install exited on 0 to know that additional invocations are required. ^_^
The point is, that, unless there are bugs introduced in future versions of freebsd-update, this loop should be safe by all means.
There was a problem hiding this comment.
@skarekrow i feel like maybe putting an upper limit here ? i.e 5 maybe ? not sure, i just don't want us to be affected by any unwanted upstream changes. @grembo would you have any leads ?
There was a problem hiding this comment.
@sonicaj If it helps: If FreeBSD-update changed in the future, this would also affect ezjail and iocell (and probably many other handcrafted solutions). The advantage iocage has over ezjail and iocell with this patch is that it can tell apart „finished“ and „something broke“ (they run “while ok”, while this code runs “while file is there, expect ok”). Besides, a looping upgrade process will certainly cause an issue being opened immediately.
In general, I’m all for defensive coding and my first instinct was to put in a runaway handler - just looking the at remainder of iocage I think it’s written to trust return codes from the various tools used.
That said, if these concerns delay the patch, I’m more than happy to add an upper limit + exception - I’m absolutely not religious about it. I would select a fairly high number like 50 iterations in this case though and fail the upgrade process once it’s reached.
There was a problem hiding this comment.
I think it's reasonable for us to rely on exit codes of our upstream projects. Which we do in a lot of cases. I'm not against defensive programming either, but like @grembo mentioned, a user will very likely notice that the script is indeed looping and kill it, and (hopefully) promptly filing an issue.
What I want to avoid is us having to low of a ceiling in case upstream changes how things are done (perhaps it becomes a wrapper to pkg base in the future -- who knows), so I agree with @grembo's suggestion of 50. That seems rather high, but it's high enough we don't worry about prematurely killing things, and low enough that the program won't spin for ages.
Regardless, this essentially replaces what we did before which was a fairly naive loop that just hoped for the best. I don't think this PR will bring us any additional risk of undesired behavior, and the fix is worth the risk imo.
So tldr; let's add a 50 iteration ceiling, that's a good enough compromise.
There was a problem hiding this comment.
@grembo can you please add a ceiling of 50 iterations ? Thank you
@skarekrow i agree with the compromise ;)
There was a problem hiding this comment.
@sonicaj I implemented a runaway check, also fixed an indentation I got wrong in the previous version and corrected a function description I stumbled upon (upgrade_install doesn't return anything any more and even before it didn't return the exit code, but True/False/None).
Okay, I did a lot of testing - unfortunately freebsd-update isn't super fast and debugging the issues found took some time as well. As expected, the patch works okay on all upgrade paths I tested, which were:
I found few other problems that way though - all of which are unrelated to the patch (I tested with a patched version of iocage 1.1):
You can find the bug and patches to the port skeleton of sysutils/iocage here: |
There was a problem hiding this comment.
@grembo thanks again for your contribution and for thoroughly testing the cases. I just have a minor query for @skarekrow, else we should be good.
iocage_lib/ioc_upgrade.py
Outdated
| silent=self.silent | ||
| ) | ||
| while os.path.islink(self.freebsd_install_link): | ||
| self.__upgrade_install__(tmp.name) |
There was a problem hiding this comment.
@grembo @cperciva thank you for clarifying. @skarekrow are you comfortable with this loop ? I am not sure how to proceed with this, if anything changes on freebsd-update end even as a bug for some time, this is going to repeat itself endlessly. If you think we are good, i am okay with it then.
`freebsd-update install' on upgrade. Since r345055[0] (first used in 11.3-RELEASE, but not 12.0-RELEASE), freebsd-update returns 1 in case no updates are available to install and fetch wasn't called first. This restored the behaviour it had until r324441 and was required to unbreak ezjail and iocell[1]. This change caused iocage to assume the update failed and kept it from updating config.json properly. This change, which I also discussed with the original author of freebsd-update, fixes this problem in a straight-forward manner that is backwards compatible (using the symbolic link is considered a stable enough interface to freebsd-update). As a bonus, it doesn't rely on a hard coded number of invocations of freebsd-update any more (which was fixed to "3" before). As part of the update, I removed dead/unreachable exception handling code that would've also done the wrong thing if it was ever reached. As part of testing this change I also tried running upgrade with interactive set to False (manually in the source), which also worked ok. It might make sense to remove non-interactive code from ioc_upgrade in the future though. [0]https://svnweb.freebsd.org/base?view=revision&revision=345055 [1]https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229346
You're welcome :) |
|
I updated the patch to the port skeleton at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240177 to match the approved version of this pull request. |
|
I'm late to the party!!!! I have a bug report on FreeBSD made by @grembo, I'm wondering if we might soon have a new iocage release (it is time), or should I add the patch on the ports till we wait for a new iocage release that will incorporate this github pr. @skarekrow and @sonicaj any plan? |
|
@araujobsd it is long overdue. We have been looking at regressions from the recent changes ( nat etc ) and i think we are stable enough now. @skarekrow do you think we should perhaps cut one later this week or at the start of the next one ? |
@araujobsd Regardless of a release I think it would make sense to apply the patch to the port, as
|
@sonicaj There is another upgrade problem caused by freebsd-update not detecting the src component correctly if BASEDIR isn't / - so if the jailhost has no src tree, src trees in iocage jails will never be updated. This seems to be a problem for quite some time now. It was attempted to patch this in freebsd-update earlier this year, but the patch didn't work due to the way options are evaluated in freebsd-update. I opened a review to fix this in base, I'm curious if we should add something in iocage to address it though (as a user, my expectation of an iocage upgrade operation would be to end up with a fully updated jail, including src). |
Well @grembo, i have already submitted a patch in github and opened up an issue in bugzilla for it. There hasn't been any interest shown by freebsd devs unfortunately to review it 😅 |
|
Looking at your diff, that seems good. How do you propose to handle this in iocage for updates ? |
I thought of adding a function in ioc_common that allows iocage to patch freebsd-update based on the release in question (similar to what I tried first here, but in a more formal way). |
I've been pushing for one for some time now ;)
|
|
@araujobsd Assuming this will be released soon and you don't change the port before the update to 1.2, please make sure you apply the port skeleton changes from the PR on update (especially the dependency on rcs57). |
Also: - Fix dependencies when upgrading <12 jails running on a 12-RELEASE jailhost (depend on `merge' from devel/rcs57). - Fix to unbreak updating multiple jails at once[1]. - Patch to `setup.py' to make `make check-plist' pass[2]. - Move `NO_ARCH' to make portlint happy See: [0]iocage/iocage#1027 and iocage/iocage@f66d9f0 [1]iocage/iocage@47d7c28#diff-134cbca4d064a61a693d1199494d24df [2]iocage/iocage#1043 PR: 240177 Approved by: araujo (maintainer timeout) git-svn-id: svn+ssh://svn.freebsd.org/ports/head@512299 35697150-7ecd-e111-bb59-0022644237b5
Also: - Fix dependencies when upgrading <12 jails running on a 12-RELEASE jailhost (depend on `merge' from devel/rcs57). - Fix to unbreak updating multiple jails at once[1]. - Patch to `setup.py' to make `make check-plist' pass[2]. - Move `NO_ARCH' to make portlint happy See: [0]iocage/iocage#1027 and iocage/iocage@f66d9f0 [1]iocage/iocage@47d7c28#diff-134cbca4d064a61a693d1199494d24df [2]iocage/iocage#1043 PR: 240177 Approved by: araujo (maintainer timeout) git-svn-id: svn+ssh://svn.freebsd.org/ports/head@512299 35697150-7ecd-e111-bb59-0022644237b5
Also: - Fix dependencies when upgrading <12 jails running on a 12-RELEASE jailhost (depend on `merge' from devel/rcs57). - Fix to unbreak updating multiple jails at once[1]. - Patch to `setup.py' to make `make check-plist' pass[2]. - Move `NO_ARCH' to make portlint happy See: [0]iocage/iocage#1027 and iocage/iocage@f66d9f0 [1]iocage/iocage@47d7c28#diff-134cbca4d064a61a693d1199494d24df [2]iocage/iocage#1043 PR: 240177 Approved by: araujo (maintainer timeout)
`freebsd-update install' on upgrade. Since r345055[0] (first used in 11.3-RELEASE, but not 12.0-RELEASE), freebsd-update returns 1 in case no updates are available to install and fetch wasn't called first. This restored the behaviour it had until r324441 and was required to unbreak ezjail and iocell[1]. This change caused iocage to assume the update failed and kept it from updating config.json properly. This change, which I also discussed with the original author of freebsd-update, fixes this problem in a straight-forward manner that is backwards compatible (using the symbolic link is considered a stable enough interface to freebsd-update). As a bonus, it doesn't rely on a hard coded number of invocations of freebsd-update any more (which was fixed to "3" before). As part of the update, I removed dead/unreachable exception handling code that would've also done the wrong thing if it was ever reached. As part of testing this change I also tried running upgrade with interactive set to False (manually in the source), which also worked ok. It might make sense to remove non-interactive code from ioc_upgrade in the future though. [0]https://svnweb.freebsd.org/base?view=revision&revision=345055 [1]https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229346
Also: - Fix dependencies when upgrading <12 jails running on a 12-RELEASE jailhost (depend on `merge' from devel/rcs57). - Fix to unbreak updating multiple jails at once[1]. - Patch to `setup.py' to make `make check-plist' pass[2]. - Move `NO_ARCH' to make portlint happy See: [0]iocage/iocage#1027 and iocage/iocage@f66d9f0 [1]iocage/iocage@47d7c28#diff-134cbca4d064a61a693d1199494d24df [2]iocage/iocage#1043 PR: 240177 Approved by: araujo (maintainer timeout)
This un-breaks updates to 11.3-RELEASE. First reported in
#992.
Since r345055[0] (first used in 11.3-RELEASE, but not
12.0-RELEASE), freebsd-update returns 1 in case no updates are
available to install and fetch wasn't called first. This
restored the behaviour it had until r324441 and was required to
unbreak ezjail and iocell[1].
This changed caused iocage to assume the update failed and kept
it from updating config.json properly.
Updated 2019-08-24:
This change, which I also discussed with the original author of
freebsd-update, fixes this problem in a straight-forward
manner that is backwards compatible (using the symbolic
link is considered a stable enough interface to freebsd-update).
As a bonus, it doesn't rely on a hard coded number of invocations
of freebsd-update any more (which was fixed to "3" before).
As part of the update, I removed dead/unreachable exception
handling code that would've also done the wrong thing if it was
ever reached.
As part of testing this change I also tried running upgrade
with interactive set to False (manually in the source),
which also worked ok. It might make sense to remove non-interactive
code from ioc_upgrade in the future though.
[0]https://svnweb.freebsd.org/base?view=revision&revision=345055
[1]https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229346
Make sure to follow and check these boxes before submitting a PR! Thank you.