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

APP-7128 support modules on windows #4605

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Dec 6, 2024

What changed

  • high-level: support modules on windows by using TCP sockets instead of unix sockets
  • added utils.ViamTCPSockets to control new 'tcp sockets mode'. this is always true on windows, can be forced with VIAM_TCP_SOCKETS env var
  • when ViamTCPSockets is true:
    • Module.Start uses incrementing localhost ports instead of unix sockets on windows
    • in webService.StartModule, addr = "127.0.0.1:14998" instead of parent.sock
  • disable ftdc on windows because it logs tracebacks; guessing this can be fixed as follow-up

Why

Windows support on non-WSL machines.

Checklist

  • factor out windows detection to a util, allow it to be set behind VIAM_TCP_SOCKETS var as well
  • run existing module unit tests in TCP mode

Follow-up

  • fix ftdc on windows
  • investigate unix sockets support on windows -- details in comments in ViamTCPSockets() function
  • think about port collisions w/ other services -- simplest fix might be to allow users to configure the port range so if this does happen, it's simple to patch. Think about port exhaustion -- should reclaim ports when done. ideally, use system-assigned ports (but hard bc of how we generate them)

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 6, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 6, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 10, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 10, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 10, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 11, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 11, 2024
@abe-winter abe-winter marked this pull request as ready for review December 11, 2024 19:08
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

has this been tested on a windows machine? also would definitely would want the followup to deal with port exhaustion and reclaming ports

module/modmanager/manager.go Show resolved Hide resolved
@abe-winter
Copy link
Member Author

abe-winter commented Dec 13, 2024 via email

Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

Comments only for the moment. I'm nervous about opening up TCP.

@@ -40,6 +41,9 @@ import (
rutils "go.viam.com/rdk/utils"
)

// tcpPortRange is the beginning of the port range. Only used when ViamTCPSockets() = true.
const tcpPortRange = 13500
Copy link
Member

Choose a reason for hiding this comment

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

What happens when a port is occupied?

Copy link
Member Author

Choose a reason for hiding this comment

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

when a port is occupied, this breaks; long term, we need a better way to do this. can potentially request an unused port (:0) and have module report to RDK via the parent socket, but that's more plumbing

Copy link
Member

@dgottlieb dgottlieb Dec 18, 2024

Choose a reason for hiding this comment

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

I'm perfectly cool with this. I'd recommend copying that expectation into the documentation:

tcpPortRange is the beginning of the port range. Only used when ViamTCPSockets() = true. If a port is already taken, the server breaks and behaves in an undefined way. Given this is for a pre-production windows release, we're ok with this outcome. The code/algorithm will become more robust in the future.

const SubtypeName = "web"
const (
SubtypeName = "web"
TCPParentPort = 14998
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this port is occupied?

Copy link
Member Author

Choose a reason for hiding this comment

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

TCP sockets support breaks

follow-ups ticket APP-7240 will change this to zero to auto-assign

module/modmanager/manager.go Show resolved Hide resolved
@abe-winter
Copy link
Member Author

abe-winter commented Dec 13, 2024

@cheukt created APP-7240 for port exhaustion

@abe-winter
Copy link
Member Author

@Otterverse

have you considred named pipes

yes -- planning to explore named pipes and unix sockets support as follow-up. best outcome would be for us to solve go-grpc support on windows AF_UNIX so that we can be completely stock

@viamrobotics viamrobotics deleted a comment from Otterverse Dec 13, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants