tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.99k stars 250 forks source link

Reinstallation using provided scripts no longer possible #1662

Closed cghague closed 9 months ago

cghague commented 10 months ago

We recently switched to using Debian packaging, and as a result, simply rerunning the standard installation commands doesn't result in a complete reinstallation as it did previously. This issue occurs because apt won't reinstall the Debian packages if they are already at the latest version.

Implications

This change in behavior means some configuration changes are only picked up on a fresh installation or an upgrade. For example, switching between a USB HDMI capture device and a TC358743 HDMI capture device is no longer possible by simply modifying /boot/config.txt and reinstalling TinyPilot.

Example 1: Switching capture devices fails

Steps to reproduce

  1. Start with TinyPilot already configured for a USB HDMI adapter.
  2. Follow the instructions for using a TC358743 chip.

Actual outcome

TinyPilot remains configured for a USB HDMI adapter, which can be seen as the symlink target of /opt/ustreamer-launcher/configs.d/000-defaults.yml remains as /opt/ustreamer-launcher/config-library/hdmi-to-usb.yml.

Expected outcome

TinyPilot becomes configured for the TC358743 chip, which can be seen as the symlink target of /opt/ustreamer-launcher/configs.d/000-defaults.yml changes to /opt/ustreamer-launcher/config-library/tc358743.yml.

Example 2: Mangled file not restored

Steps to reproduce

  1. Start with a working installation of TinyPilot.
  2. Edit /opt/tinypilot/app/version.py and add a comment to the first line.
  3. Run the standard installation instructions.

Actual outcome

The file isn't modified.

Expected outcome

The file is replaced with the version shipped in the latest release.

Suggested fix

There are several approaches we could take to fix this issue. I've done some brief experimentation, and my suggested strategy would be to go with a basic change of modifying the installer script to use apt-get reinstall instead of apt-get install:

https://github.com/tiny-pilot/tinypilot/blob/951b9bfdd51dc856f8c2d8318c6d1859744d00dc/bundler/bundle/install#L57

mtlynch commented 10 months ago

Can you update the description with a bit more context? Can we include repro steps + expected result vs actual result? Do we know the PR where the regression began?

cghague commented 10 months ago

@mtlynch - I've added a bit more context to the description. Regarding reproduction, it's tricky as the issue has no direct implications and only affects other processes. I've instead added a few examples of problems that could result from it and how to reproduce those.

This regression will have been introduced when we switched to using Debian packaging, which I believe was during this change.

mtlynch commented 10 months ago

I've done some brief experimentation, and my suggested strategy would be to go with a basic change of modifying the installer script to use apt-get reinstall instead of apt-get install:

That sounds like a pretty broad fix for what seems to be a niche issue. Doesn't that also drive up our disk writes for each install?

I think we can probably find a more targeted fix for the problems it's creating.

Example 1: Switching capture devices fails

This is a problem, but can we fix it at the instructions layer rather than adjusting our install logic for everyone?

Example 2: Mangled file not restored

This isn't really a bug. If the user edits TinyPilot files, the results are undefined. I think users would generally prefer this, as they'd probably prefer not to keep making local edits.

If users want to undo local changes, we can provide instructions for uninstalling / reinstalling.

cghague commented 10 months ago

That sounds like a pretty broad fix for what seems to be a niche issue. 

The question comes down to what a user would expect to happen if they run the installation scripts on a system that already has TinyPilot installed. The previous behavior was to perform a reinstallation, which seems sensible to me.

Doesn't that also drive up our disk writes for each install?

A fresh installation would always install all three packages, so using the reinstall option wouldn't increase writes in that scenario. We are already deploying a newer version of the TinyPilot package during upgrades, so there wouldn't be any increase in disk writes in that scenario, as they'd be happening anyway.

I think we can probably find a more targeted fix for the problems it's creating.

We can create targeted fixes for the two example issues described above. However, we'd also need to consider any other scenario where we've recommended reinstallation using this method. There might also be implications when going from Community to Pro and vice-versa.

This is a problem, but can we fix it at the instructions layer rather than adjusting our install logic for everyone?

The manual solution is to remove the tinypilot Debian package using apt, change /boot/config.txt, and then reinstall the TinyPilot package (presumably using the script). It's not particularly difficult, but this is just one manifestation.

This isn't really a bug. If the user edits TinyPilot files, the results are undefined. I think users would generally prefer this, as they'd probably prefer not to keep making local edits. If users want to undo local changes, we can provide instructions for uninstalling / reinstalling.

I intended this example to artificially simulate issues such as file corruption rather than an intentional edit by a user. This example is relevant because we recommend that users re-run the installation scripts as our "Fallback method" of reinstallation, something we usually suggest when TinyPilot shows corruption but the rest of the system appears stable. Unfortunately, due to the behavior change described by this bug, this reinstallation method wouldn't actually reinstall anything and thus won't fix any issues. We'd need to either fix the regression, update the TinyPilot Pro download page to include a reinstallation section, or stop suggesting this reinstallation method altogether.

mtlynch commented 10 months ago

That sounds like a pretty broad fix for what seems to be a niche issue.

The question comes down to what a user would expect to happen if they run the installation scripts on a system that already has TinyPilot installed. The previous behavior was to perform a reinstallation, which seems sensible to me.

Right, it's sensible to reinstall TinyPilot if the user tries to run installation scripts on a system that already has TinyPilot.

The part I'm saying is a broad fix would be applying reinstall to every install, not just for users who try to install on a system where TinyPilot is already installed.

If we have a way of applying reinstall for just the users who are in the scenarios we care about, I'd be in favor of that, but it sounds like we'd have to modify the install path for 100% of users, which is much higher risk.

Doesn't that also drive up our disk writes for each install?

A fresh installation would always install all three packages, so using the reinstall option wouldn't increase writes in that scenario. We are already deploying a newer version of the TinyPilot package during upgrades, so there wouldn't be any increase in disk writes in that scenario, as they'd be happening anyway.

Wouldn't this trigger reinstalls of our third-party dependencies on every install even if they haven't changed since the last TinyPilot release?

I think we can probably find a more targeted fix for the problems it's creating.

We can create targeted fixes for the two example issues described above. However, we'd also need to consider any other scenario where we've recommended reinstallation using this method. There might also be implications when going from Community to Pro and vice-versa.

For me, the decision comes down to how many users the change affects and how many unknown-unknowns might affect them.

The percentage of users who install first for HDMI to USB and then want to switch to HDMI to CSI is probably 0.2%, and probably almost nearly zero TinyPilot Pro users.

The percentage of users who have a corrupt filesystem and whose systems get fixed by re-running install is probably about 0.2% as well.

If we change to reinstall, instead of affecting around 0.4% of users, we're affecting 100% of users.

Then the question is: what are the chances of introducing an unexpected side-effect by installing with apt-get in a non-standard way? I'd put it at at least 20%.

This example is relevant because we recommend that users re-run the installation scripts as our "Fallback method" of reinstallation, something we usually suggest when TinyPilot shows corruption but the rest of the system appears stable. Unfortunately, due to the behavior change described by this bug, this reinstallation method wouldn't actually reinstall anything and thus won't fix any issues. We'd need to either fix the regression, update the TinyPilot Pro download page to include a reinstallation section, or stop suggesting this reinstallation method altogether.

I think it makes more sense here to update our support guidance for rare scenarios than to change the core install logic if we can achieve the same thing in either case.

I'd be more in favor of adjusting core logic to reduce the number of users who require explicit support instructions, but in the scenarios we're talking about, it's all cases where the user has to run extra commands that we provide outside of the normal install, so we might as well target our adjustments to those cases where we have to provide targeted fixes.

cghague commented 10 months ago

Right, it's sensible to reinstall TinyPilot if the user tries to run installation scripts on a system that already has TinyPilot.

The part I'm saying is a broad fix would be applying reinstall to every install, not just for users who try to install on a system where TinyPilot is already installed.

If we have a way of applying reinstall for just the users who are in the scenarios we care about, I'd be in favor of that, but it sounds like we'd have to modify the install path for 100% of users, which is much higher risk.

It's my understanding that the only impact of changing the command to apt-get reinstall would be that re-running the script on a system that already has TinyPilot installed would result in reinstallation and that none of the other paths would be affected:

Package state Outcome of install Outcome of reinstall
Not installed Install from new Install from new
Installed, outdated Update to latest Update to latest
Installed, up-to-date No action Reinstall

Wouldn't this trigger reinstalls of our third-party dependencies on every install even if they haven't changed since the last TinyPilot release?

I've tested this and verified that dependencies aren't affected. Only the packages I specified as part of the apt command were reinstalled.

I think it makes more sense here to update our support guidance for rare scenarios than to change the core install logic if we can achieve the same thing in either case.

I'd be more in favor of adjusting core logic to reduce the number of users who require explicit support instructions, but in the scenarios we're talking about, it's all cases where the user has to run extra commands that we provide outside of the normal install, so we might as well target our adjustments to those cases where we have to provide targeted fixes.

I'm happy to implement the changes to our documentation if that's the preferred approach. I've had a quick check around, and I believe we'd need to make changes in the following places:

mtlynch commented 10 months ago

It's my understanding that the only impact of changing the command to apt-get reinstall would be that re-running the script on a system that already has TinyPilot installed would result in reinstallation and that none of the other paths would be affected:

Still, I think we're introducing too much uncertainty by switching to the non-standard path on such a delicate part of our install. Let's stick with install and adjust the other support flows.

The "Installation Options" Wiki page. We'd need to add text explaining how to change the settings when TinyPilot is already installed.

Is it possible for us to just provide commands that will work either way rather than push additional work onto the user?

The README.md file. The current structure makes it sound like you can run a simple installation and make customizations afterward, so we should clarify that the correct approach is to do an advanced install to begin with.

Isn't the README the same? If we assume they're using an HDMI to USB dongle, which is the common case, the simple install works. If they have HDMI to CSI, then the README points them to the advanced install. The advanced install should cover the case when they installed for HDMI to USB and are switching to HDMI to CSI, so the README should stay the same.

The "Install from command-line" instructions for TinyPilot Pro.

This should stay the same. The commands assume that the user has a working version of TinyPilot Community and is upgrading to TinyPilot Pro.

cghague commented 10 months ago

Is it possible for us to just provide commands that will work either way rather than push additional work onto the user?

Detecting if a package is installed is surprisingly complicated, so it's not something we'd want to do in a snippet. We could use the following commands if we're happy with always removing the package before reinstalling it:

(sudo apt-get remove --yes tinypilot || true) && \
  curl [...]

Isn't the README the same? If we assume they're using an HDMI to USB dongle, which is the common case, the simple install works. If they have HDMI to CSI, then the README points them to the advanced install. The advanced install should cover the case when they installed for HDMI to USB and are switching to HDMI to CSI, so the README should stay the same.

We currently have this text after the simple installation instructions:

If you're using an HDMI to CSI capture chip (such as with a TinyPilot Voyager series device), see the additional configuration steps required for video capture.

However, the positioning and wording don't indicate that this is a different type of installation and imply that the user can make the changes after installation. I'd suggest changing it to:

If you're using an HDMI to CSI capture chip (such as with a TinyPilot Voyager series device), see how to use an advanced installation to enable video capture.


This should stay the same. The commands assume that the user has a working version of TinyPilot Community and is upgrading to TinyPilot Pro.

This approach means our playbook's reinstallation "fallback method" instructions aren't valid. Should we update the playbook to directly contain the relevant commands (as opposed to our current process of asking users to go through the license check page), or should we drop it as a troubleshooting step?

mtlynch commented 10 months ago

We could use the following commands if we're happy with always removing the package before reinstalling it:

Yep, that looks good to me. In the wiki specifically, though, not in our standard install, as it's only necessary for an advanced install.


Isn't the README the same? If we assume they're using an HDMI to USB dongle, which is the common case, the simple install works. If they have HDMI to CSI, then the README points them to the advanced install. The advanced install should cover the case when they installed for HDMI to USB and are switching to HDMI to CSI, so the README should stay the same.

We currently have this text after the simple installation instructions:

...

However, the positioning and wording don't indicate that this is a different type of installation and imply that the user can make the changes after installation.

If we change the wiki to add the sudo apt-get remove --yes tinypilot, then it's true that the user can make the changes after installation, right?

If that's true, then the guidance on the standard README.md can stay the same.


This should stay the same. The commands assume that the user has a working version of TinyPilot Community and is upgrading to TinyPilot Pro.

This approach means our playbook's reinstallation "fallback method" instructions aren't valid. Should we update the playbook to directly contain the relevant commands (as opposed to our current process of asking users to go through the license check page), or should we drop it as a troubleshooting step?

I can't find the documentation you mean. Can you link it here? Linking to Notion even in public is fine because Notion does its own auth.

cghague commented 10 months ago

Thanks Michael!

If we change the wiki to add the sudo apt-get remove --yes tinypilot, then it's true that the user can make the changes after installation, right?

That's true, although it introduces extra disk writes. If you're confident this section is clear enough, then I'm happy to leave it as is.

I can't find the documentation you mean. Can you link it here? Linking to Notion even in public is fine because Notion does its own auth.

Good point! The relevant section is here.

mtlynch commented 10 months ago

If we change the wiki to add the sudo apt-get remove --yes tinypilot, then it's true that the user can make the changes after installation, right?

That's true, although it introduces extra disk writes. If you're confident this section is clear enough, then I'm happy to leave it as is.

Yep, this is fine. It's not a big deal to add disk writes in a case where a very small number of users will go through the workflow (like switching from USB to CSI) or if the filesystem is already corrupt and we're trying to repair it.

I can't find the documentation you mean. Can you link it here? Linking to Notion even in public is fine because Notion does its own auth.

Good point! The relevant section is here.

Ah, right. Yeah, we should update that section to unconditionally remove tinypilot before reinstalling.

So the work items for closing this bug are:

cghague commented 9 months ago

Thanks @mtlynch. Can you please confirm you're happy for me to make the following change to the relevant part of the wiki?

  1. Run the get-tinypilot.sh script:
sudo apt remove --yes tinypilot && \
  curl \
  --silent \
  --show-error \
  https://raw.githubusercontent.com/tiny-pilot/tinypilot/master/get-tinypilot.sh | \
    bash - && \
  sudo reboot

I've also put a similar change through on Notion.

mtlynch commented 9 months ago

Wouldn't it need to be

(sudo apt remove --yes tinypilot || true) && \
  curl ...

Otherwise, the command would fail on systems where the tinypilot package isn't already installed.

But outside of that, yep, that's what I had in mind. Same thing for Notion - we just need to make it work in both the case where tinypilot is already installed and when it's not.

cghague commented 9 months ago

Thanks @mtlynch, I've made the required changes!

mtlynch commented 9 months ago

Great, thanks!