cmd/sshpiperd: allow starting without plugins running#653
cmd/sshpiperd: allow starting without plugins running#653josharian wants to merge 1 commit intotg123:masterfrom
Conversation
|
The three PRs I just sent may have conflicts with each other. I'm happy to rebase, sequence them, etc, just say the word! I sent them all independently to start with in case you had opinions about wanting some but not others. :) |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the sshpiperd daemon to allow starting without plugins running, implementing a lazy plugin initialization strategy. Instead of failing startup when plugins can't be initialized, the daemon now defers plugin initialization until the first connection attempt.
- Deferred plugin initialization from startup to first connection
- Added lazy plugin loading with atomic state tracking and single-flight pattern
- Enhanced logging with structured fields for better observability
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go.mod | Updated Go version and various dependency versions, added tailscale.com dependency |
| cmd/sshpiperd/main.go | Modified to store plugin configs instead of initializing plugins immediately |
| cmd/sshpiperd/daemon.go | Added lazy plugin initialization with atomic state and wrapper listener |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tg123
left a comment
tg123
left a comment
There was a problem hiding this comment.
could you please add some test in e2e to convert lazy init?
cmd/sshpiperd/daemon.go
Outdated
| daemon *daemon | ||
| } | ||
|
|
||
| func (l *lazyPluginListener) Accept() (net.Conn, error) { |
There was a problem hiding this comment.
may i know the reason using a listener to active
why not put this to
for {
accept
}
cmd/sshpiperd/daemon.go
Outdated
| return nil // previously initialized successfully | ||
| } | ||
|
|
||
| _, err, _ := d.pluginIniSingleFlight.Do(unused{}, func() (unused, error) { |
There was a problem hiding this comment.
may i know if a simple lock works?
seems introducing tailscale is too heavy
cmd/sshpiperd/daemon.go
Outdated
| } | ||
|
|
||
| func (d *daemon) initializePlugins() error { | ||
| var plugins []*plugin.GrpcPlugin |
There was a problem hiding this comment.
a bit worry about partial plugin failure and might create too many zombies
do not retry on inited plugin?
|
Great feedback. I'll turn my attention back to this soon, I hope, and incorporate all of it. |
|
At long last, another rev, which should address all your concerns. Thank you for your patience. We've been using the actual code changes for a while. I haven't banged on it too hard, but haven't seen any issues yet either. The e2e test is new; confidence in it is lower. |
This is an initial implementation. It doesn't have many miles on it. I'm sharing it for early feedback and thoughts.
Fixes #640