[lxc-devel] [PATCH] shutdown: Rework API and lxc-stop
Stéphane Graber
stgraber at ubuntu.com
Fri Jan 31 17:55:43 UTC 2014
On Fri, Jan 31, 2014 at 05:51:47PM +0000, Stéphane Graber wrote:
> On Fri, Jan 31, 2014 at 05:41:02PM +0000, Serge Hallyn wrote:
> > Quoting Stéphane Graber (stgraber at ubuntu.com):
> > > With this change, shutdown() will no longer call stop() after the
> > > timeout, instead it'll just return false and it's up to the caller to
> > > then call stop() if appropriate.
> > >
> > > This also updates the bindings, tests and other scripts.
> > >
> > > lxc-stop is then updated to do proper option checking and use shutdown,
> > > stop or reboot as appropriate.
> > >
> > > Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
> >
> > Two questions below. Provided the answer is "that's what I wanted",
> > then
> >
> > Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> >
> > > ---
> > > src/lxc/lxc-start-ephemeral.in | 6 ++--
> > > src/lxc/lxc_stop.c | 64 ++++++++++++++++++++++++++++++++-----
> > > src/lxc/lxccontainer.c | 4 ---
> > > src/lxc/lxccontainer.h | 8 ++---
> > > src/python-lxc/examples/api_test.py | 3 +-
> > > src/python-lxc/lxc.c | 5 ++-
> > > src/tests/shutdowntest.c | 3 ++
> > > 7 files changed, 70 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/src/lxc/lxc-start-ephemeral.in b/src/lxc/lxc-start-ephemeral.in
> > > index d40ce4e..62a6194 100644
> > > --- a/src/lxc/lxc-start-ephemeral.in
> > > +++ b/src/lxc/lxc-start-ephemeral.in
> > > @@ -270,7 +270,8 @@ if not dest.start() or not dest.wait("RUNNING", timeout=5):
> > > # Deal with the case where we just attach to the container's console
> > > if not args.command and not args.daemon:
> > > dest.console()
> > > - dest.shutdown(timeout=5)
> > > + if not dest.shutdown(timeout=5):
> > > + dest.stop()
> > > sys.exit(0)
> > >
> > > # Try to get the IP addresses
> > > @@ -318,6 +319,7 @@ for ip in ips:
> > > break
> > >
> > > # Shutdown the container
> > > -dest.shutdown(timeout=5)
> > > +if not dest.shutdown(timeout=5):
> > > + dest.stop()
> > >
> > > sys.exit(retval)
> > > diff --git a/src/lxc/lxc_stop.c b/src/lxc/lxc_stop.c
> > > index d8de2b0..47db9e8 100644
> > > --- a/src/lxc/lxc_stop.c
> > > +++ b/src/lxc/lxc_stop.c
> > > @@ -77,7 +77,7 @@ Options :\n\
> > > .options = my_longopts,
> > > .parser = my_parser,
> > > .checker = NULL,
> > > - .timeout = 60,
> > > + .timeout = -2,
> > > };
> > >
> > > /* returns -1 on failure, 0 on success */
> > > @@ -95,7 +95,7 @@ static int do_reboot_and_check(struct lxc_arguments *a, struct lxc_container *c)
> > > return -1;
> > > if (a->nowait)
> > > return 0;
> > > - if (timeout <= 0)
> > > + if (timeout == 0)
> > > goto out;
> > >
> > > for (;;) {
> > > @@ -118,7 +118,7 @@ static int do_reboot_and_check(struct lxc_arguments *a, struct lxc_container *c)
> > > if (ret)
> > > break;
> > > elapsed_time = tv.tv_sec - curtime;
> > > - if (timeout - elapsed_time <= 0)
> > > + if (timeout != -1 && timeout - elapsed_time <= 0)
> > > break;
> > > timeout -= elapsed_time;
> > > }
> > > @@ -145,6 +145,51 @@ int main(int argc, char *argv[])
> > > my_args.progname, my_args.quiet, my_args.lxcpath[0]))
> > > return 1;
> > >
> > > + /* Set default timeout */
> > > + if (my_args.timeout == -2) {
> > > + if (my_args.hardstop) {
> > > + my_args.timeout = 0;
> > > + }
> > > + else {
> > > + my_args.timeout = 60;
> > > + }
> > > + }
> > > +
> > > + if (my_args.nowait) {
> > > + my_args.timeout = 0;
> > > + }
> > > +
> > > + /* some checks */
> > > + if (!my_args.hardstop && my_args.timeout < -1) {
> >
> > Is this (< -1) what you actually meant? What other value besides
> > -2 that is < -1 is valid here?
>
> None which is why it prints an error message if a negative value other
> than -1 is passed.
>
> >
> > > + fprintf(stderr, "invalid timeout\n");
> > > + return 1;
> > > + }
> > > +
> > > + if (my_args.hardstop && my_args.nokill) {
> > > + fprintf(stderr, "-k can't be used with --nokill\n");
> > > + return 1;
> > > + }
> > > +
> > > + if (my_args.hardstop && my_args.reboot) {
> > > + fprintf(stderr, "-k can't be used with -r\n");
> > > + return 1;
> > > + }
> > > +
> > > + if (my_args.hardstop && my_args.timeout) {
> > > + fprintf(stderr, "-k doesn't allow timeouts\n");
> > > + return 1;
> > > + }
> > > +
> > > + if (my_args.nolock && !my_args.hardstop) {
> > > + fprintf(stderr, "--nolock may only be used with -k\n");
> > > + return 1;
> > > + }
> > > +
> > > + if (my_args.nokill && my_args.timeout < 1) {
> > > + fprintf(stderr, "--nokill requires a positive timeout\n");
> > > + return 1;
> > > + }
> >
> > Oh, I thought you were going to have --no-kill -t 0 simply wait
> > forever for the shutdown to complete. This is fine, but I just
> > want to make sure it's what you wanted.
>
> lxc-stop -n p1 -t 0 is what you're describing (sends SIGPWR and exits 0
> immediately).
That's wrong and the --nokill indeed doesn't seem to be properly
handled, I'll think about this, fix it and send a V2.
>
> >
> > > +
> > > /* shortcut - if locking is bogus, we should be able to kill
> > > * containers at least */
> > > if (my_args.nolock)
> > > @@ -167,22 +212,25 @@ int main(int argc, char *argv[])
> > > goto out;
> > > }
> > >
> > > + /* kill */
> > > if (my_args.hardstop) {
> > > ret = c->stop(c) ? 0 : 1;
> > > goto out;
> > > }
> > > +
> > > + /* reboot */
> > > if (my_args.reboot) {
> > > ret = do_reboot_and_check(&my_args, c);
> > > goto out;
> > > }
> > >
> > > - if (my_args.nokill)
> > > - my_args.timeout = 0;
> > > -
> > > + /* shutdown */
> > > s = c->shutdown(c, my_args.timeout);
> > > if (!s) {
> > > - if (!my_args.shutdown)
> > > - ret = c->wait(c, "STOPPED", -1) ? 0 : 1;
> > > + if (!my_args.nokill)
> > > + ret = c->stop(c) ? 0 : 1;
> > > + else if (my_args.timeout == 0)
> > > + ret = 0;
> > > else
> > > ret = 1; // fail
> > > } else
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index a0a5af5..85c644a 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -1348,10 +1348,6 @@ static bool lxcapi_shutdown(struct lxc_container *c, int timeout)
> > > haltsignal = c->lxc_conf->haltsignal;
> > > kill(pid, haltsignal);
> > > retv = c->wait(c, "STOPPED", timeout);
> > > - if (!retv && timeout > 0) {
> > > - c->stop(c);
> > > - retv = c->wait(c, "STOPPED", 0); // 0 means don't wait
> > > - }
> > > return retv;
> > > }
> > >
> > > diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
> > > index 5f5d9b2..92c76b4 100644
> > > --- a/src/lxc/lxccontainer.h
> > > +++ b/src/lxc/lxccontainer.h
> > > @@ -362,12 +362,10 @@ struct lxc_container {
> > > * SIGPWR.
> > > *
> > > * \param c Container.
> > > - * \param timeout Seconds to wait before forcing a hard stop
> > > - * (value must be >0).
> > > + * \param timeout Seconds to wait before returning false.
> > > + * (-1 to wait forever, 0 to avoid waiting).
> > > *
> > > - * \return \c true if configuration was loaded successfully, else \c false.
> > > - *
> > > - * \note A \p timeout of \c 0 means do not wait.
> > > + * \return \c true if the container was shutdown successfully, else \c false.
> > > */
> > > bool (*shutdown)(struct lxc_container *c, int timeout);
> > >
> > > diff --git a/src/python-lxc/examples/api_test.py b/src/python-lxc/examples/api_test.py
> > > index f72cc56..a1fe6f9 100755
> > > --- a/src/python-lxc/examples/api_test.py
> > > +++ b/src/python-lxc/examples/api_test.py
> > > @@ -144,7 +144,8 @@ if len(sys.argv) > 1 and sys.argv[1] == "--with-console":
> > >
> > > ## Shutting down the container
> > > print("Shutting down the container")
> > > -container.shutdown(3)
> > > +if not container.shutdown(3):
> > > + container.stop()
> > >
> > > if container.running:
> > > print("Stopping the container")
> > > diff --git a/src/python-lxc/lxc.c b/src/python-lxc/lxc.c
> > > index 05727bf..9fdc27f 100644
> > > --- a/src/python-lxc/lxc.c
> > > +++ b/src/python-lxc/lxc.c
> > > @@ -1591,9 +1591,8 @@ static PyMethodDef Container_methods[] = {
> > > METH_VARARGS|METH_KEYWORDS,
> > > "shutdown(timeout = -1) -> boolean\n"
> > > "\n"
> > > - "Sends SIGPWR to the container and wait for it to shutdown "
> > > - "unless timeout is set to a positive value, in which case "
> > > - "the container will be killed when the timeout is reached."
> > > + "Sends SIGPWR to the container and wait for it to shutdown."
> > > + "-1 means wait forever, 0 means skip waiting."
> > > },
> > > {"snapshot", (PyCFunction)Container_snapshot,
> > > METH_VARARGS|METH_KEYWORDS,
> > > diff --git a/src/tests/shutdowntest.c b/src/tests/shutdowntest.c
> > > index 6e4bb31..c5d609d 100644
> > > --- a/src/tests/shutdowntest.c
> > > +++ b/src/tests/shutdowntest.c
> > > @@ -69,6 +69,9 @@ int main(int argc, char *argv[])
> > > goto out;
> > > }
> > >
> > > + /* Wait for init to be ready for SIGPWR */
> > > + sleep(10);
> > > +
> > > if (!c->shutdown(c, 60)) {
> > > fprintf(stderr, "%d: failed to shut down %s\n", __LINE__, MYNAME);
> > > goto out;
> > > --
> > > 1.9.rc1
> > >
> > > _______________________________________________
> > > lxc-devel mailing list
> > > lxc-devel at lists.linuxcontainers.org
> > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
>
> --
> Stéphane Graber
> Ubuntu developer
> http://www.ubuntu.com
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140131/a205643f/attachment.pgp>
More information about the lxc-devel
mailing list