Fix updater [WIP] #1241

0pcom · 2022-06-01T23:58:45Z

Did you run make format && make check?
yes

The initial work adds a much abbreviated update process in place of the existing one, without changing much.

Needed / Proposed Changes:

Remove version checks completely
prompt if repo configuration is needed
otherwise, update without prompting or checking the version, via apt

Version Checks

apt update && apt install skywire-bin will update skywire to the latest version or install it if it is not installed. Packages are not re-installed, by default, with apt install (correct me if I am wrong).

The version checking is entirely unnecessary, all of it. Yet they occupy a substantial part of the code that I was not able to remove it without breaking things.

Prompt for repository configuration

If the repository used for updating the .deb packages is not already configured in the software sources, it is highly abnormal to preform this configuration without the explicit consent of the user.

Secondly, the way this configuration was previously done was no longer congruent with the skybian images (on my fork / in my open PR), and actually conflicting with that (wouldn't detect the existing config and then would redundantly set it in a different file).

This would be the open question - the repository configuration in my skybian fork is provided by the skybian package, which is architecture-specific to arm architectures. This repository configuration could be made a seprate .deb package (and could include the repository signing key) and would then be able to work on amd64. There is no real reason not to do this, from my perspective, and the advantages for testing should be obvious.

So there needs to be a prompt to preform this configuration ; a .deb package would be downloaded from the archive of the apt repo server, and then upon it's installation, skywire-bin would be available to install via apt

otherwise update without prompting

Clicking the update button should either not prompt the user for confirmation if the repository is set, or it should only give a generic "are you sure you want to install / update?" confirmation dialog.

The visor should restart at the end of the update

Testing

requires:

visor is run as root
.deb based distro

git clone https://github.com/the-skycoin-project/skywire
git checkout fix-updater
make run-source-sudo

open http://127.0.0.1:8000
go to the visor page
click update from the drop-down menu
ui prompt for adding the repository configuration (installing the package described earlier)
click yes, software sources are configured as they would be in skybian
skywire is installed

So in theory, when this can take place, the update button will also function as the installer or a wrapper for the process which installs it, and that should be the easiest thing to test

Still needed

In order for the above outlines process to be robust in terms of using the key that the visor was already running with, when it is being installed by the updater button the key should be passed via environmental variable to skkywire-autoconfig, which will further pass it to the config gen command.

Additionally, the autoconfig script assumes skywire was already running as a systemd service, so there should be some thought given to the transition between the visor running from the source code to the visor running from the installation.

jdknives · 2022-06-02T10:00:47Z

pkg/util/updater/updater.go

@@ -86,60 +88,64 @@ const (
 // NOTE: Update may call os.Exit.
 func (u *Updater) Update(updateConfig UpdateConfig) (updated bool, err error) {
 	if !atomic.CompareAndSwapInt32(&u.updating, 0, 1) {
-		return false, ErrAlreadyStarted
+		return false, errors.New("updating already started")


Why are we replacing this with an inline error?

I had taken out most of the code in that file and several others, until the point it wouldnt work, so i copied the one function i modified and then git reset.

most of the errors already declared will be taken out, based on my proposal.

i will change it to non-inline errors before all is said and done.

jdknives · 2022-06-02T10:01:42Z

pkg/util/updater/updater.go

-	// No update is available.
-	if version == "" {
-		return false, nil
+		u.status.Set("failed checking permissions")


Is this status we set logged later down the line by the calling function?

jdknives · 2022-06-02T10:04:16Z

pkg/util/updater/updater.go

-	u.status.Set(fmt.Sprintf("Installing Skywire %q", version))
-	if err := u.aptInstall(); err != nil {
-		return false, err
+	if si.OS.Vendor == "arch" {


We are not going to implicitly support arch here in the binaries. Please take this out.

This i will need to implement for testing. None of the code i added for updating .deb was tested yet because i am writing it on arch.

I can remove it before this gets merged or we can discuss more of an appropriate context for this, but if i am able to implement this, it would let me much more easily test the updater on the system it is being developed on.

all of the .deb packages, everything i have done, was first done on arch and tested there and then re-implemented for debian.

it may not be able to be implemented, because of how permissions need to be handled for invoking yay on archlinux, but i need to try.

jdknives · 2022-06-02T10:05:15Z

pkg/util/updater/updater.go

+		u.status.Set("Checking for available package")
+		available, err := script.Exec(`apt-cache search skywire-bin`).String()
+		if err != nil {
+			return false, err


I would maybe use fmt.Errorf here so we can add some additional context to the error. Same goes for below errors that stem from executing apt commands.

jdknives · 2022-06-02T10:07:24Z

pkg/util/updater/updater.go

+		}
+		u.log.Info("installed skywire-bin")
+		u.log.Info("updating config and restarting visor")
+		if _, err := script.Exec(`DMSGPTYTERM=true skywire-autoconfig`).String(); err != nil {


Will this run if we run apt-update before successfully?

jdknives · 2022-06-02T10:26:55Z

An additional comment from my side. Instead of checking for the architecture, I would like to just check for the presence of the Skybian build tag. If it is set, we update. Otherwise we do not. Anything that is not a Skybian system can and should be handled differently, namely through the user running apt or pacman commands for Linux and just getting the latest installer for MacOS and Windows. This should be cut back to be an updating convenience feature specifically for the boards we support and know of. In all other scenarios, the updating should not be handled by the visor, since the much better solution is to let the user handle updating manually.

0pcom · 2022-06-02T12:32:05Z

An additional comment from my side. Instead of checking for the architecture, I would like to just check for the presence of the Skybian build tag. If it is set, we update. Otherwise we do not. Anything that is not a Skybian system can and should be handled differently, namely through the user running apt or pacman commands for Linux and just getting the latest installer for MacOS and Windows. This should be cut back to be an updating convenience feature specifically for the boards we support and know of. In all other scenarios, the updating should not be handled by the visor, since the much better solution is to let the user handle updating manually.

there is no architecture check that i know of. I am checking the linux distro, which should be at least additionally checked along with the build tag.

In the second place, the same binaries are in the same packages used by the images in the same repo used by anyone who installs skywire-bin. The arm binaries could have the skybian build tag, but this wont differentiate a skybian installation from the same package installed outside the skybian image.

its possible to switch on the skybian env instead of the build tag.

in the second place, if the updater is right, it should be available as part of every package equally (no reason not to) or otherwise there needs to be a carve-out that it will work on amd64 for testing.

if its not equally implemented and available for updating all the architecture .deb packages which are available, then we need to take out the updater completely and move it to skybian.

it should be separate anyways, i think, but its about to work correctly with a few more changes.

no objections to removing all the version checks or otherwise the outline of the update procedure?

0pcom · 2022-06-03T07:51:43Z

closing this PR in favor of #1244 after discussion with @jdknives

0pcom added 2 commits June 1, 2022 18:00

initial changes to core update functionality

a4047da

make format check

230942e

jdknives approved these changes Jun 2, 2022

View reviewed changes

This was referenced Jun 2, 2022

Remove updater #1243

Closed

remove updater #1244

Closed

0pcom closed this Jun 3, 2022

0pcom deleted the fix-updater branch June 3, 2022 07:52

0pcom mentioned this pull request Jun 9, 2022

remove updater #1251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix updater [WIP] #1241

Fix updater [WIP] #1241

0pcom commented Jun 1, 2022

jdknives Jun 2, 2022

0pcom Jun 2, 2022

jdknives Jun 2, 2022

jdknives Jun 2, 2022

0pcom Jun 2, 2022

jdknives Jun 2, 2022

0pcom Jun 2, 2022

jdknives Jun 2, 2022

0pcom Jun 2, 2022

jdknives commented Jun 2, 2022

0pcom commented Jun 2, 2022

0pcom commented Jun 3, 2022

Fix updater [WIP] #1241

Fix updater [WIP] #1241

Conversation

0pcom commented Jun 1, 2022

Version Checks

Prompt for repository configuration

otherwise update without prompting

Testing

Still needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdknives commented Jun 2, 2022

0pcom commented Jun 2, 2022

0pcom commented Jun 3, 2022