Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixup between command and args in Pod resource description #10

Open
victornoel opened this issue Jun 24, 2019 · 10 comments
Open

Mixup between command and args in Pod resource description #10

victornoel opened this issue Jun 24, 2019 · 10 comments
Labels
kind/bug Something isn't working kind/enhancement New feature or request

Comments

@victornoel
Copy link

On an AKS cluster with virtual nodes enable, I created a Pod resource to be deployed on ACI, but it seems that if I specify args for it, ACI uses them as the whole command instead of appending to the entrypoint of the docker image (for the record, docker's entrypoint is kubernetes' command, and docker's command is kubernetes' args).

If I use a command, it works as expected.

Also when I used args and the launching the ACI instance failed, there was no error at all except for some logs saying:

failed to open log file "/var/log/pods/0e49cbc3-943d-11e9-a2bc-000d3a28fe89/mypod_0.log": open /var/log/pods/0e49cbc3-943d-11e9-a2bc-000d3a28fe89/mypod_0.log: no such file or directory

There is maybe something to improve in ACI error reporting too.

For the record here is the version of the virtual node: v1.13.1-vk-v0.9.0-1-g7b92d1ee-dev

@grahamhayes
Copy link
Contributor

This looks like a side effect of the ACI API - https://docs.microsoft.com/en-us/rest/api/container-instances/containergroups/createorupdate#container

It only has a "command" field, which overrides the entrypoint if set, so combined with https://github.com/virtual-kubelet/azure-aci/blob/master/aci.go#L1163 if the command is not set, this just passes the list of args, with no prefixed commands.

Not sure what the fix is here - ACI adding a "args" field, or virtual-kubelet inspecting the docker container and extracting the entrypoint, and pre-filling in the command on it's side before passing the full command to ACI.

@kiall
Copy link

kiall commented Jul 1, 2019

Another alternative, which would still be an API breakage, would be to reject pods that contains args without command. However, this would be preferable to just executing the args without the command but obviously not as preferable as maintaining the K8S API compatibility.

@victornoel
Copy link
Author

@grahamhayes @kiall it would be best if compatibility with kubernetes normal behaviour is kept, it's going to be hell to maintain deployments as a op if not.
I don't believe it should be too hard to do exactly as @grahamhayes proposes:

virtual-kubelet inspecting the docker container and extracting the entrypoint, and pre-filling in the command on it's side before passing the full command to ACI.

@cpuguy83
Copy link
Contributor

I do not think VK or even the ACI lib here should be inspecting the image like this.
I'm not sure how ACI's "Command" winds up as a container command, we need someone from ACI to dig into this.

ping @srrengar

@grahamhayes
Copy link
Contributor

I think @kiall 's suggestion of blocking the creation of pods with args + no command is probably a good short term solution, and when ACI updates its API (prefered), or decide not to, and the Azure VK plugin implements some sort of inspection (definitely not prefered), it can be unblocked.

grahamhayes added a commit to grahamhayes/azure-aci that referenced this issue Jul 12, 2019
As ACI currently combined args + command, a pod
with args and no command will have un-expected behaviour.

This blocks the un-expected behavior.

Related: virtual-kubelet#10

Signed-off-by: Graham Hayes <[email protected]>
@cpuguy83
Copy link
Contributor

I'm wondering if it's worth blocking if things are obviously failing.
If we do that and ACI fixes their end we'll have to update here to not error out.

@cpuguy83
Copy link
Contributor

So, is this failing safely already? If so then it seems ok to leave it, if it is not failing safely (or is not obvious that it's failed), then I agree we should patch.

@victornoel
Copy link
Author

@cpuguy83 from my user POV, it's not failing safely because I have no idea why this happened, there was no log nor clear error, and found a solution just by luck :)

@kiall
Copy link

kiall commented Jul 17, 2019

Right, the translation between the Pod manifest and what's actually run is incorrect, in certain circumstances a valid Pod spec will end up executing an unexpected command - the only insight you'll have into this is (hopefully!) a "no such file or directory" log inside the containers logs. In certain circumstances, it's possible that a valid but unexpected command will be found and executed.

To make up a contrived example, if my container image has a default command of echo, and I provide in the Pod manifest args of ["rm", "-rf", "/"], without specifying command, I expect the full command executed to be echo rm -rf /.

However, what will actually be executed is just rm -rf /. Again, contrived example - but it highlights that it's not failing safely (unexpected commands are ran), and not failing in a consistent manner (e.g. it's not always going to fail to run something and log an error).

I believe this stems from K8S having both a "command" and "args" PodSpec property, and ACI just having a combined command and args property. For a correct mapping to ACI, either we need to infer the unspecified PodSpec fields (i.e. inspect the container image) or reject the pod with a clear and consistent error when one, but not both, PodSpec fields are provided.

@grahamhayes has PR #11 which takes this second route.

From a end user POV, the first route would obviously be nicer - but the implications of VK having to download the image and inspect it's contents are likely not worth it, so the second route seems best to me.

Edit: corrected my last sentence, I accidentally said first in place of second.

grahamhayes added a commit to grahamhayes/azure-aci that referenced this issue Jul 17, 2019
As ACI currently combined args + command, a pod
with args and no command will have un-expected behaviour.

This blocks the un-expected behavior.

Related: virtual-kubelet#10

Signed-off-by: Graham Hayes <[email protected]>
@cpuguy83
Copy link
Contributor

Makes ense. Thanks!

grahamhayes added a commit to grahamhayes/azure-aci that referenced this issue Jul 22, 2019
As ACI currently combined args + command, a pod
with args and no command will have un-expected behaviour.

This blocks the un-expected behavior.

Related: virtual-kubelet#10

Signed-off-by: Graham Hayes <[email protected]>
grahamhayes added a commit to grahamhayes/azure-aci that referenced this issue Aug 21, 2019
As ACI currently combined args + command, a pod
with args and no command will have un-expected behaviour.

This blocks the un-expected behavior.

Related: virtual-kubelet#10

Signed-off-by: Graham Hayes <[email protected]>
@karenhchu karenhchu added kind/bug Something isn't working kind/enhancement New feature or request labels Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants