Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On 2014/1/21 11:38, Stéphane Graber wrote: > On Tue, Jan 21, 2014 at 11:33:42AM +0800, Qiang Huang wrote: >> On 2014/1/21 11:20, Stéphane Graber wrote: >>> On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote: Hi Stéphane, I'm not sure I understand this. In my understanding, anything using the API to control an already running backgrounded container will use lxc_container_new to create a new lxc_container, it only share the original one's name and config and so on. After dealing, will call lxc_container_put to free this lxc_container, this *new* lxc_container doesn't contain the PID file information, so it's freeing won't remove PID file. >>> >>> Hmm, yeah, seems like you're right, I should have rechecked the patch >>> rather than speak from the vague memory of what I saw when you initialy >>> posted it. >>> >>> >>> So, re-reading more carefully, I don't completely disagree with the >>> approach, though I don't like having to extend lxc_container just for >>> that... >> >> Without 1/2 patch, this patch won't work, I'll send new patches. >> So would it be better if I put this into lxc_conf? >> >>> >>> There's however one rather big problem, the PID file will contain the >>> wrong PID. The PID being written is getpid() from the parent lxc-start, >>> not the running, forked lxc-start process and that child PID is only >>> known by the start() function in lxccontainer.c, so you can't create the >>> PID file from lxc_start.c >> >> Yeah, I noticed that before, I thought that's exactly what we want, >> because we can get the init pid of container from lxc_info command. >> >> If it's not, we can fix it incrementally. :) > > No, what I meant is that the PID we are writing is that of a > non-existing process, which makes the pid file entirely useless. > > When you start a container, you have at least 3 processes: > 1) The command the user start (lxc-start -d) > 2) The backgrounded fork of that command after start() is done > 3) The container init process > > lxc-info and the API init_pid() function gives us (3), what we want to > see in a pidfile is (2) and what we are currently writing is (1). > > As (1) exits as soon as the container is started, it's completely > useless as you can't use that to check whether LXC still has control > over the container (i.e. is lxc-start still running in the background). > Oh, that's right, in the daemon model the PID file stores wrong pid, I'll think about it and fix it in the new patch. Thanks Stéphane. ___ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On Tue, Jan 21, 2014 at 11:33:42AM +0800, Qiang Huang wrote: > On 2014/1/21 11:20, Stéphane Graber wrote: > > On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote: > >> Hi Stéphane, > >> > >> I'm not sure I understand this. > >> > >> In my understanding, anything using the API to control an already > >> running backgrounded container will use lxc_container_new to create > >> a new lxc_container, it only share the original one's name and config > >> and so on. After dealing, will call lxc_container_put to free this > >> lxc_container, this *new* lxc_container doesn't contain the PID file > >> information, so it's freeing won't remove PID file. > > > > Hmm, yeah, seems like you're right, I should have rechecked the patch > > rather than speak from the vague memory of what I saw when you initialy > > posted it. > > > > > > So, re-reading more carefully, I don't completely disagree with the > > approach, though I don't like having to extend lxc_container just for > > that... > > Without 1/2 patch, this patch won't work, I'll send new patches. > So would it be better if I put this into lxc_conf? > > > > > There's however one rather big problem, the PID file will contain the > > wrong PID. The PID being written is getpid() from the parent lxc-start, > > not the running, forked lxc-start process and that child PID is only > > known by the start() function in lxccontainer.c, so you can't create the > > PID file from lxc_start.c > > Yeah, I noticed that before, I thought that's exactly what we want, > because we can get the init pid of container from lxc_info command. > > If it's not, we can fix it incrementally. :) No, what I meant is that the PID we are writing is that of a non-existing process, which makes the pid file entirely useless. When you start a container, you have at least 3 processes: 1) The command the user start (lxc-start -d) 2) The backgrounded fork of that command after start() is done 3) The container init process lxc-info and the API init_pid() function gives us (3), what we want to see in a pidfile is (2) and what we are currently writing is (1). As (1) exits as soon as the container is started, it's completely useless as you can't use that to check whether LXC still has control over the container (i.e. is lxc-start still running in the background). -- Stéphane Graber Ubuntu developer http://www.ubuntu.com signature.asc Description: Digital signature ___ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On 2014/1/21 11:20, Stéphane Graber wrote: > On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote: >> Hi Stéphane, >> >> I'm not sure I understand this. >> >> In my understanding, anything using the API to control an already >> running backgrounded container will use lxc_container_new to create >> a new lxc_container, it only share the original one's name and config >> and so on. After dealing, will call lxc_container_put to free this >> lxc_container, this *new* lxc_container doesn't contain the PID file >> information, so it's freeing won't remove PID file. > > Hmm, yeah, seems like you're right, I should have rechecked the patch > rather than speak from the vague memory of what I saw when you initialy > posted it. > > > So, re-reading more carefully, I don't completely disagree with the > approach, though I don't like having to extend lxc_container just for > that... Without 1/2 patch, this patch won't work, I'll send new patches. So would it be better if I put this into lxc_conf? > > There's however one rather big problem, the PID file will contain the > wrong PID. The PID being written is getpid() from the parent lxc-start, > not the running, forked lxc-start process and that child PID is only > known by the start() function in lxccontainer.c, so you can't create the > PID file from lxc_start.c Yeah, I noticed that before, I thought that's exactly what we want, because we can get the init pid of container from lxc_info command. If it's not, we can fix it incrementally. :) > ___ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote:
> Hi Stéphane,
>
> On 2014/1/21 0:07, Stéphane Graber wrote:
> > On Mon, Jan 20, 2014 at 03:22:31PM +0800, Qiang Huang wrote:
> >> On 2014/1/19 8:57, Stéphane Graber wrote:
> >>> On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
> This is for bug: https://github.com/lxc/lxc/issues/89
>
> When start container with daemon model, the daemon process's
> father will return back to main in start time, and pidfile
> is removed then, that's not right.
> So we store pidfile to lxc_container, and remove it when
> lxc_container_free.
> >>>
> >>> That one looks wrong to me, removing the pid file on lxc_container_free
> >>> is wrong. We want the pidfile removed when the background lxc-start
> >>> exits, not whenever a random API client flushes the container from
> >>> memory.
> >>>
> >>> With your patch, doing something like:
> >>> - lxc-start -n p1 -d -p /tmp/pid
> >>> - python3
> >>> import lxc
> >>> p1 = lxc.Container("p1")
> >>> p1 = None
> >>
> >> I'm sorry I'm not family with python, can you explain how this would
> >> happen in real world? Thanks.
> >
> > In the real world, anything using the API to control an already running
> > backgrounded container with a PID file and that does lxc_container_put
> > once it's done dealing with the container object will cause the PID file
> > to be removed.
>
> I'm not sure I understand this.
>
> In my understanding, anything using the API to control an already
> running backgrounded container will use lxc_container_new to create
> a new lxc_container, it only share the original one's name and config
> and so on. After dealing, will call lxc_container_put to free this
> lxc_container, this *new* lxc_container doesn't contain the PID file
> information, so it's freeing won't remove PID file.
Hmm, yeah, seems like you're right, I should have rechecked the patch
rather than speak from the vague memory of what I saw when you initialy
posted it.
So, re-reading more carefully, I don't completely disagree with the
approach, though I don't like having to extend lxc_container just for
that...
There's however one rather big problem, the PID file will contain the
wrong PID. The PID being written is getpid() from the parent lxc-start,
not the running, forked lxc-start process and that child PID is only
known by the start() function in lxccontainer.c, so you can't create the
PID file from lxc_start.c
>
> Anyway, my patch is based on one theory:
> A running container will have an lxc_container to hold it's information,
> which is created from start, the struct lxc_container lasts for all of
> container's life cycle.
>
> If it's wrong, my question is:
> How could it be reasonable for a container running without an lxc_container?
>
> >
> > Actually, even calling lxc-list should cause the PID file to be removed
> > as lxc-list calls list_containers which calls list_defined_containers
> > which in turns iterate through all containers, get their struct and then
> > lxc_container_put them.
> >
> >>
> >>>
> >>> Or the equivalent with any binding, or directly in C, will destroy the
> >>> pid file even though the container is clearly still running.
> >>
> >> I thought when the backgroud lxc-start exits, it's time for container
> >> to free, because there are no other place to do lxc_contaier_get to
> >> hold the container from freeing.
> >>
> >> I must missed something :( , so waiting for your more details.
> >>
> >>
> >
>
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
Hi Stéphane,
On 2014/1/21 0:07, Stéphane Graber wrote:
> On Mon, Jan 20, 2014 at 03:22:31PM +0800, Qiang Huang wrote:
>> On 2014/1/19 8:57, Stéphane Graber wrote:
>>> On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
This is for bug: https://github.com/lxc/lxc/issues/89
When start container with daemon model, the daemon process's
father will return back to main in start time, and pidfile
is removed then, that's not right.
So we store pidfile to lxc_container, and remove it when
lxc_container_free.
>>>
>>> That one looks wrong to me, removing the pid file on lxc_container_free
>>> is wrong. We want the pidfile removed when the background lxc-start
>>> exits, not whenever a random API client flushes the container from
>>> memory.
>>>
>>> With your patch, doing something like:
>>> - lxc-start -n p1 -d -p /tmp/pid
>>> - python3
>>> import lxc
>>> p1 = lxc.Container("p1")
>>> p1 = None
>>
>> I'm sorry I'm not family with python, can you explain how this would
>> happen in real world? Thanks.
>
> In the real world, anything using the API to control an already running
> backgrounded container with a PID file and that does lxc_container_put
> once it's done dealing with the container object will cause the PID file
> to be removed.
I'm not sure I understand this.
In my understanding, anything using the API to control an already
running backgrounded container will use lxc_container_new to create
a new lxc_container, it only share the original one's name and config
and so on. After dealing, will call lxc_container_put to free this
lxc_container, this *new* lxc_container doesn't contain the PID file
information, so it's freeing won't remove PID file.
Anyway, my patch is based on one theory:
A running container will have an lxc_container to hold it's information,
which is created from start, the struct lxc_container lasts for all of
container's life cycle.
If it's wrong, my question is:
How could it be reasonable for a container running without an lxc_container?
>
> Actually, even calling lxc-list should cause the PID file to be removed
> as lxc-list calls list_containers which calls list_defined_containers
> which in turns iterate through all containers, get their struct and then
> lxc_container_put them.
>
>>
>>>
>>> Or the equivalent with any binding, or directly in C, will destroy the
>>> pid file even though the container is clearly still running.
>>
>> I thought when the backgroud lxc-start exits, it's time for container
>> to free, because there are no other place to do lxc_contaier_get to
>> hold the container from freeing.
>>
>> I must missed something :( , so waiting for your more details.
>>
>>
>
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On Mon, Jan 20, 2014 at 03:22:31PM +0800, Qiang Huang wrote:
> On 2014/1/19 8:57, Stéphane Graber wrote:
> > On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
> >> This is for bug: https://github.com/lxc/lxc/issues/89
> >>
> >> When start container with daemon model, the daemon process's
> >> father will return back to main in start time, and pidfile
> >> is removed then, that's not right.
> >> So we store pidfile to lxc_container, and remove it when
> >> lxc_container_free.
> >
> > That one looks wrong to me, removing the pid file on lxc_container_free
> > is wrong. We want the pidfile removed when the background lxc-start
> > exits, not whenever a random API client flushes the container from
> > memory.
> >
> > With your patch, doing something like:
> > - lxc-start -n p1 -d -p /tmp/pid
> > - python3
> > import lxc
> > p1 = lxc.Container("p1")
> > p1 = None
>
> I'm sorry I'm not family with python, can you explain how this would
> happen in real world? Thanks.
In the real world, anything using the API to control an already running
backgrounded container with a PID file and that does lxc_container_put
once it's done dealing with the container object will cause the PID file
to be removed.
Actually, even calling lxc-list should cause the PID file to be removed
as lxc-list calls list_containers which calls list_defined_containers
which in turns iterate through all containers, get their struct and then
lxc_container_put them.
>
> >
> > Or the equivalent with any binding, or directly in C, will destroy the
> > pid file even though the container is clearly still running.
>
> I thought when the backgroud lxc-start exits, it's time for container
> to free, because there are no other place to do lxc_contaier_get to
> hold the container from freeing.
>
> I must missed something :( , so waiting for your more details.
>
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On 2014/1/19 8:57, Stéphane Graber wrote:
> On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
>> This is for bug: https://github.com/lxc/lxc/issues/89
>>
>> When start container with daemon model, the daemon process's
>> father will return back to main in start time, and pidfile
>> is removed then, that's not right.
>> So we store pidfile to lxc_container, and remove it when
>> lxc_container_free.
>
> That one looks wrong to me, removing the pid file on lxc_container_free
> is wrong. We want the pidfile removed when the background lxc-start
> exits, not whenever a random API client flushes the container from
> memory.
>
> With your patch, doing something like:
> - lxc-start -n p1 -d -p /tmp/pid
> - python3
> import lxc
> p1 = lxc.Container("p1")
> p1 = None
I'm sorry I'm not family with python, can you explain how this would
happen in real world? Thanks.
>
> Or the equivalent with any binding, or directly in C, will destroy the
> pid file even though the container is clearly still running.
I thought when the backgroud lxc-start exits, it's time for container
to free, because there are no other place to do lxc_contaier_get to
hold the container from freeing.
I must missed something :( , so waiting for your more details.
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped
On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
> This is for bug: https://github.com/lxc/lxc/issues/89
>
> When start container with daemon model, the daemon process's
> father will return back to main in start time, and pidfile
> is removed then, that's not right.
> So we store pidfile to lxc_container, and remove it when
> lxc_container_free.
That one looks wrong to me, removing the pid file on lxc_container_free
is wrong. We want the pidfile removed when the background lxc-start
exits, not whenever a random API client flushes the container from
memory.
With your patch, doing something like:
- lxc-start -n p1 -d -p /tmp/pid
- python3
import lxc
p1 = lxc.Container("p1")
p1 = None
Or the equivalent with any binding, or directly in C, will destroy the
pid file even though the container is clearly still running.
>
> Signed-off-by: Qiang Huang
> ---
> src/lxc/lxc_start.c| 9 +
> src/lxc/lxccontainer.c | 6 ++
> src/lxc/lxccontainer.h | 6 ++
> 3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
> index d5379da..fd2dc6e 100644
> --- a/src/lxc/lxc_start.c
> +++ b/src/lxc/lxc_start.c
> @@ -302,6 +302,11 @@ int main(int argc, char *argv[])
> }
>
> if (my_args.pidfile != NULL) {
> + if (ensure_path(&c->pidfile, my_args.pidfile) < 0) {
> + ERROR("failed to ensure pidfile '%s'", my_args.pidfile);
> + goto out;
> + }
> +
> pid_fp = fopen(my_args.pidfile, "w");
> if (pid_fp == NULL) {
> SYSERROR("failed to create pidfile '%s' for '%s'",
> @@ -342,10 +347,6 @@ int main(int argc, char *argv[])
> c->want_close_all_fds(c, true);
>
> err = c->start(c, 0, args) ? 0 : -1;
> -
> - if (my_args.pidfile)
> - unlink(my_args.pidfile);
> -
> out:
> lxc_container_put(c);
> if (pid_fp)
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index ddea0d7..d742781 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -231,6 +231,11 @@ static void lxc_container_free(struct lxc_container *c)
> free(c->config_path);
> c->config_path = NULL;
> }
> + if (c->pidfile) {
> + unlink(c->pidfile);
> + free(c->pidfile);
> + c->pidfile = NULL;
> + }
> free(c);
> }
>
> @@ -3051,6 +3056,7 @@ struct lxc_container *lxc_container_new(const char
> *name, const char *configpath
> lxcapi_clear_config(c);
> }
> c->daemonize = true;
> + c->pidfile = NULL;
>
> // assign the member functions
> c->is_defined = lxcapi_is_defined;
> diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
> index 921e47d..a860648 100644
> --- a/src/lxc/lxccontainer.h
> +++ b/src/lxc/lxccontainer.h
> @@ -68,6 +68,12 @@ struct lxc_container {
>
> /*!
>* \private
> + * File to store pid.
> + */
> + char *pidfile;
> +
> + /*!
> + * \private
>* Container semaphore lock.
>*/
> struct lxc_lock *slock;
> --
> 1.8.3
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
