Fix execute check #96
Conversation
container.go
Outdated
| @@ -495,7 +495,7 @@ func (c *Container) Execute(args ...string) ([]byte, error) { | |||
| c.mu.Lock() | |||
| defer c.mu.Unlock() | |||
|
|
|||
| if err := c.makeSure(isNotDefined); err != nil { | |||
| if err := c.makeSure(isDefined); err != nil { | |||
There was a problem hiding this comment.
So I think the "isNotDefined" was actually intentional here. I ran into this earlier when fixing a bunch of issues with the go-lxc tests. Execute, per its description, was meant to run a command inside a temporary, throw-away container, not using a defined LXC container.
@caglar10ur may have more background on why that was done this way.
There was a problem hiding this comment.
I agree that it'd have been better if Execute() behaved like lxc-execute, unfortunately that's not how it's been implemented and I'm worried that reversing that logic now will break users.
There was a problem hiding this comment.
I think - as @stgraber said this was meant to run a command inside a temporary, throw-away container, not using a defined LXC container.
How about adding ExecuteInContainer for executing something inside an already defined container?
There was a problem hiding this comment.
That would be the same as StartExecute or whatever it's called that @brauner added though right?
There was a problem hiding this comment.
Ah yep, just saw that you merged it :)
There was a problem hiding this comment.
That's why I added the code right below this check which on 2.1+ will create and then remove a temporary config, I believe this did fix the tests on recent liblxc at least.
There was a problem hiding this comment.
Maybe we should error out on <= 2.1 then? Because without that, it won't work.
There was a problem hiding this comment.
I'm confused, I only started seeing this failing with 2.1+ so thought it was a behavior change and that 2.0 and earlier didn't need this particular change.
If 2.0 also needs this now, then we should just change the version check to cover it too.
There was a problem hiding this comment.
The api call c->start(do_execute()) would always fail and did always fail if the container is not defined. The lxc-execute binary just provided a dummy and actually quite insecure config via lxc_conf_init() which is an internal function not exposed via the API. This hack was removed in preparation for splitting out the tools from the shared library. It seems that this was backported to stable-2.0 for the lxc-2.0.9 release. This might have happened on accident.
There was a problem hiding this comment.
Ok, so we should change go-lxc to just always create the temp config. It's a hack but to be fair, this entire function is a hack so whatever :)
With the immanent arrival of StartExecute(), Execute() is probably going to be enshrined in hackery-whackery for the rest of time. So let's get rid of this commented out code. Signed-off-by: Tycho Andersen <tycho@tycho.ws>
We always need this. So let's create it. Signed-off-by: Tycho Andersen <tycho@tycho.ws>
tych0
force-pushed
the
fix-execute-check
branch
from
67d98df
to
6fbff88
Compare
|
Ok, I fixed it up accordingly. Thanks! |
Moving away from internal symbols we need can't do hacks like we currently do in lxc-start and call internal functions like lxc_conf_init(). This is unsafe anyway. Instead, we should simply error out if the user didn't give us a configuration file to use. lxc-start refuses to start in that case already. Relates to discussion in lxc/go-lxc#96 (comment) . Closes lxc#2023. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Moving away from internal symbols we can't do hacks like we currently do in lxc-start and call internal functions like lxc_conf_init(). This is unsafe anyway. Instead, we should simply error out if the user didn't give us a configuration file to use. lxc-start refuses to start in that case already. Relates to discussion in lxc/go-lxc#96 (comment) . Closes lxc#2023. Reported-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Moving away from internal symbols we can't do hacks like we currently do in lxc-start and call internal functions like lxc_conf_init(). This is unsafe anyway. Instead, we should simply error out if the user didn't give us a configuration file to use. lxc-start refuses to start in that case already. Relates to discussion in lxc/go-lxc#96 (comment) . Closes #2023. Reported-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Moving away from internal symbols we can't do hacks like we currently do in lxc-start and call internal functions like lxc_conf_init(). This is unsafe anyway. Instead, we should simply error out if the user didn't give us a configuration file to use. lxc-start refuses to start in that case already. Relates to discussion in lxc/go-lxc#96 (comment) . Closes #2023. Reported-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Moving away from internal symbols we can't do hacks like we currently do in lxc-start and call internal functions like lxc_conf_init(). This is unsafe anyway. Instead, we should simply error out if the user didn't give us a configuration file to use. lxc-start refuses to start in that case already. Relates to discussion in lxc/go-lxc#96 (comment) . Closes lxc#2023. Reported-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
see patch notes for details