remote run: fix error checks

[vrothberg]

As error types are not preserved on the client side (due to marshaling),
we cannot use errors.Cause(...) and friends but, unfortunately, have
to fall back to looking for substring the error messages.

Change the error checks in remote run to do substring matches and fix
issue #7340.

Fixes: #7340
Signed-off-by: Valentin Rothberg rothberg@redhat.com

[rhatdan]

LGTM other the tini nit.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,vrothberg]

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

[QiWang19]

LGTM

[rhatdan]

/lgtm
/hold

[vrothberg]

@edsantiago PTAL

[edsantiago]

@vrothberg did you try the reproducer in #7340? I'm still seeing it:

$ while :;do got=$(./bin/podman-remote run --rm -v testvol1:/etc/apk alpine_labels ls /etc/apk); test "$got" = "$expect" || echo "got: $got";done
Error: no container with name or ID 447c18f8966f912c98e113257a980351c94e47c0d4c5692cd83efc011058b6ef found: no such container
Error: no container with name or ID be5a8aa6cc0be4c6f5d7add69e748498ee29fd2c465e399a14ac678ed8d4a40f found: no such container
Error: no container with name or ID 354abc85b5046fcb82bed15099f4c0cfd9cf4a7004dd57cb787daf0dee75bb7e found: no such container

[vrothberg]

@vrothberg did you try the reproducer in #7340? I'm still seeing it:

I ran it before for 20 minutes and didn't catch it anymore. I will recheck, maybe the last change screwed things up again. Thanks for checking.

/hold

[vrothberg]

/hold

[TomSweeneyRedHat]

and some possible test issues.

[vrothberg]

Looks like there's more unpack. Both, tunnel and abi, are doing (if opts.Rm):

if err := ic.Libpod.RemoveContainer(ctx, ctr, false, true); err != nil {       
        if errors.Cause(err) == define.ErrNoSuchCtr ||                         
                errors.Cause(err) == define.ErrCtrRemoved {                    
                logrus.Errorf("Container %s does not exist: %v", ctr.ID(), err)
        } else {                                                               
                logrus.Errorf("Error removing container %s: %v", ctr.ID(), err)
        }                                                                      
}                                                                              

Both are suspect to a race with the podman container cleanup --rm process. However, the tunnel parts run into the additional issue that we're not preserving the error types (we're losing them over the wire).

@mheon, do you know if the seemingly redundant removal in ContainerRun serves a purpose I may not be aware of? Seeing the CI failures, there might be something going on with volumes. Are (libpod).RemoveContainer() and container cleanup --rm identical?

[mheon]

@vrothberg Basically, we have to be sure that the container is completely removed when the podman run process exits to prevent races - but we also need the cleanup process, because the podman run process can be killed, or the user can manually detach causing it to exit. So we need both. As you surmise, RemoveContainer() and cleanup --rm are the same thing - we're just invoking in duplicate to be certain the container is removed.

Generally speaking, the increasing priority of terminal-interactive tasks means that in the local case podman run will run before the cleanup process, so it will be able to call RemoveContainer() first. The cleanup process will fail, but it's not really logging its results anywhere, so that's perfectly fine. I imagine remote makes this considerably more complicated...

[vrothberg]

Thanks, @mheon! Once #7574 is merged, I rebase and rework this PR.

[vrothberg]

On top of the latest changes, it was sufficient to just fix the error checking.

@mheon @rhatdan PTanotherL

[rhatdan]

/lgtm

[vrothberg]

/hold cancel