Add exec_run implementation for containers

[JacobCallahan]

This change adds a basic implementation of exec_run for containers
Environment variable conversions are handled from dict->list[str]
DetachKeys will need an actual argument option
detach, stream, socket, and demux arguments are currently not handled.

[JacobCallahan]

@jwhonce this is the version that is currently seeing the 500 ISE when used in my project.

[jwhonce]

@JacobCallahan Please review https://docs.podman.io/en/latest/_static/api.html#operation/ContainerExecLibpod, name is the only query parameter you may pass, all the other fields need to be passed as data.

[JacobCallahan]

addressed in current update

[JacobCallahan]

@jwhonce thanks again for the sage advice!
Now there is a question of what to return from this method.
right now it is a dict of the exec instance info, but I think an actual object (preferably one that you can pull stdout/err/status) from would be best.

If the object route is the way to go, how would that be done?

[rhatdan]

@jwhonce @JacobCallahan what is going on with this PR?

[JacobCallahan]

@jwhonce @JacobCallahan what is going on with this PR?

Unfortunately, I've been dealing with much higher priorities since the last update. Hopefully I'll be able to revisit this soon.

One thing that would help with this would be if someone could point out an efficient way to get stdout/stderr from an exec instance container

[rhatdan]

@JacobCallahan @jwhonce What should we do with this PR?

[JacobCallahan]

@rhatdan sorry, we have been focusing on other priorities. We're finishing planning for this year and if all goes well, I can revisit this early next month.

I believe I left off on how to construct a container object from an exec instance id

[JacobCallahan]

@rhatdan @jwhonce I've revisited this today and have it to the point where it returns the original container object (not the exec container since that seems to be gone by the time the manager can get the object). I don't think this is a very useful return. An output-like object would be better, but I'm not sure how to create that. Any pointers would be appreciated.

[JacobCallahan]

Alright, In the latest version, I have exec_run working with basic commands. However, since it uses entrypoint, it complicates bash-like commands being passed in. In the simple example below, the command executes correctly, the file is created, and we get the correct return code. However, we currently are unable to get any additional relevant information or output from commands.

In [4]: ubi8.exec_run("touch test.txt")
{'CanRemove': True, 'ContainerID': 'a260ee5258383680de1189f603f3218a9609ddb664d68d390b5dad58481932af', 'DetachKeys': '', 'ExitCode': 0, 'ID': 'fd37335600af2af4c4f717723eda0fc80ba5767613c1b22b5e3480972179cfab', 'OpenStderr': True, 'OpenStdin': False, 'OpenStdout': True, 'Running': False, 'Pid': 0, 'ProcessConfig': {'arguments': ['test.txt'], 'entrypoint': 'touch', 'privileged': False, 'tty': True, 'user': 'root'}}
Out[4]: 0

[msisj]

@JacobCallahan
Actually you have almost working solution. It just need a quick tweaks :

1. split() is a wrong method to use here. As this is exec is using (working similar) as the entrypoint, to allow more complex command you should use shlex.split() . Then the complex commands works (one of those is shown in example below)
2. rename response to something else to not lose it e.g. start_respose
3. return tuple start_response.text, response.json().get('ExitCode')
4. In example you'll see some ascii signs returned by Podman REST API. Either erase them in REST API or here to prevent further issues
5. Update method's documentation's Return section

So it turns your the exec start is blocking whole REST and the end in text we have the output of command. I've tried it with longer commands (sleep for 90seconds and then print whatever and it worked)

Example :

>>> container
<Container: 65246b3e7b>
>>> output, ec = container.exec_run('bash -c "sleep 15; cat /etc/resolv.conf"')
>>> output
'\x01\x00\x00\x00\x00\x00\x00+search dns.podman\nnameserver 192.168.194.1\n'
>>> ec
0
>>> output, ec = container.exec_run('wrong')
>>> output
'\x02\x00\x00\x00\x00\x00\x00¿Error: exec failed: container_linux.go:380: starting container process caused: exec: "wrong": executable file not found in $PATH: OCI runtime attempted to invoke a command that was not found\n'
>>> ec
127

Let me know what you think about it

[JacobCallahan]

@msisj Thanks for the fantastic testing! I'm pretty swamped at the moment, but I will definitely check out and try your suggestions. Also, do you think it would be more useful to return the output and exit status in a single object or separate how it is now? I additionally need to consider demuxing the streams.

[msisj]

@JacobCallahan
Normally I'd say to stay as similar to docker-py (when it comes to the interface) as possible, but as I can see in docker-py there were only exec_[create|inspect|resize|start] methods which are available in podman REST API. There was no collective exec_run which did all of these functionalities at once. So that's a no go.

Returning an object is generally a good idea I suppose, but I'd try to ask any of the owners of this product what interface they want to have honestly.

If it comes to demuxing streams I'm wondering why do you need to consider this, is there a planned rework for Podman REST API in this area?
As for current, fairly new version (4.0.0), I can see there is no way of getting them separate (honestly I'm not even sure if the stderr is shown now, haven't tested it).

To be even more honest, documentation is rather lacking as the exec start basically does not say that it returns any output.
What is worth noting is that to successfully and reliably get an output from start is to use detach=False as the documentation says.

[msisj]

@JacobCallahan Are you planning to work on this draft? If you don't, I can take some of this code, tweak it and create PR for it (basically I need this feature on master).

[samarAvi]

@JacobCallahan @rhatdan Do we have any ETA on this feature? This feature is absolutely critical for our use-case.

[JacobCallahan]

@msisj @samarAvi I'm still busy with other things, but I will try to revisit this today.

[samarAvi]

@JacobCallahan Hope you got the chance to revisit this. Thanks in advance. Please share if you have any updates.

[JacobCallahan]

@msisj i had a bit of time to play around with this a bit more today and try out your suggestions. However, I'm still not actually seeing the command output you're seeing. I am only getting back the Id for the exec_start instance.
Is there perhaps some other local change you made to include that command output?

In [4]: res = ch.exec_run('bash -c "sleep 15; cat /etc/resolv.conf"')

In [5]: res
Out[5]: 
('{"Id":"7d0d0a9219f6f932b7a85bfda7b64b3b011e33623f67e116401c9f47169ac446"}\n',
 0)

for reference, the meat of my local version of exec_run

# create the exec instance
start_resp = self.client.post(f"/containers/{self.name}/exec", data=json.dumps(data))
start_resp.raise_for_status()
exec_id = start_resp.json()['Id']
# start the exec instance
response = self.client.post(f"/exec/{exec_id}/start", data=json.dumps(
    {"Detach": detach, "Tty": tty}
))
response.raise_for_status()
# get and return exec information
response = self.client.get(f"/exec/{exec_id}/json")
response.raise_for_status()
return start_resp.text, response.json().get('ExitCode')

[msisj]

@JacobCallahan
You are super close.
I think you've overlooked what I've written. In point 2, when you click it I've linked which line should have the start_response.
As the name states, this is response of start call rather than exec, that's weird but the output is saved to /start endpoint by podman REST API.
To be more precise :

start_resp = self.client.post(f"/containers/{self.name}/exec", data=json.dumps(data))
start_resp.raise_for_status()

->

response = self.client.post(f"/containers/{self.name}/exec", data=json.dumps(data))
response .raise_for_status()

and

response = self.client.post(f"/exec/{exec_id}/start", data=json.dumps(
    {"Detach": detach, "Tty": tty}
))
response.raise_for_status()

->

start_resp = self.client.post(f"/exec/{exec_id}/start", data=json.dumps(
    {"Detach": detach, "Tty": tty}
))
start_resp.raise_for_status()

[samarAvi]

Thanks @msisj @JacobCallahan, That helped.

In [29]: r = cont.exec_run("ls", privileged = True, stdin = True, tty = True,stream= True)
In [31]: r[1].text
Out[31]: 'bin   core  etc   lib\t lib64\t media\topt   root  sbin  sys  usr\r\nboot  dev   home  lib32  libx32  mnt\tproc  run   srv   tmp  var\r\n'

But I'm having issues with interactive-tty . Below code gets stuck since it's waiting for stdin.

In [32]: r = cont.exec_run("ssh aviuser@10.102.66.74", privileged = True, stdin = True, tty = True, stream= True)

Process is getting started inside the container.

root@01a1aad29664:/# ps ax | grep ssh
     93 pts/4    Ss+    0:00 ssh aviuser@10.102.66.74
    102 pts/2    S+     0:00 grep --color=auto ssh
root@01a1aad29664:/#

Any hints?

[JacobCallahan]

@msisj ahh yep I misunderstood what you meant, thanks for clarifying! The new version is up now and working fine for me.

One thing we may want to consider is some kind of parity with Docker's implementation where the response is packaged into a response object with output and exit_code attributes at a minimum. This potentially could be something as simple as a named tuple or another custom object.

@samarAvi What you would need is an implementation for attach/attach_socket. With that, you could send and read at will.

[samarAvi]

Thanks for amazing work so far @JacobCallahan. IMHO attach/attach_socket also needs to be implemented as part of this PR then, could you please take it up. @msisj what do you think?

[JacobCallahan]

@jwhonce thanks. i've switched the return order so the return code is leading. However, I'm getting normal strings back from the API without any additional processing. In order to return bytes, I'd have to explicitly convert it (specifying an encode type like utf-8). If this is something you want, I can certainly add it.

In [4]: res = ch.exec_run('hostname')

In [5]: res
Out[5]: (0, '1b122d3312b8\r\n')

In [6]: type(res[1])
Out[6]: str

[enhaut]

@JacobCallahan
APIClient.post() returns Response object, so instead of using property .text, try to use .content - https://requests.readthedocs.io/en/latest/user/quickstart/#binary-response-content in your exec_run implementation

[JacobCallahan]

@enhaut I believe the intention was to make the return similar to docker-py's. @jwhonce please feel free to weigh in

[jwhonce]

@JacobCallahan For compatibility, I agree with @enhaut you should change to .content from .text. I understand we're pushing the burden to convert to str on most developers, but should the output be binary those will need this type. From reading the documentation and walking code, I think this is it!

[JacobCallahan]

@jwhonce @enhaut i've updated the PR to now return the content as requested. Hope this helps!

[enhaut]

@JacobCallahan you need to remove the draft status

[enhaut]

Whats going on with this PR? @rhatdan

[rhatdan]

@enhaut Isn't failing a gating test? @JacobCallahan Could you fix this to pass the test?

[JacobCallahan]

@rhatdan looks like all automatic checks are passing now

[rhatdan]

/approve
@mwhahaha @jwhonce PTAL

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JacobCallahan, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jwhonce]

LGTM

[rhatdan]

/lgtm