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

Make it possible to use systemd activation for UDP sockets as well. #27

Closed
wants to merge 3 commits into from
Closed

Conversation

theraphim
Copy link

Refactor the wrapping of sockets into a separate function. For every
fd, try it as a packet socket first. If it errors out, try net.Listener.

Refactor the wrapping of sockets into a separate function. For every
fd, try it as a packet socket first. If it errors out, try net.Listener.
@theraphim
Copy link
Author

Hi, I was wondering is there anything else needed for this to be merged? thanks

@philips
Copy link
Contributor

philips commented Feb 26, 2014

@theraphim Thanks for the contribution. What do you think about adding an activation.PacketConns method instead? Then it is a little more consistent with the rest of the API.

@theraphim
Copy link
Author

@philips well, you would usually want to retrieve all of your sockets at once. The order may also matter to you. I can create PacketConns method with semantics similar to Listeners, which may be good for simple daemons like tftp server, but for more complex cases where your service does both udp and tcp I haven't found a better approach than retrieving them and wrapping them all in one call.

@porjo
Copy link

porjo commented Jul 17, 2014

+1

This was referenced Jul 17, 2014
@philips
Copy link
Contributor

philips commented Aug 4, 2014

@theraphim Your more complex app wouldn't want the TCP and UDP sockets separated though?

@philips
Copy link
Contributor

philips commented Aug 4, 2014

@theraphim If your main concern is that you want to get the ordering then how about we return a slice of Listeners and a slice of ints that tells us the order? Or a slice of type ActivationListener that has the integer for ordering such as:

type ActivationListener struct {
    net.Listener
    Order int
}

@theraphim
Copy link
Author

@philips not sure what do you mean. If I'm doing it through WrapSystemdSockets, I'm getting back both udp and tcp sockets, in one call, preserving the ordering.
It is kind of possible to make it work with 2 functions (if we ignore unsetEnv or make a local copy of Files), I just don't know why we should do it - in both functions we will have to test all sockets anyway.

@philips
Copy link
Contributor

philips commented Aug 4, 2014

@theraphim The wrapped socket struct is sort of annoying because it is two completely different things and I have to check for nil on both. Iterating is cheap since this is only done on startup.

@philips
Copy link
Contributor

philips commented Aug 5, 2014

@theraphim How about we have a type assertion switch that people should use? So the function prototype becomes:

func Sockets(unsetEnv bool) ([]Interface{}, error) {

@miekg
Copy link

miekg commented Aug 5, 2014

I don't really like the 'TcpOrUdp' struct. The ordering proposed with the ActivationListener seems sane and simple. It would need to ActivationPacketConn as well?

Using this is just, sort -> iterate -> start daemon

@philips
Copy link
Contributor

philips commented Aug 5, 2014

@miekg So the total interface would become:

func Listeners(unsetEnv bool) ([]net.ActivationListener, error)
func PacketConns(unsetEnv bool) ([]net.ActivationPacketConns, err)

type ActivationListener struct {
    net.Listener
    Order int
}

type ActivationPacketConns struct {
    net.PacketConns
    Order int
}

@miekg
Copy link

miekg commented Aug 5, 2014

Yes, that would tie in nicely with #54.

And drop the 's' on net.PacketConnS :)
On 5 Aug 2014 22:16, "Brandon Philips" notifications@github.com wrote:

@miekg https://github.com/miekg So the total interface would become:

func Listeners(unsetEnv bool) ([]net.ActivationListener, error)
func PacketConns(unsetEnv bool) ([]net.ActivationPacketConns, err)

type ActivationListener struct {
net.Listener
Order int
}

type ActivationPacketConns struct {
net.PacketConns
Order int
}


Reply to this email directly or view it on GitHub
#27 (comment).

@philips
Copy link
Contributor

philips commented Aug 5, 2014

@miekg OK, this seems good. If you or @porjo want to tackle this let me know!

@miekg
Copy link

miekg commented Aug 5, 2014

I'll let @porjo chip in. If we hear nothing the next few days I will hammer
his PR into submission
On 5 Aug 2014 22:21, "Brandon Philips" notifications@github.com wrote:

@miekg https://github.com/miekg OK, this seems good. If you or @porjo
https://github.com/porjo want to tackle this let me know!


Reply to this email directly or view it on GitHub
#27 (comment).

@porjo
Copy link

porjo commented Aug 5, 2014

Can someone explain to me why anyone would care about socket ordering? Is it worth the extra level of indirection?

@theraphim
Copy link
Author

Socket ordering might matter if I am having different levels of service on different items.
For example I can run a websocket/http on first tcp port, tftp on first udp port, custom proto on second tcp port, something else on second udp port.
If you don't like the indirection, and want to have udp and tcp sockets returned by separate functions, what about:
func Listeners(unsetEnv bool) ([]net.Listener, []int, error)
and similarly for PacketConns

that way, if you don't care about the order, just ls, _, err := it

@porjo
Copy link

porjo commented Aug 5, 2014

@theraphim That makes sense but isn't the sequence of the sockets within the slice itself sufficient ordering for this case? i.e. why do we need an additional Order field?

@philips
Copy link
Contributor

philips commented Aug 5, 2014

No because the app might rxpect UDP and TCP sockets.
On Aug 5, 2014 3:27 PM, "Ian Bishop" notifications@github.com wrote:

@theraphim https://github.com/theraphim That makes sense but isn't the
sequence of the sockets within the slice itself sufficient ordering for
this case? i.e. why do we need an additional Order field?


Reply to this email directly or view it on GitHub
#27 (comment).

@porjo
Copy link

porjo commented Aug 5, 2014

No because the app might rxpect UDP and TCP sockets

I'm still not seeing how the Order field would be used in practice. Can you provide some code to demonstrate?

For the scenario where you have multiple services running from one process, the app would have internal config (taken from command line args for example) telling it which protocol/port it should use for which service. It should then be matching those up with what go-systemd returns. E.g. tftp service has been defined to use UDP/2121: iterate over []PacketConns and look for matching entry. If none found, then we have an error condition. This way order is not important.

@philips
Copy link
Contributor

philips commented Aug 5, 2014

@porjo Sockets can be configured in a specific order. From the systemd docs:

"Sockets configured in one unit are passed in the order of configuration, but no ordering between socket units is specified."

http://www.freedesktop.org/software/systemd/man/systemd.socket.html

@theraphim
Copy link
Author

IIRC systemd passes sockets down in order they're specified in a corresponding .socket file; therefore I can reasonably assign different service levels depending on the order they're coming in the environment.

@porjo
Copy link

porjo commented Aug 6, 2014

@philips @theraphim Yes, I know sockets are sent from Systemd in a particular order and I see now why this is useful. My question is why the Order field is necessary i.e. why can't the caller use the slice's sequence as the socket order? For example,

    packetConns, _ := activation.PacketConns(false)
    listeners, _ := activation.Listeners(true)

    // Start our tftp service on the first UDP port returned
    tftpConn := packetConns[0]

    // start our api on the second TCP port returned
    apiListener := listeners[1]

    // and so on...

I've updated my branch and replaced range with a traditional for loop, to ensure order is preserved.

@philips
Copy link
Contributor

philips commented Aug 6, 2014

@porjo This seems reasonable to me, no idea why I didn't think of it before the Order thing... let me know when you have a PR ready! :)

@porjo
Copy link

porjo commented Aug 6, 2014

@philips If you can reopen #54 then it's ready there, otherwise I'll send another PR.

@philips
Copy link
Contributor

philips commented Aug 6, 2014

On Tue, Aug 5, 2014 at 7:55 PM, Ian Bishop notifications@github.com wrote:

@philips https://github.com/philips If you can reopen #54
#54 then it's ready there,
otherwise I'll send another PR.

Reopened.

@philips philips closed this Aug 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants