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

Add pods created by play kube to a default network #16029

Merged

Conversation

ancosma
Copy link
Contributor

@ancosma ancosma commented Oct 2, 2022

In order to allow pods to reach other pods (as in Kubernetes) they all need to be added to the same network. A network is created (if it doesn't exist) and pods created by play-kube are added to that network. When network options are passed to kube command the pods are not attached to the default kube network.

Signed-off-by: Andrei Natanael Cosma [email protected]

Does this PR introduce a user-facing change?

Behavior of play-kube is changed to create a network (podman-kube) if it doesn't exists and adds pods created by kube-play to that network, which allows pods to reach other pods - as in Kubernetes.

It partially overlaps with #12965

@ancosma ancosma changed the title Add pods created by kube play to a default network Add pods created by play kube to a default network Oct 2, 2022
@rhatdan
Copy link
Member

rhatdan commented Oct 3, 2022

@Luap99 @mheon @umohnani8 WDYT

Should we take down the network, when all pods/containers are removed?

@mheon
Copy link
Member

mheon commented Oct 3, 2022

I see no reason to remove it.

Overall this feels like a good change. I would like to see it documented in the manpages, though.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should remopve this network, implementing this without TOCTOU race would require us to lock almost everything which can lead to performance problems.

Please add a test for this and remove [NO NEW TEST NEEDED] from your commit. This is only intended for things that cannot be tested in CI.

pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved
pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved
@ancosma ancosma force-pushed the kube-default-network branch from cca3364 to fb8c1c6 Compare October 3, 2022 18:00
@ancosma ancosma requested review from Luap99 and rhatdan and removed request for Luap99 and rhatdan October 3, 2022 19:57
@rhatdan
Copy link
Member

rhatdan commented Oct 4, 2022

@mheon @Luap99 PTAL

Probably should get this into podman 4.3?

@mheon
Copy link
Member

mheon commented Oct 4, 2022

Changes LGTM

docs/source/markdown/podman-kube-play.1.md.in Outdated Show resolved Hide resolved
pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved
@ancosma ancosma force-pushed the kube-default-network branch from fb8c1c6 to db4a8a7 Compare October 4, 2022 16:33
@ancosma ancosma requested a review from Luap99 October 4, 2022 16:39
pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved
In order to allow pods to reach other pods (as in Kubernetes) they all
need to be added to the same network. A network is created (if it
doesn't exist) and pods created by play-kube are added to that network.
When network options are passed to kube command the pods are not
attached to the default kube network.

Signed-off-by: Andrei Natanael Cosma <[email protected]>
@ancosma ancosma force-pushed the kube-default-network branch from 6589e6a to f250560 Compare October 4, 2022 20:00
@ancosma ancosma requested review from Luap99 and rhatdan and removed request for Luap99 and rhatdan October 5, 2022 09:46
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mheon Do you want to include this in v4.3 or wait for v4.4?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrei-n-cosma, Luap99

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 Oct 5, 2022
@mheon
Copy link
Member

mheon commented Oct 5, 2022

/lgtm
Since I'm getting distracted by other things and haven't gotten the release out yet, I can just rebase the 4.3 branch to include this before I cut the tag.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2022
@openshift-merge-robot openshift-merge-robot merged commit ab2f3cf into containers:main Oct 5, 2022
@ancosma ancosma deleted the kube-default-network branch October 5, 2022 14:30
@brendon
Copy link

brendon commented Mar 16, 2023

Just wondering if there's any documentation around how to access a container in another pod from the current pod in terms of host names?

@mheon
Copy link
Member

mheon commented Mar 17, 2023

The pod name is the hostname. No domain name should be necessary, appropriate search domains are set to ensure that.

@brendon
Copy link

brendon commented Mar 17, 2023

Excellent :) Yes I figured that out with a bit of trial and error. It's very elegant, well done!

@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 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 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