search

virtual-kubelet / azure-aci

Things related to Azure Container Instances for Virtual Kubelet
Apache License 2.0
92 stars 71 forks source link

Mixup between command and args in Pod resource description #10

Open victornoel opened 5 years ago

victornoel commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Makes ense. Thanks!