Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle an already connected network in libpod API #15516

Merged

Conversation

kubealex
Copy link
Contributor

@kubealex kubealex commented Aug 27, 2022

Signed-off-by: Alessandro Rossi [email protected]
The PR introduces a new "ErrNetworkConnected" error in libpod defines, to handle the specific case where a user tries to connect a container to a network that is already connected to it.

The user from the CLI will still receive an error, but since from API point of view the operation results in nothing to be done and the state is preserved, it should return 200, aligned with the behavior that docker has.

This should address #15499

Does this PR introduce a user-facing change?

None

ErrNetworkConnected was introduced for when trying to connect a container to a network it is already connected to

@kubealex kubealex force-pushed the handle-connected-network branch from 1e7c425 to 3756c68 Compare August 27, 2022 16:57
@baude
Copy link
Member

baude commented Aug 27, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, kubealex

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2022
libpod/define/errors.go Outdated Show resolved Hide resolved
@baude
Copy link
Member

baude commented Aug 27, 2022

LGTM, I missed with your commit and release note message a little. Thanks for the PR

@kubealex kubealex force-pushed the handle-connected-network branch 7 times, most recently from e1ac31c to ca214f2 Compare August 27, 2022 18:52
@kubealex
Copy link
Contributor Author

@baude seems like there are issues with test image pull 😔

@n1hility
Copy link
Member

@kubealex @baude IMO we should cover the CLI case as well, since a goal for podman is alias docker=podman. One other factor when it comes to compat, is that docker still errors on this condition once the container hits started state.

Something like this: n1hility@53d45ae

Sorry for the collision: I had self-assigned this issue, had time to work on it and then when I went to open a PR saw this referenced on the issue. Feel free to pull this in and rework.

-Jason

@kubealex
Copy link
Contributor Author

@n1hility docker CLI preserves the error, hence my idea of creating an error that could fit better the use case.
For docker, CLI reports a strict error, stating that the container already is an endpoint for the network, while API reports 200 :)

@n1hility
Copy link
Member

n1hility commented Aug 27, 2022

@n1hility docker CLI preserves the error, hence my idea of creating an error that could fit better the use case.
For docker, CLI reports a strict error, stating that the container already is an endpoint for the network, while API reports 200 :)

Right This only happens when started. Ex:


$ docker create alpine sleep 86400
408f8751d16a5566c506e17182fa26643513762eae2d26bf0d3e88b8ff722c0a
$ docker network connect bridge 408f8751d16a5566c506e17182fa26643513762eae2d26bf0d3e88b8ff722c0a
$ echo $?
0
$ docker start 408f8751d16a5566c506e17182fa26643513762eae2d26bf0d3e88b8ff722c0a
408f8751d16a5566c506e17182fa26643513762eae2d26bf0d3e88b8ff722c0a
$ docker network connect bridge 408f8751d16a5566c506e17182fa26643513762eae2d26bf0d3e88b8ff722c0a
Error response from daemon: endpoint with name romantic_pike already exists in network bridge

(same on the REST interface)

@kubealex
Copy link
Contributor Author

Right, I'll focus on that tomorrow!
Thx a lot!

@kubealex kubealex force-pushed the handle-connected-network branch 8 times, most recently from 6409445 to 5a97f5d Compare August 27, 2022 22:13
@kubealex
Copy link
Contributor Author

kubealex commented Aug 27, 2022

@n1hility I just merged your effort in the commit, not sure if I did it correctly, feel free to rework it if it isn't compliant, also reach out in GChat to ease the conversation :)

@n1hility n1hility force-pushed the handle-connected-network branch from 5a97f5d to 77808cd Compare August 27, 2022 23:45
@n1hility
Copy link
Member

@kubealex cool thanks! I just made one small change with the leftover few lines on the rest endpoint, since the error is filtered early. This also allows the rest endpoint to reflect the start error conditions.

Compat: Treat already attached networks as a no-op
Applies only to containers in created state. Maintain error in running state.

Co-authored-by: Alessandro Rossi <[email protected]>
Co-authored-by: Brent Baude <[email protected]>
Co-authored-by: Jason T. Greene <[email protected]>
Signed-off-by: Alessandro Rossi <[email protected]>
Signed-off-by: Jason T. Greene <[email protected]>
@n1hility n1hility force-pushed the handle-connected-network branch from 77808cd to 78aec21 Compare August 28, 2022 01:04
@kubealex
Copy link
Contributor Author

Nice @n1hility 💪 thanks a lot for the integration!

@rhatdan
Copy link
Member

rhatdan commented Aug 29, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2022
@openshift-merge-robot openshift-merge-robot merged commit e78363d into containers:main Aug 29, 2022
@kubealex kubealex deleted the handle-connected-network branch August 29, 2022 12:21
@n1hility
Copy link
Member

@kubealex you are most welcome @kubealex. It was great collaborating with you, thank you for digging in and submitting a PR!

@n1hility
Copy link
Member

/cherry-pick v4.2

@openshift-cherrypick-robot
Copy link
Collaborator

@n1hility: new pull request created: #15554

In response to this:

/cherry-pick v4.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants