[lxc-devel] [RFC PATCH] allow multiple monitor clients

[Serge Hallyn]

Quoting Dwight Engen (dwight.engen at oracle.com):
> After the recent discussions on the lxc monitor, I took a closer look at
> it. It seems like Serge was resigned to having some sort of daemon as
> having multicast unix domain supported by the kernel is not going to
> happen any time soon, and doing it over loopback has some
> complications too.
> 
> Initially I thought it would be nice to just have lxc-start "be the
> daemon" as Stéphane had suggested. I tried this out, putting the fds
> for the consumer clients (lxc-monitors) in lxc_mainloop , but this is
> problematic for the scenario where lxc-monitor runs first and we miss
> some startup states.
> 
> So here is what I'm proposing: when lxc-monitor starts, it attempts to
> start lxc-monitord. lxc-monitord creates a fifo and a socket on lxcpath.

Thanks, Dwight.  Looks awesome.  Some comments below, but I'm only
not adding an ack bc you say you want to make some changes first
anyway.

When you send the next version I'll run it through my testsuite (and
presumably ack).

> Producers (containers) wishing to send messages open the fifo and
> write their lxc_msg's on it. The reason I used a fifo is so that we get
> permission checks on the opening of it. Right now it is created 0600.
> This fixes the problem we have today where anyone can open the unix
> socket and send state changes in.
> 
> Consumers (lxc-monitor) connect to the unix socket and each gets a copy
> of the messages. lxc-monitord can do a PEERCRED to ensure the client
> should be able to get these messages, this is one benefit to using unix
> domain. I don't think it would be difficult to write a lxc-monitor
> like client that would bridge the messages onto d-bus.
> 
> When lxc-monitord hasn't had a client for 30 seconds it will exit.
> 
> Attached is a proof of concept patch for your comments. It works for
> the common cases that I've tried but I know it needs some cleanup and
> testing if we like the approach. The monitor.c part is hard to read
> because that file is mostly rewritten.
> 
> I need to test S.Çağlar Onur's scenario of parallel startup with his
> test script. We need to decide if we want to monitor per lxcpath or per
> container,

Do you mean 'per lxcpath or per host'?  We definately don't want per
container.

> today it is per path and I think that being able to do
> lxc-monitor * would be very useful.

If by that you mean 'watch all paths', well - I don't think * is
as useful, but certainly 'lxc-monitor /var/lib/lxc /home/serge/lxcbase'
would be nice to do.  But we can do that later with your monitord,
just by watching both sockets, right?

> 
> --
> 
> Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> ---
...
> +static void lxc_monitord_delete(struct lxc_monitor *mon, const char *lxcpath)
> +{
> +	int i;
> +
> +	lxc_mainloop_del_handler(&mon->descr, mon->listenfd);
> +	close(mon->listenfd);

The ordering here might need to change to ensure we don't get any
clients hanging between the two steps.

...

> +/* routines used by monitor subscribers (lxc-monitor) */
> +int lxc_monitor_close(int fd)
>  {
> -	struct sockaddr_un addr = { .sun_family = AF_UNIX };
> -	char *offset = &addr.sun_path[1];
> -	int fd;
> -	size_t ret, len;
> +	return close(fd);
> +}
>  
> -	/*
> -	 * addr.sun_path is only 108 bytes.
> -	 * should we take a hash of lxcpath?  a subset of it?
> +int lxc_monitor_sock_name(const char *lxcpath, struct sockaddr_un *addr) {
> +	size_t len,ret;
> +	char *sockname = &addr->sun_path[0]; // 1 for abstract
> +
> +	/* addr.sun_path is only 108 bytes.
> +	 * should we take a hash of lxcpath?  a subset of it? ftok()? none
> +	 * are guaranteed.

guaranteed to what?  The hash should be pretty immune to duplicates, and
does guarantee the length.  The problem with that is simply that we lose
the ability to easily predict (without a tool :) the abstract socket
name and type it by hand.

...
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index aefccd6..9451fb7 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -429,6 +429,9 @@ struct lxc_handler *lxc_init(const char *name, struct lxc_conf *conf, const char
>  	if (lxc_command_init(name, handler, lxcpath))
>  		goto out_free_name;
>  
> +	if (lxc_monitord_spawn(lxcpath))
> +		goto out_close_maincmd_fd;

Can I ask why you're doing this?  I don't think it's necessary.  Simply
accept that we might send events that get ignored (as we do now).

> +
>  	if (lxc_read_seccomp_config(conf) != 0) {
>  		ERROR("failed loading seccomp policy");
>  		goto out_close_maincmd_fd;
> @@ -437,7 +440,7 @@ struct lxc_handler *lxc_init(const char *name, struct lxc_conf *conf, const char
>  	/* Begin the set the state to STARTING*/
>  	if (lxc_set_state(name, handler, STARTING)) {
>  		ERROR("failed to set state '%s'", lxc_state2str(STARTING));
> -		goto out_free_name;
> +		goto out_close_maincmd_fd;
>  	}
>  
>  	/* Start of environment variable setup for hooks */
> @@ -808,7 +811,7 @@ int lxc_spawn(struct lxc_handler *handler)
>  	/* TODO - pass lxc.cgroup.dir (or user's pam cgroup) in for first argument */
>  	if ((handler->cgroup = lxc_cgroup_path_create(NULL, name)) == NULL)
>  		goto out_delete_net;
> -	
> +
>  	if (lxc_cgroup_enter(handler->cgroup, handler->pid) < 0)
>  		goto out_delete_net;
>  
> -- 
> 1.8.1.4
> 
> 
> 
> ------------------------------------------------------------------------------
> Precog is a next-generation analytics platform capable of advanced
> analytics on semi-structured data. The platform includes APIs for building
> apps and a phenomenal toolset for data science. Developers can use
> our toolset for easy data analysis & visualization. Get a free account!
> http://www2.precog.com/precogplatform/slashdotnewsletter
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel