Mixup between command and args in Pod resource description

[victornoel]

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]

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]

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]

@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]

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]

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.

[cpuguy83]

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]

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]

@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]

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.

[cpuguy83]

Makes ense. Thanks!