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

Rework system upgrade check #276

Closed
wants to merge 9 commits into from

Conversation

antonlacon
Copy link
Contributor

This does a minor cleanup, and then enhances how the addon checks for LE system upgrades.

The cleanup is to convert from oe.dbg_log() to log.log() when writing to kodi's log.

The change to the update check is as follows:

Submit information on device and statistics (if enabled).
Discard what server responds with for an update file.
Read LE's releases.json
If a device is running a development version (eg self-built), don't bother checking anything
If there is a bugfix release available (eg 10.0.1 -> 10.0.2), then show an notification (if enabled) and perform an an automatic upgrade (if enabled)
If there is a major/minor upgrade (9.0.x -> 9.2.x or 9.0.x -> 10.x), then show a notification (if enabled)

A device needs to be caught up on bugfix releases before it will notify about a major upgrade.

I've done some testing with how an RPi would update via version spoofing (my device is on self-built). I need feedback on what update file a uboot device will use. Will a LibreELEC-A64.arm-10.0.2.tar file upgrade any A64 device, for example?

Some further cleanup may be possible, but I need to know it's functionally complete before I try to reduce it.

@CvH CvH requested a review from mglae June 4, 2022 05:10
@CvH CvH marked this pull request as ready for review June 4, 2022 05:10
@mglae
Copy link
Contributor

mglae commented Jun 12, 2022

This replaces how the addon checks for updates. It will still submit
statistics (if enabled), but will not pay attention to the response
with the location and filename of an upgrade file.

Instead, the addon will read releases.json to determine what upgrade
file it should use. [...]

a) Is this the only change of the update algorithm? If not can you summarize the old behavior?

b) I'm suspecting returning the update file is the mechanism to delay autoupdate of a new release a few days. Maybe @CvH do know? If yes I doubt this should be dropped.

I need feedback on what update file a uboot device will use. Will a LibreELEC-A64.arm-10.0.2.tar file upgrade any A64 device, for example?

On Generic we never touch the boot configuration. Therefore I assume "Yes" for other architectures too. Please correct me if I'm wrong.

@CvH
Copy link
Member

CvH commented Jun 12, 2022

I have not too much insight, as soon I change the value to what version it should update it start updating.
Afaik it uses the given filename too (I fat fingered a wrong filename once and fubar happened), so as soon I change it at the backend it start updating.

image

And we need control about the auto update. We need to disable it far too often in the past.


bugfix_filename = release_data[f"{oe.DISTRIBUTION}-{oe.VERSION_ID}"]['project'][oe.ARCHITECTURE]['releases'][highest_device_release]['file']['name']
# assumes filename format is "distribution-device.arch-version.tar"
bugfix_filename_version = bugfix_filename.split("-")[2].rstrip(".tar")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rstrip() is stripping all characters of the string (yes, strip() is misused in the current update code too). We may need to add something like rchop().

@chewitt
Copy link
Member

chewitt commented Jun 12, 2022

In the current workflow the back-end doesn't reply with an update file until we add the release filenames and their min/max versions to the releases table in the DB. This is the step we consistently forget when making releases that results in long waits for auto-updating to happen. The goal here is to make the update check a pure "stats" push and move all update logic client-side into the settings add-on. To replicate a delay in auto-update we probably need to add "date" info into the JSON manifest so the add-on can detect file/release dates and enforce a canary period, i.e. so we do not notify about the update until date+7 (days) has elapsed. I'd probably hard-code the canary period instead of making it configurable. Users that want to skip the canary period and risk the bug we missed in pre-release testing can always switch to manual updates and select the update earlier if they want to.

@chewitt
Copy link
Member

chewitt commented Jun 12, 2022

NB: The primary motivation for moving update logic client-side is to decommission the GUI parts of the udpate server. It was created in 2012 and uses frameworks that are now dangerously outdated (with bugs) and nobody really knows how it works to update it. As security depends on obscurity we have never publicly released the code for it. For security this is bad, and we are supposed to be promoting "open" source, so closed-source bits are bad too.

@antonlacon
Copy link
Contributor Author

Just a rebase after the manual update changes; still needs work.

This replaces how the addon checks for updates. It will still submit
statistics (if enabled), but will not pay attention to the response
with the location and filename of an upgrade file.

Instead, the addon will read releases.json to determine what upgrade
file it should use. Its priority is to first upgrade to the latest
bugfix release within the current update channel (eg 9.2.5 -> 9.2.6
would occur before 9.2.5 -> 10.02). Bugfixes within the same
channel are also the only upgrades that will occur automatically if
enabled.

When upgrading major versions (9.2.x -> 10.0.x), the addon will send
an on screen notification, but will not take any steps to automatically
upgrade.

Note: examples are only examples. The addon will not run on LE 9.2.

Signed-off-by: Ian Leonard <[email protected]>
Mostly consistent use of type of quotes

Signed-off-by: Ian Leonard <[email protected]>
@antonlacon
Copy link
Contributor Author

This has "it should work" untested changes for when the stable releases.json gets updated with the new fields (that's also in the "probably works" category, but was tested against the nightly infra instead of release so may have an issue lurking). This will error without that. Commits need to be squashed and a cleanup done once tested.

This has a 7-day period between when the update tar gets timestamped and when the addon will consider it. It will not take into account timezone differences. If that's a concern, I'd rather just make it an 8-day window then add those to the mix.

Between os-releases's VERSION and VERSION_ID, it should be possible to update from nightly -> stable. That's intended for a separate PR, unless the current update mechanism is capable of doing it. I don't plan on doing devel -> stable, on the idea that said images are from builders and they're using a devel image on purpose.

@antonlacon
Copy link
Contributor Author

Upgrade logic moved to a standalone script: LibreELEC/LibreELEC.tv#7540

Revisit after merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants