runfinch / finch

The Finch CLI is an open source client for container development
https://www.runfinch.com
Apache License 2.0
3.48k stars 87 forks source link

fix: add finch attach command #847

Closed haytok closed 3 months ago

haytok commented 3 months ago

The finch attach command cannot be executed at the top level. Therefore, when the finch attach -h command is executed, the following error occurs.

  > finch attach -h
  FATA[0000] unknown command "attach" for "finch"

Also, this bug has been reported in issue/747 below, and it is hoped that finch attach will be executed.

Meanwhile, with the nerdctl command, it is possible to execute the nerdctl attach command.

Therefore, in this commit, we will make corrections so that the finch attach command can be executed.

Issue #, if available: #747

Description of changes: Details are described in commit message.

Testing done: No

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Shubhranshu153 commented 3 months ago

Thank you for the commit. Can we add an e2e test for this. There is a separate package called common-test, where we can add this test. We have some examples in the package https://github.com/runfinch/common-tests

For more info: https://github.com/runfinch/finch/blob/main/CONTRIBUTING.md

haytok commented 3 months ago

@Shubhranshu153

Thanks for comments.

I have investigated the following, but am having trouble implementing the test for the finch attach command because we cannot use the -it option within ginkgo. Could you please advise on how to implement the test?

What I have investigated

First, when testing locally, we can check the operation as follows

haytok finch [add-attach-command]
> _output/bin/finch run --rm --name test -it alpine sh
/ #
/ #
/ # lbin    dev    etc    home   lib    media  mnt    opt    proc   root   run    sbin   srv    sys    tmp    usr    var
/ #

Other session

haytok finch [add-attach-command]
> _output/bin/finch attach test

/ #
/ # s

However, as you can see in the following content, it is difficult to implement a test that executes finch attach against a container launched interactively in the test.

// TODO: specifying -t flag will have error in test -> panic: provided file is not a console

So, for example, suppose that the container is launched as follows. (ref)

            ginkgo.BeforeEach(func() {
                command.Run(o, "run", "-d", "--name", testContainerName, defaultImage, "sleep", "infinity")
            })

Since the shell process is not running in the container launched with sleep infinity, the test for finch attach cannot be executed.

haytok finch [add-attach-command]
> _output/bin/finch run -d --name test alpine sleep infinity
0b4d9dc034ddb95335fe9685c48b7bdfaeec8d2cdfc5fe7be46fc369ddac682d
haytok finch [add-attach-command]
> _output/bin/finch ps
CONTAINER ID    IMAGE                              COMMAND             CREATED          STATUS    PORTS    NAMES
0b4d9dc034dd    docker.io/library/alpine:latest    "sleep infinity"    4 seconds ago    Up                 test
haytok finch [add-attach-command]
> _output/bin/finch attach test
FATA[0000] failed to attach to the container: failed to open stdout fifo: error creating fifo binary:///usr/local/bin/nerdctl?_NERDCTL_INTERNAL_LOGGING=%2Fvar%2Flib%2Fnerdctl%2F1935db59: no such file or directory
FATA[0000] exit status 1
Shubhranshu153 commented 3 months ago

for your test you might be able to use

finch run -d ubuntu bash -c "while true; do echo /'Hello, world/'; sleep 1; done"

to lunch a container that is prininting hellow world per second. Then you can able to attach the container and test if it is in stdout. there are some examples for that in Events.go https://github.com/runfinch/common-tests/blob/146e16fe020f6ef94139d473b78103e374180647/tests/events.go#L4

haytok commented 3 months ago

@Shubhranshu153

The verification was carried out by starting the container you advised on. However, due to a bug on the nerdctl side, it was not possible to attach it to the container with finch attach.

More details were described in the nerdctl discussion below I asked.

Therefore, it seems difficult to implement tests for finch attach at the moment. So, should we put this PR pending for now?

Shubhranshu153 commented 3 months ago

thanks for the investigation. wondering if its issue with rootless only containers as your issue suggest in nerdctl?

haytok commented 3 months ago

The issue occurs in a rootless container created with nerdctl and containerd.

The error message indicates that the nerdctl attach process cannot access the directory /var/lib/nerdctl/, which is why this error occurs.

[haytok@lima-finch nerdctl]$ ls /var/lib/nerdctl/1935db59/
containers etchosts names volumes

However, I have not been able to verify that the error does not occur when operating from a non-rootless container.

Shubhranshu153 commented 3 months ago

it occurs in sudo mode also, left comments on nerdctl channel.