activation: allow more control over socket-activated file descriptors #441
Conversation
percivalalb
changed the title
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Not sure if you still want to work on this. If yes, here are my initial comments.
activation/filesmethod_unix.go
Outdated
| case ReserveFiles: | ||
| devNull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0755) | ||
| if err != nil { | ||
| return fmt.Errorf("accessing /dev/null: %w", err) |
There was a problem hiding this comment.
nit: no need to wrap an error as os.Open returns PathError which already has the file name.
There was a problem hiding this comment.
This particular error is wrapped by the errorDevNullSetup type now - as are a few other errors produced in this ReserveFiles case. Sorry this Errorf was from an old change in an earlier commit, and I hadn't squash as I iterated on my changes.
I'm thinking of changing the error message from
"setting up " + strconv.Itoa(e.fd) + " fd to /dev/null: " + e.err.Error()
to
"reserving FD #" + strconv.Itoa(e.fd) + " : " + e.err.Error()`
activation/listeners.go
Outdated
| // corresponding with "udp, tcp, tcp", then the slice would contain {nil, net.Listener, net.Listener} | ||
| func Listeners() ([]net.Listener, error) { | ||
| files := Files(true) | ||
| func Listeners(opts ...option) ([]net.Listener, error) { |
There was a problem hiding this comment.
This breaks existing API, so you probably need to add ListenersWithOptions or something like that.
There was a problem hiding this comment.
While techincally the function signature was different, I was hoping I could get away with it as consumers of the library would not have to update their code. I've added WithOptions functions to seperate the old and new for now.
activation/listeners.go
Outdated
| // It uses default Listeners func and forces TCP sockets handlers to use TLS based on tlsConfig. | ||
| func TLSListeners(tlsConfig *tls.Config) ([]net.Listener, error) { | ||
| listeners, err := Listeners() | ||
| func TLSListeners(tlsConfig *tls.Config, opts ...option) ([]net.Listener, error) { |
activation/filesmethod_unix.go
Outdated
| "syscall" | ||
| ) | ||
|
|
||
| type ErrorDevNullSetup struct { |
There was a problem hiding this comment.
Do this need to be public?
There was a problem hiding this comment.
No, you're right. Especially since the fields are private too. Also at the moment this error isn't event returned up the chain (though I'd like to change that).
activation/options.go
Outdated
| } | ||
| } | ||
|
|
||
| // Options takes some [option]s and produces a stuct containing the flags and settings. |
There was a problem hiding this comment.
It is weird to refer to a private type in doc.
There was a problem hiding this comment.
Removed. I've djusted how the options struct is populated to running a apply function on the type instead.
|
Hi, thank you for the review. I'll resolve the comments over the next week |
The latter was not around in 1.12 (min targetted version)
Avoids it being a breaking change
Uses explict octal and 666 file permission which I hear is more conventional for device files.
|
I've rebase onto main and added some more commits with changes based on your comments. I will squash the comments down once the changes have been reviewed. Before deployment I will push some unit tests. |
| if pc, err := net.FileListener(f); err == nil { | ||
| listeners[i] = pc | ||
|
|
||
| o.method.Apply(f) |
There was a problem hiding this comment.
Any thoughts on how to handle the error from this? Previously this was a f.Close() and the error was ignored (would linux ever return an error anyway?). Now a few more things could happen - e.g. setting up the /dev/null stuff.
2 participants
The XY of the problem is the fds indices used by systemd socket activation are left empty once
activation.Listeneris called. Any new fds typically take the lowest available fds, which means the expected socket-activated indices may be populated by other fds at any other point in the executing of the program.I'm creating a library that enables zero-downtime restarts by
execve'ing the same program which requires that the fds be populated in the expected indices. This is not possible if another part of the program is using those fds.ConsumeFilesthe default and matches the previous logic - closes the systemd fds passed in.ReserveFiles"reserves" the systemd fds by atomically replacing them with fds pointing at/dev/nullConserveFilesdoes nothing and keeps the systemd fds openunsetEnvof the system