search

tiny-pilot / tinypilot

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

Investigation: Find a more robust way for users to change settings only applied during upgrades or installation #1709

Closed cghague closed 3 months ago

cghague commented 7 months ago

Related: #1662

Overview

Certain TinyPilot settings like HDMI capture device are set at installation time. If a user wants to change their HDMI capture device after installing TinyPilot, it's hard for them to do so.

Background

We used to use Ansible to install TinyPilot, so it was always possible for the user to change any setting because they could re-run the Ansible playbook and Ansible would get their system into the right state.

When we switched from Ansible to Debian packages (https://github.com/tiny-pilot/tinypilot/issues/1596), one subtle change was that users could no longer change settings and re-install the same version of TinyPilot. If the Debian package manager sees that the user is trying to install the same version of TinyPilot that's already installed, it will skip installation, which means the user can't reinstall with different settings.

The inability to change install-time settings has become a problem for some users, as they're unable to fix settings like HDMI capture device without uninstalling TinyPilot and then reinstalling.

Affected Settings

There are several settings that a user can only change during installation:

To keep this work concrete, we're going to focus on the capture device type setting, as it's the one that users have reported the most. But we plan to repeat this solution for any other install-time setting that we later want to let users change after install-time.

Relevant Factors

We've rated each of the potential solutions below based on the following four factors:

Effort

We could implement some methods within Support Engineering with only minor code changes and documentation updates. In contrast, some of the other methods would require a significant amount of developer time.

Risk

Various risks are associated with changing our installation process. The risk is likely to be greater the more complex the change, particularly if we're working in a technical area with which we don't have experience as a team.

Coventionality

There are standard ways of configuring Linux packages that we may want to match. For example, a typical Linux user would expect an application to parse a configuration file like ours at runtime. This factor has minimal technical impact but affects user experience.

Compatibility

Our current Debian packages are incompatible with some of the standard tooling. This factor is low-impact since we don't publish to a repository and generally recommend users only use our installation scripts rather than directly interacting with apt or dpkg.

Potential Solutions

1 - Making our Debian packages compatible with dpkg-reconfigure

Effort Risk Conventional Compatible
High High Yes Yes

Brief Description

The dpkg-reconfigure command runs the cached postinst script from the Debian package with the configure argument. We could implement support for this feature. There isn't any requirement for this feature to be interactive, although it often is.

How does the user apply changes?

The user would run dpkg-reconfigure tinypilot.

Effort Required

Key Risks

Prior Discussion / References

2 - Modifying install to uninstall TinyPilot when changes are detected

Effort Risk Conventional Compatible
Medium Low No No

Brief Description

Our install script is fundamentally a launcher for apt, which means we can conditionally take action before handing over control to the Debian package tools. One action we could take would be to modify the install script to automatically uninstall the tinypilot Debian packages when it detects changes to our configuration files. The install script would then run through the standard installation process.

How does the user apply changes?

The user would download and run get-tinypilot[-pro].sh. This user experience is the same as we had before switching to Debian packages.

Changes Required

Key Risks

Prior Discussion / References

N/A

3 - Modifying install to use apt-get reinstall instead of apt-get install

Effort Risk Conventional Compatible
Low Low No No

Brief Description

Our install script currently launches apt-get with the install argument, which means nothing happens if TinyPilot is already on the system.

We could change the install script to launch apt-get with the reinstall argument, which would cause apt to automatically remove and reinstall TinyPilot if and only if the same TinyPilot version is already on the system. This change wouldn't affect fresh installations or upgrades.

How does the user apply changes?

The user would download and run get-tinypilot[-pro].sh. This user experience is the same as we had before switching to Debian packages.

Changes Required

Key Risks

Prior Discussion / References

4 - Separating the configuration scripts and executing them on demand

Effort Risk Conventional Compatible
High Medium No Possibly indirectly

Brief Description

The logic for parsing and processing our configuration files is currently part of the postinst script in the tinypilot Debian package. We could extract this logic into standalone scripts. The Debian package would call the scripts during installation, but they would remain on the system, allowing users to run them after making configuration changes.

How does the user apply changes?

The user would run the new script, e.g., sudo /opt/tinypilot-privileged/update-settings.

Changes Required

Key Risks

Prior Discussion / References

N/A

5 - Separating the configuration scripts and executing them at runtime

Effort Risk Conventional Compatible
High Medium Yes Possibly indirectly

Brief Description

This approach would be identical to the previous one, except that instead of having the installer or user run the standalone scripts, the system, or possibly the TinyPilot app, would automatically execute them at runtime.

How does the user apply changes?

The user would restart TinyPilot using any convenient method.

Changes Required

Key Risks

Prior Discussion / References

N/A

cghague commented 7 months ago

There are five methods that I am aware of that would resolve our configuration problems:

For each of the above methods, I have considered the following factors:

I've summarized my thoughts in the following table (updated 2024-01-24):

Method Effort Risk Conventional Compatible
dpkg-reconfigure High High Yes Yes
install detects changes Medium Low No No
apt-get reinstall Low Low No No
Separate scripts (on-demand) High Medium No Possibly indirectly
Separate scripts (runtime) High Medium Yes Possibly indirectly

@mtlynch - Based on the above, I'd suggest the best option is to separate the configuration logic into a standalone script, which would be called by the tinypilot service each time the application starts.

mtlynch commented 7 months ago

@cghague - Can you share more detail on each of these methods? It's currently hard for a teammate to review the ratings or weigh in because we're currently not really "showing our work" of how we reached these conclusions.

Can we share more about what work is needed for each approach and which specific tasks we find risky?

I'd like to see something like this:

Unconditionally reinstall the Debian package

We can adjust the apt-get install step of installing our Debian packages by changing the command to apt-get reinstall.

This would be an easy change, but the risk is high, as that's an unusual way of installing Debian packages, and we're concerned about unexpected side effects from straying from the standard install path.

We discussed this option previously in https://github.com/tiny-pilot/tinypilot/issues/1662

  • Effort: Low
  • Risk: High

Especially dpkg-reconfigure, I'd like to see more detail about what's involved, but all of them I think could use more fleshing out so it's clear what kind of change we're talking about and why we think it's easy/hard risky/safe.

cghague commented 5 months ago

Making our Debian packages compatible with dpkg-reconfigure

Method Effort Risk Conventional Compatible
dpkg-reconfigure High High Yes Yes

Brief Description

The dpkg-reconfigure command runs the cached postinst script from the Debian package with the configure argument. We could implement support for this feature. There isn't any requirement for this feature to be interactive, although it often is.

Effort Required

Key Risks

Prior Discussion / References

Modifying install to uninstall TinyPilot when changes are detected

Method Effort Risk Conventional Compatible
install detects changes Medium Low No No

Brief Description

Our install script is fundamentally a launcher for apt, which means we can conditionally take action before handing over control to the Debian package tools. One action we could take would be to modify the install script to automatically uninstall the tinypilot Debian packages when it detects changes to our configuration files. The install script would then run through the standard installation process.

Changes Required

Key Risks

Prior Discussion / References

N/A

Modifying install to use apt reinstall instead of apt install

Method Effort Risk Conventional Compatible
apt-get reinstall Low Low No No

Brief Description

Our install script currently launches apt with the install argument, which means nothing happens if TinyPilot is already on the system. We could change the install script to launch apt with the reinstall argument, which would cause apt to automatically remove and reinstall TinyPilot if and only if the same TinyPilot version is already on the system. This change wouldn't affect fresh installations or upgrades.

Changes Required

Key Risks

Prior Discussion / References

Separating the configuration scripts and executing them on demand

Method Effort Risk Conventional Compatible
Separate scripts (on-demand) High Medium No Possibly indirectly

Brief Description

The logic for parsing and processing our configuration files is currently part of the postinst script in the tinypilot Debian package. We could extract this logic into standalone scripts. The Debian package would call the scripts during installation, but they would remain on the system, allowing users to run them after making configuration changes.

Changes Required

Key Risks

Prior Discussion / References

N/A

Separating the configuration scripts and executing them at runtime

Method Effort Risk Conventional Compatible
Separate scripts (runtime) High Medium Yes Possibly indirectly

Brief Description

This approach would be identical to the previous one, except that instead of having the installer or user run the standalone scripts, the system, or possibly the TinyPilot app, would automatically execute them at runtime.

Changes Required

Key Risks

Prior Discussion / References

N/A

cghague commented 5 months ago

@mtlynch - I've provided detailed descriptions of each option in the previous post and updated the summary table so that each option is ranked against the others rather than considered in isolation. I think this illustrates the situation better, but let me know if anything isn't clear!

mtlynch commented 5 months ago

@cghague - Thanks! This is good progress.

I'd like to bring in the rest of the team for discussion. Can we consolidate the details into the root bug? The things I'd like to consolidate are:

I'd also like to flesh out which settings we're considering for this. We should have at least one concrete case based on user requests / support tickets (e.g., user wants to switch from HDMI to USB, user is trying to fix a bad/modified install). We'd probably just want to solve for one common case rather than burn effort on every install config change a user might possibly do.

cghague commented 4 months ago

@mtlynch - I've updated the original message as requested. Please let me know if you want me to expand or clarify anything!

mtlynch commented 4 months ago

@cghague - Thanks, I think we're pretty close.

Defining the UX

One thing I'm realizing we're missing is what the user experience looks like in the proposed solutions. Can we add a section to each like, "How user applies capture device settings." Like, assuming the user runs this command:

if ! grep --silent '^dtoverlay=tc358743$' /boot/config.txt; then
  echo 'dtoverlay=tc358743' | sudo tee --append /boot/config.txt
fi

What do they need to do to make TinyPilot recognize the change?

For example, for the ones where we're changing the install script, the new UX would be:

How user applies new capture device settings

User re-runs get-tinypilot.sh or get-tinypilot-pro.sh.

But it would be different for dpkg-reconfigure and the config script versions.

Clarifying risks for config scripts

I'm confused about this part:

Separating the configuration scripts and executing them on demand

Key Risks

  • Requires large-scale code changes that can result in new bugs.
  • Drastically different user experience to present.

Aren't we talking about extracting like 10 lines of bash code out to a helper script? I wouldn't consider that a large-scale code change.

And the impact on the user experience seems small. They'd be running one command-line script instead of another when they want to change their capture device, but that's a small change and a rare event.

(ditto for the other option where we evaluate config scripts at runtime)

cghague commented 4 months ago

Thanks @mtlynch. I've added a section to each method explaining how the user would apply any changes they've made to the TinyPilot settings. I also noticed you'd made a few edits, and while reviewing them, I realized that the "Overview" didn't make sense, so I've updated it to reflect what I think you intended it to say.

Regarding your request to clarify the risks associated with config scripts, I think this might be down to my interpretation of what "code change" actually means. I agree that it would only be a few lines of bash code moved around, but the context of pulling code out of the Debian installer to a standalone file makes that seem like a significant change to me, as we'd have to consider aspects like user permissions and so on.

The impact on user experience is open to interpretation. My reason for thinking it will be drastically different is that, while the user will indeed only run a single script in either scenario, the visible effect of that script will be quite noticeably different. The config script would likely only show a simple confirmation that it has updated the settings, whereas the get-tinypilot[-pro].sh script would show an entire installation process.

I hope that makes sense, please let me know if you have any questions!

mtlynch commented 4 months ago

Regarding your request to clarify the risks associated with config scripts, I think this might be down to my interpretation of what "code change" actually means. I agree that it would only be a few lines of bash code moved around, but the context of pulling code out of the Debian installer to a standalone file makes that seem like a significant change to me, as we'd have to consider aspects like user permissions and so on.

The impact on user experience is open to interpretation. My reason for thinking it will be drastically different is that, while the user will indeed only run a single script in either scenario, the visible effect of that script will be quite noticeably different. The config script would likely only show a simple confirmation that it has updated the settings, whereas the get-tinypilot[-pro].sh script would show an entire installation process.

@cghague - I agree that these are open to interpretation and subjective. Can we rephrase them to be more objective and specific in factual details?

I think we can disagree about running one bash command over another is a drastically different user experience, but if we explain what the difference in UX is, that's something we'll probably agree on. Similarly, if you think the risky part of refactoring into helper scripts is getting file permissions right, we should say specifically that the risk is getting file permissions right when we refactor.

cghague commented 4 months ago

Thanks for the clarification, @mtlynch; I've updated the text.

mtlynch commented 3 months ago

Thanks for updating! I'll pull in the dev team to review once we complete https://github.com/tiny-pilot/tinypilot-pro/issues/1266

mtlynch commented 3 months ago

@jotaen4tinypilot, @jdeanwallace - Can you review the root bug description and weigh in on which solution seems most sensible to you?

jotaen4tinypilot commented 3 months ago

I’d feel that choosing a sensible solution would depend a bit on the following factors:

My thinking is, the more we’d lean towards these settings to be changed infrequently and/or only by relatively few users, the more I’d think a pragmatic “sledge hammer” solution like a full re-install would be justified. If it’s the other way around, I’d think it would warrant for a more sophisticated solution, even if that’s more costly to build.

mtlynch commented 3 months ago

How likely are we to add support for more of such settings customisations in the future?

It's not likely that we'd ever support an official flow where we guide the user through changing capture hardware.

The goal here is just a slightly tidier way of changing settings that depend on install-time state, the most conspicuous one being capture hardware. The current workaround of apt remove tinypilot isn't too bad, but it has the downside of temporarily putting the user into a bad state (TinyPilot has been removed) just to change one setting.

How common is it for users to change these settings? I.e., how big is the portion of users that are interested in that? Do we consider this to be something “normal”, or is it rather somewhat edge-casey and exceptional?

I'll let Charles add more context here since I've stopped following support tickets as closely, but my sense is that it's pretty rare, like a handful of customers per year talk to us about needing to do this.

For users who are interested to change these settings, how often would they toggle them back and forth? I.e., could we legitimately expect someone to constantly change their keyboard/mouse HID path for some reason, or is it more that users would apply a customisation once and then forget about it?

I don't think people would swap back and forth very often, so it would likely be to change it once and then stick with that change.

jotaen4tinypilot commented 3 months ago

My thoughts are these then:

If changing settings is rarely requested, and if we’d consider it bearable for users to go through a full reinstall in that case, I’d lean towards the “Modifying install to use apt reinstall instead of apt install” solution. It’s not super convenient, and it might feel a bit strange to make a full TinyPilot reinstall only to provision custom certificates (or the like), but it’s pretty safe from our side, and we can also transparently communicate what it does.

It seems that the “reinstall” option is pretty quick to build and ship, so we could e.g. start with that and see whether people accept it and what feedback we get. If we see that people struggle or complain, we could still invest into a more sophisticated solution. In this case, it would seem worthwhile to me to explore the dpkg-reconfigure option, as that seems most native, to get a feeling of how complex or risky it would be in reality.

mtlynch commented 3 months ago

If changing settings is rarely requested, and if we’d consider it bearable for users to go through a full reinstall in that case, I’d lean towards the “Modifying install to use apt reinstall instead of apt install” solution. It’s not super convenient, and it might feel a bit strange to make a full TinyPilot reinstall only to provision custom certificates (or the like), but it’s pretty safe from our side, and we can also transparently communicate what it does.

Just to make sure we're on the same page, for that plan, we wouldn't really be telling users to run apt reinstall. We'd be modifying this line in the install script so that it always reinstalls the tinypilot, ustreamer, and janus packages regardless of what version is installed. Is that your understanding?

cghague commented 3 months ago

If changing settings is rarely requested, and if we’d consider it bearable for users to go through a full reinstall in that case, I’d lean towards the “Modifying install to use apt reinstall instead of apt install” solution. It’s not super convenient, and it might feel a bit strange to make a full TinyPilot reinstall only to provision custom certificates (or the like), but it’s pretty safe from our side, and we can also transparently communicate what it does.

Just to make sure we're on the same page, for that plan, we wouldn't really be telling users to run apt reinstall. We'd be modifying this line in the install script so that it always reinstalls the tinypilot, ustreamer, and janus packages regardless of what version is installed. Is that your understanding?

That's not quite correct; apt will only reinstall the package if exactly the same version is already installed. The flow doesn't change in any other situation. The matrix below, which I've copied from my comment on the related issue, summarizes this:

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
mtlynch commented 3 months ago

@cghague - Is there a difference between "Install from new," "Update to latest," or "Reinstall"? I thought they were all effectively the same thing in that they result in the packages in the bundle being installed.

Could the table be restated like this?

Package state Outcome of install Outcome of reinstall
Not installed Install all packages from bundle Install all packages from bundle
Installed, outdated Install all packages from bundle Install all packages from bundle
Installed, up-to-date No action Install all packages from bundle
jotaen4tinypilot commented 3 months ago

Just to make sure we're on the same page, for that plan, we wouldn't really be telling users to run apt reinstall. We'd be modifying this line in the install script so that it always reinstalls the tinypilot, ustreamer, and janus packages regardless of what version is installed. Is that your understanding?

My initial reading was this: for a re-install, we’d be modifying the install script to basically say reinstall where it now says install. For the reconfigure, users would either have to run dpkg-reconfigure as separate command, or we incorporate it into the install script and wrap it into a check – e.g., either “if package installed, run dpkg-reconfigure, otherwise (re-)install (like now)”, or we introduce a --reconfigure flag.

To clarify my thoughts, I was more considering the maintainability aspect on our end, than the experience for end-users. With the dpgk-reconfigure option, it seems that we’d create a completely new entrypoint into our Debian packages. So in addition to the two current flows of installing and uninstalling, we’d then have a third flow that we’d have to test and maintain. (Is that correct?)

So my thinking was to avoid that complexity/liability as long as possible, without introducing any custom and unconventional workarounds. That’s why I thought reinstall was a pragmatic and clean (yet not super elegant) approach. However, if we’d feel that reinstall is too inconvenient for this purpose, then I’d prefer reconfigure despite its complexity, because to me it would seem like the most native and “correct” approach in the Debian packaging realm.

mtlynch commented 3 months ago

My initial reading was this: for a re-install, we’d be modifying the install script to basically say reinstall where it now says install.

@jotaen4tinypilot - The place where I think there might be a divergence is that it sounds like you're talking about us giving special instructions to users for a re-install where they run apt-get reinstall while other users continue using the standard install script with apt-get install. But proposal (3) is that we modify the install script for everyone, not just users who need to change capture hardware.

I just wanted to verify that we're talking about the same thing.

jdeanwallace commented 3 months ago

I agree that ideally we'd probably want to go the dpkg-reconfigure route (i.e., solution 1) because that seems like the most Debian-way. However, for now I'd probably go for conditionally executing install or reinstall based on a "certain condition" (i.e., solution 2 or 3). The proposed solutions suggest that the "certain condition" is determined automatically, but perhaps we could get away with a prettier manually flag.

For example, instead of this:

(sudo apt remove --yes tinypilot || true) && \
  curl \
    --silent \
    --show-error \
    https://raw.githubusercontent.com/tiny-pilot/tinypilot/master/get-tinypilot.sh | \
    bash - && \
  sudo reboot

We could add a flag that allows re-installs if the same version has been detected:

curl \
  --silent \
  --show-error \
  https://raw.githubusercontent.com/tiny-pilot/tinypilot/master/get-tinypilot.sh | \
  bash -s -- \
    --allow-reinstall && \
  sudo reboot

In get-tinypilot-pro.sh we already have an allow-reinstall flag: https://github.com/tiny-pilot/gatekeeper/blob/031632d257a3019baa221ce7d93dbdf11250dcb0/static/get-tinypilot-pro.sh#L23-L25

which we only use for importing old-settings: https://github.com/tiny-pilot/tinypilot-pro/blob/3df954aae69cccdaff320cc20f74574adaec4592/debian-pkg/opt/tinypilot-privileged/scripts/import-settings#L176-L184

So my question is, must the reinstall condition be determined automatically, or can it be indicated by the user?

mtlynch commented 3 months ago

However, for now I'd probably go for conditionally executing install or reinstall based on a "certain condition" (i.e., solution 2 or 3)

To clarify, (3) proposes for us to unconditionally do apt-get reinstall for all users, not just for certain conditions.

In get-tinypilot-pro.sh we already have an allow-reinstall flag

I think that the --allow-reinstall flag has the same problem we have here.

I haven't tested it, but I think we'd just see the same behavior. It's a gatekeeper-level check to tell Gatekeeper not to get in the way of the reinstall. It doesn't do anything to prevent apt-get from skipping the Debian package install process when the installed version and the target version are the same.

So we can add a flag to get-tinypilot.sh, but then we'd have to add plumbing to propagate the flag down to the install script in the bundle where we actually call apt-get install.

jdeanwallace commented 3 months ago

To clarify, (3) proposes for us to unconditionally do apt-get reinstall for all users, not just for certain conditions.

Thanks for clarifying.

In that case, I would go for option (3) with the possibility of adding a conditional check in get-tinypilot[-pro].sh to guard against reinstalling the same version each time (we already have this in pro).

So essentially, our ./install script will always attempt a apt-get install --reinstall, but the get-tinypilot[-pro].sh script would automatically block against reinstalling the same version unless indicated by a CLI flag like --allow-reinstall.

This should maintain the current install behaviour while allowing certain users to conditionally reconfigure their device in a less hacky way.

mtlynch commented 3 months ago

Okay, I think we can close this here. It seems like people are comfortable with the apt-get reinstall solution. I'd also like to see a scratch implementation of (4) before we move forward with any solution.

For now, let's shelve this since we don't see a lot of customers hitting this issue, but if it begins coming up more, let's revisit this and consider whether it's worth investing more in a cleaner solution than our current workaround of (sudo apt remove --yes tinypilot || true).