Instance: Set receive timeout for LXC command client sock #11293

mihalicyn · 2023-01-17T18:41:21Z

Issue #4257

Signed-off-by: Alexander Mikhalitsyn aleksandr.mikhalitsyn@canonical.com

Please see lxc/lxc#4257

mihalicyn · 2023-01-17T18:43:57Z

We may want to use liblxc.HasAPIExtension(extension) to detect if set timeout is supported or not. Let's decide.

lxc/go-lxc#166
lxc/lxc#4260

tomponline · 2023-01-18T09:11:45Z

lxd/instance/drivers/driver_lxc.go

@@ -632,6 +632,12 @@ func (d *lxc) initLXC(config bool) error {
 		return err
 	}

+	// To discuss: take defalt timeout value from some global config?
+	err = liblxc.SetTimeout(time.Duration(10) * time.Second)


I don't think we need to make it configurable.
However what happens if we, say, set a 10s timeout by default on init, as you're doing here, will this affect a longer timeout passed to d.c.Shutdown()?

Also, can se set the timeout on the cc returned from liblxc.NewContainer rather than on the library as a whole?

Also, can se set the timeout on the cc returned from liblxc.NewContainer rather than on the library as a whole?

sure, it's just mistake. Thanks!

I don't think we need to make it configurable. However what happens if we, say, set a 10s timeout by default on init, as you're doing here, will this affect a longer timeout passed to d.c.Shutdown()?

Yep, that's what I've said in the original issue. That's why right now this timeout is not applied to all commands. We will need to extend liblxc and each command one by one to support this timeout. [like this commit ] And I'm not sure that we really need that. So, it's thing to discuss.

OK so right now this would just record the timeout value but not use it, and then you would extend each underlying call to use it. And we wouldn't need to update d.c.Shutdown() to use it because it already has its own timeout support?

Right now, for Shutdown this container-wide timeout will be ignored at all.

You want to propose to take this "container-wide timeout" as a default value for Shutdown call (if timeout specified as -1)?

well in Go at least we cannot specify -1, but we could have it use the container wide timeout if the timeout passed was zero perhaps?

That's an interesting point.

What we have in the liblxc:

/*! * \brief Request the container shutdown by sending it \c * SIGPWR. * * \param c Container. * \param timeout Seconds to wait before returning false. * (-1 to wait forever, 0 to avoid waiting). * * \return \c true if the container was shutdown successfully, else \c false. */ bool (*shutdown)(struct lxc_container *c, int timeout);

In the go-lxc:

// Shutdown shuts down the container. func (c *Container) Shutdown(timeout time.Duration) error { c.mu.Lock() defer c.mu.Unlock() if c.container == nil { return ErrNotDefined } if err := c.makeSure(isRunning); err != nil { return err } if !bool(C.go_lxc_shutdown(c.container, C.int(timeout.Seconds()))) { return ErrShutdownFailed } return nil }

I don't think that it would be correct to change semantics of liblxc shutdown call. But we can change go-lxc's Shutdown implementation to take the "container-wide timeout" as a default in case when timeout is 0. Or, better, to add another method like ShutdownWithTimeout.

We don't need ShutdownWithTimeout as we already have Shutdown(timeout time.Duration).

I propose we just ignore the container wide timeout for the Shutdown(timeout time.Duration) function then.

I propose we just ignore the container wide timeout for the Shutdown(timeout time.Duration) function then.

It's just the current implementation behavior.

Issue canonical#4257 Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

tomponline · 2023-01-23T08:49:09Z

@mihalicyn happy to close this for now?

mihalicyn · 2023-01-23T09:22:00Z

@mihalicyn happy to close this for now?

why close? We don't want to take it?

tomponline · 2023-01-23T09:23:15Z

Oh right, you never requested a subsequent review on it and the RFC subject suggested this was just a design proposal.

So does liblxc support this now?

tomponline · 2023-01-23T09:23:39Z

static analysis tests are failing, suggesting not.

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
       ^
Error: lxd/instance/drivers/driver_lxc.go:636:11: cc.SetTimeout undefined (type *lxc.Container has no field or method SetTimeout) (typecheck)
	err = cc.SetTimeout(time.Duration(10) * time.Second)
	         ^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

tomponline · 2023-01-23T09:24:49Z

And jenkins is red across the board with compile errors:

https://jenkins.linuxcontainers.org/job/lxd-github-pull-test/8656/

lxd/instance/drivers/driver_lxc.go:636:11: cc.SetTimeout undefined (type *lxc.Container has no field or method SetTimeout)

tomponline · 2023-01-23T09:25:42Z

I thought you would open a working PR once the changes had been made to liblxc itself.

mihalicyn · 2023-01-23T09:27:54Z

I've posted PRs to all projects https://github.com/lxc/lxd/pull/11293#issuecomment-1385870390

tomponline · 2023-01-23T09:29:10Z

OK I missed that. The "RFC" in the title has really confused me as thought it was just a proposal and not ready yet.
If its ready for review lets drop the "RFC" bit.

mihalicyn · 2023-01-23T09:29:35Z

Oh right, you never requested a subsequent review on it and the RFC subject suggested this was just a design proposal.

That's it. But I've started from implementing that in liblxc, go-lxc, lxd. It's easier to discuss when everything is almost ready :)

So does liblxc support this now?

lxc/lxc#4260

tomponline · 2023-01-23T09:29:55Z

This helps with the weekly news letter too as the PR titles show up in there.

tomponline · 2023-01-23T09:31:47Z

We may want to use liblxc.HasAPIExtension(extension) to detect if set timeout is supported or not. Let's decide.

On this point, what happens in go-lxc if we call the timeout and the underlying liblxc doesn't support the extension?
If nothing happens, then I don't see a need to use HasAPIExtension and we can just always set the timeout and then it only gets applied if liblxc supports it.

What do you think?

Or are you thinking that an error should be returned if liblxc doesn't support it and thus we should check HasAPIExtension before calling it?

tomponline · 2023-01-23T09:32:17Z

lxd/instance/drivers/driver_lxc.go

@@ -632,6 +632,12 @@ func (d *lxc) initLXC(config bool) error {
 		return err
 	}

+	// To discuss: take defalt timeout value from some global config?


Lets take this comment out now the PR is ready for review.

mihalicyn · 2023-01-23T10:00:33Z

On this point, what happens in go-lxc if we call the timeout and the underlying liblxc doesn't support the extension?

Right now, SetTimeout from go-lxc should return an error.

Or are you thinking that an error should be returned if liblxc doesn't support it and thus we should check HasAPIExtension before calling it?

Yep, we can just use error code to detect that operation is not supported EOPNOTSUPP or we can use HasAPIExtension. It's a matter of taste.

tomponline · 2023-01-23T10:06:02Z

I see. I'm happy with either, but detecting the error seems simpler. Thanks

stgraber · 2023-01-25T04:42:16Z

Moving to Draft until the liblxc and go-lxc side of this is finalized.

tomponline · 2023-02-16T11:55:42Z

@mihalicyn happy to close this until its ready?

mihalicyn mentioned this pull request Jan 17, 2023

LXC API extension: set_timeout lxc/lxc#4260

Merged

mihalicyn requested a review from tomponline January 17, 2023 18:50

tomponline reviewed Jan 18, 2023

View reviewed changes

driver_lxc: set receive timeout for LXC command client sock

5846137

Issue canonical#4257 Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

mihalicyn force-pushed the lxc_set_timeout branch from 4f07113 to 5846137 Compare January 18, 2023 09:17

tomponline changed the title ~~[RFC] driver_lxc: set receive timeout for LXC command client sock~~ Instance: Set receive timeout for LXC command client sock Jan 23, 2023

tomponline reviewed Jan 23, 2023

View reviewed changes

stgraber marked this pull request as draft January 25, 2023 04:41

mihalicyn closed this Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instance: Set receive timeout for LXC command client sock #11293

Instance: Set receive timeout for LXC command client sock #11293

mihalicyn commented Jan 17, 2023 •

edited

mihalicyn commented Jan 17, 2023 •

edited

tomponline Jan 18, 2023

tomponline Jan 18, 2023

mihalicyn Jan 18, 2023

mihalicyn Jan 18, 2023 •

edited

tomponline Jan 18, 2023

mihalicyn Jan 18, 2023

tomponline Jan 18, 2023

mihalicyn Jan 18, 2023

tomponline Jan 18, 2023

mihalicyn Jan 18, 2023

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023 •

edited

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023 •

edited

tomponline Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

stgraber commented Jan 25, 2023

tomponline commented Feb 16, 2023

Instance: Set receive timeout for LXC command client sock #11293

Instance: Set receive timeout for LXC command client sock #11293

Conversation

mihalicyn commented Jan 17, 2023 • edited

mihalicyn commented Jan 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalicyn Jan 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023 • edited

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

tomponline commented Jan 23, 2023 • edited

Choose a reason for hiding this comment

mihalicyn commented Jan 23, 2023

tomponline commented Jan 23, 2023

stgraber commented Jan 25, 2023

tomponline commented Feb 16, 2023

mihalicyn commented Jan 17, 2023 •

edited

mihalicyn commented Jan 17, 2023 •

edited

mihalicyn Jan 18, 2023 •

edited

tomponline commented Jan 23, 2023 •

edited

tomponline commented Jan 23, 2023 •

edited