Static IP address scripts Bookworm compatibility #1843

db39 · 2024-08-22T14:25:51Z

Resolves #1680

This change updates our static IP scripts to conditionally use nmcli for compatibility with Raspberry Pi Bookworm, retaining the previous implementation for Bullseye.

db39 · 2024-08-22T14:43:36Z

@jdeanwallace - I've taken a stab at this to introduce Bookworm compatibility with our static IP scripts. I tested these scripts on Bookworm 64-bit and they seem to work as expected and don't require a reboot.

The one caveat with nmcli is that it requires the connection name, rather than the interface (i.e., 'Wired connection 1' instead of 'eth0'), so it's a little awkward. I think this is ok considering static IP changes only affect the Ethernet interface.

db39 · 2024-08-22T14:43:42Z

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

jdeanwallace

Automated comment from CodeApprove ➜

Looks good. I'll test it on device in the next review iteration.

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 134
  if [[ "${OS_CODENAME}" == "bullseye" ]]; then

Can we use single quotes for strings that use variable interpolation?

Same throughout.

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 137
    readonly INTERFACE="Wired connection 1"

It might be misleading if an interface was specified but it's being treated as a connection name. Can we search for the correct connection name instead? Something like:

CONN_NAME="$(nmcli --fields NAME,DEVICE --terse connection show |
  grep --max-count 1 "${INTERFACE}" |
  cut --fields 1 --delimiter ':')"

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 137
    readonly INTERFACE="Wired connection 1"

Actually, if we're going to be moving to nmcli, then the --interface command-line option will probably need to be phased out.

Let's just ignore the interface when using nmcli and hard code the connection name to Wired connection 1. We're already hard coding it in the unset/apply scripts.

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 145
OS_CODENAME="$(grep '^VERSION_CODENAME' /etc/os-release | cut --delimiter '=' --fields 2)"

We can also use lsb_release --codename --short to retrieve the distribution name.

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 145
OS_CODENAME="$(grep '^VERSION_CODENAME' /etc/os-release | cut --delimiter '=' --fields 2)"

Also we can consider matching on the release number instead. Something like this:
https://github.com/tiny-pilot/tinypilot-pro/blob/ef880501bb51819de660ccff1b5d65b7e8d11fef/bundler/bundle/install#L34-L35

👀 @db39 it's your turn please take a look

db39

Automated comment from CodeApprove ➜

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Matching by version number probably makes more sense if it's how we've checked this kind of thing in the past.

👀 @jdeanwallace it's your turn please take a look

jdeanwallace

Automated comment from CodeApprove ➜

@db39 - I'm concerned about how these changes would affect the Pro repo where we have a slightly more complex procedure in place for setting a static IP via the Web UI (i.e., setting a temporary static IP, waiting for the new settings to be confirmed, then permanently persisting then new static IP) to avoid the user being locked out of their device.

Would you mind exploring the changes that would need to be made to the Pro repo in a new PR before we continue with merging these changes into the Community repo?

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 141
OS_VERSION="$(lsb_release --release --short)"

Can we mark this variable as readonly?

Same throughout.

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:

> Line 143
if [[ "${OS_VERSION}" == 11 ]]; then

Can we use double parentheses for this numeric comparison?

https://github.com/tiny-pilot/style-guides/#arithmetic--numerical-comparisons

Same throughout.

👀 @db39 it's your turn please take a look

db39 · 2024-09-03T14:13:14Z

@jdeanwallace - Sure, I'll take a look at the Pro versions of the scripts and see what we'd need to do on that front.

db39

Automated comment from CodeApprove ➜

In: debian-pkg/opt/tinypilot-privileged/scripts/set-static-ip:
Resolved

db39 · 2024-09-10T12:30:34Z

@jdeanwallace - looking at this some more, we might be able to add to nmcli's connections to configure a static IP instead of using modify:

sudo nmcli connection add type ethernet con-name tinypilot-static-ip ifname 'eth0' ip4 192.168.178.100 gw4 192.168.178.1

Then run `nmcli connection show:

NAME                 UUID                                  TYPE      DEVICE 
Wired connection 1   cf15d7fb-18ce-3d4e-a433-d6695b881897  ethernet  eth0   
lo                   230c75e5-4c72-417e-bb0d-79df205005cc  loopback  lo     
preconfigured        13d94203-335a-4ba4-bc57-5109f5f86a80  wifi      --     
tinypilot-static-ip  0704fd50-2c28-4c34-bd09-34d3b5812d17  ethernet  --

Then you can switch between connections like this:

nmcli connection up tinypilot-static-ip

nmcli connection up "Wired connection 1"

This way, you don't have to save the previous config, you just switch between the connections. Although I'm not sure if this impacts other networking we do?

If we have to use nmcli modify and save the config, we could capture the config with:

nmcli connection show "Wired connection 1"

And output the fields we're interested in to a text file. Then we can restore the config from the file if required.

So the flow in Pro can be the same as it is currently as far as I understand (we'd just need to add nmcli to the additional steps).

jdeanwallace · 2024-09-10T13:33:43Z

@db39 - That's an interesting idea and it might work, but I'm not too sure how it will all fit together. Seeing as the tinypilot-pro repo has the more complex use-case (i.e., set temporary settings then revert or save settings permanently), can we create a draft PR in the Pro repo with an example of what you're suggesting or some version of it? That way it will be easier to test and give feedback on.

db39 · 2024-09-11T14:14:48Z

@jdeanwallace - after more digging, creating a new connection could work, but if the user wants to change from one static IP to another, we would have to create an additional static IP connection. At that point, we'd have to store the details of the previous connection anyway (though maybe only the name), and we'd probably have to clean up the old connection. It also appears as though the default Wired connection 1 connection may get deleted if it's not being used.

It seems like following a similar flow to what we have already with dhcpcd (storing the config in a file) and just modifying Wired connection 1 instead might be the simplest option. I'll create a draft PR for that.

db39 · 2024-10-18T14:03:51Z

It's been a little while since I last looked / worked on this.

Coming back to this, I was thinking about why we need to make the static IP script compatible with both Bullseye and Bookworm (particularly on Pro). It makes some sense with community because users may attempt to install TinyPilot Community on Bookworm and it would be nice to support that. But on Pro we're currently Bullseye-only, and when we do support Bookworm, won't we require users to re-flash to upgrade (like we did from Buster -> Bullseye)? Am I misunderstanding or missing something here?

jdeanwallace · 2024-10-21T18:15:45Z

why we need to make the static IP script compatible with both Bullseye and Bookworm (particularly on Pro)

@db39 - I think the confusion is that (as far as I know) we're not dropping support for Bullseye and we're only adding support for Bookworm. So users will still be able to install TinyPilot (Community or Pro) on both Bullseye and Bookworm via the command-line (at least).

Also, conditionally supporting both Bullseye and Bookworm allows us slowly test Bookworm compatibility on the whole system before rolling it out to our users. For example:

if BOOKWORM:
  do it the NetworkManager way
else:
  do it the dhcpcd way

db39 · 2024-10-22T11:31:46Z

(as far as I know) we're not dropping support for Bullseye and we're only adding support for Bookworm.

Ahh, thanks! It would make sense if that's the plan. Do we have confirmation that the plan is to support both versions (@scott-tp)?

initial draft static IP bookworm compatibility

619ce4b

db39 linked an issue Aug 22, 2024 that may be closed by this pull request

Static IP address scripts are incompatible with Raspberry Pi OS Bookworm #1680

Open

codeapprove bot added the codeapprove label Aug 22, 2024

db39 marked this pull request as ready for review August 22, 2024 14:27

db39 added 2 commits August 22, 2024 15:36

comments

ebd0b0e

style

46d8bf6

db39 requested a review from jdeanwallace August 22, 2024 14:43

jdeanwallace requested changes Aug 23, 2024

View reviewed changes

Using version number insead of name. Hardcode connection name

bc2fad7

db39 commented Aug 23, 2024

View reviewed changes

jdeanwallace requested changes Aug 27, 2024

View reviewed changes

fix numeric comparisons, readonly variable

4aa0e7c

db39 commented Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static IP address scripts Bookworm compatibility #1843

Static IP address scripts Bookworm compatibility #1843

db39 commented Aug 22, 2024 •

edited

Loading

db39 commented Aug 22, 2024

db39 commented Aug 22, 2024

jdeanwallace left a comment

db39 left a comment

jdeanwallace left a comment

db39 commented Sep 3, 2024

db39 left a comment

db39 commented Sep 10, 2024

jdeanwallace commented Sep 10, 2024

db39 commented Sep 11, 2024

db39 commented Oct 18, 2024

jdeanwallace commented Oct 21, 2024

db39 commented Oct 22, 2024

Static IP address scripts Bookworm compatibility #1843

Are you sure you want to change the base?

Static IP address scripts Bookworm compatibility #1843

Conversation

db39 commented Aug 22, 2024 • edited Loading

db39 commented Aug 22, 2024

db39 commented Aug 22, 2024

Automated comment from CodeApprove ➜

jdeanwallace left a comment

Choose a reason for hiding this comment

Automated comment from CodeApprove ➜

db39 left a comment

Choose a reason for hiding this comment

Automated comment from CodeApprove ➜

jdeanwallace left a comment

Choose a reason for hiding this comment

Automated comment from CodeApprove ➜

db39 commented Sep 3, 2024

db39 left a comment

Choose a reason for hiding this comment

Automated comment from CodeApprove ➜

db39 commented Sep 10, 2024

jdeanwallace commented Sep 10, 2024

db39 commented Sep 11, 2024

db39 commented Oct 18, 2024

jdeanwallace commented Oct 21, 2024

db39 commented Oct 22, 2024

db39 commented Aug 22, 2024 •

edited

Loading