Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[enh] Rework tools_update and tools_upgrade #692

Merged
merged 24 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@alexAubin
Copy link
Member

commented Mar 22, 2019

So this is still heavy work-in-progress which needs iteration and testing, but maybe we can already discuss the principle

The problem

Current implementation of tools_update and tools_upgrade leads to many issues, among which :

  • these days, at least once a day a user comes complaining about tools_update failing (most probably issues in /etc/apt/sources.list*) but there's no proper logging or error handling to diagnose, so we gotta tell people to manually run apt update to understand wtf is going on
  • the whole "delayed upgrade" thing is a UX disaster : YunoHost/issues#1209
  • ... and a concurrency / integrity hazard because the cronjob is set to run at exactly hh:00 so what if you just ran tools_upgrade at hh:59 and now the apps are upgrading ... and in parallel the cronjob starts ?
  • it relies on the damn obscure apt python lib which to me isn't transparent about what it does.
  • Kinda independent from this : in the webadmin, clicking on 'Update the system' from the home shouldn't actually run tools_update as it's inconsistent in terms of UX : YunoHost/issues#1174
  • I'm also wondering if the current API tools_upgrade which by default upgrade both the system and apps really is relevant ... it probably dates back from days where the app update was expected to be a "light" operation but my feeling is that you really want to keep those separated ... either upgrade the system, or the apps, but not both at the same time @.@ (and that's kinda reflected in the webadmin)

Solution

Rework tools_update and tools_upgrade using shell-calls to apt directly...

Also the main issue is the delayed upgrade thing here. And for this I integrate the same principle as was used for the stretch migration, which is to launch something like os.system("apt dist-upgrade &") (note the & to launch the command in background) so that the dist-upgrade happens right away and not somewhere in the future ... That is pretty dirty and hackish but well, I wasn't able to find any better solution, and it turned out pretty well for the stretch migration ...

We loose control of stdout/stderr so those can't be displayed in the webadmin (but clearly there's no trivial answer to this issue, and currently we anyway don't show the upgrade logs anyway since they happen in a cronjob...)

PR Status

Still needs more development but it ~kinda works

How to test

Run tools_update and tools_upgrade ... the tricky part being to create / reproduce situations where something is to be upgraded

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Josue-T

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

We loose control of stdout/stderr so those can't be displayed in the webadmin.

Maybe can we redirect the output to a file. And read the result with tail ? Or read in /var/log/dpkg.log or /var/log/apt/... ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

Maybe can we redirect the output to a file. And read the result with tail ? Or read in /var/log/dpkg.log or /var/log/apt/... ?

But how do you serve that content ... the way the admin was built is that it interacts with the yunohost-api, and here we precisely don't want the yunohost-api to be doing anything (or any yunohost command to be running) so we can't do that. That's the core issue 😉

@zamentur

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

With 2 last commits I suggest to add log of "critical pakage upgrade" into the operration_logger already created. The end date is updated after the command finish and a success info is added if it's relevant.

@alexAubin alexAubin referenced this pull request Mar 27, 2019

Merged

[fix] Rework tools update (only) #695

0 of 4 tasks complete

@alexAubin alexAubin added this to the 3.6.x milestone Mar 28, 2019

@alexAubin alexAubin changed the title [heavy-wip] Rework tools_update and tools_upgrade [enh] Rework tools_update and tools_upgrade Apr 24, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

(Still needs a few adjustements but this is pretty much ready)

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Sooooo basically it should be done now

Note the related PR on webadmin : YunoHost/yunohost-admin#240

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Soooo proposing to merge in a few days

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Sooo mergin' :/

@alexAubin alexAubin merged commit 218f4ce into stretch-unstable May 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the rework-update-and-upgrade branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.