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.

Sign up for GitHub

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

Jump to bottom

Podman network #1790

Merged
merged 5 commits into from
Dec 4, 2020
Merged

Podman network #1790

merged 5 commits into from
Dec 4, 2020

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Aug 18, 2020 •
edited
Loading

Since 0.8.0, KIND uses custom networks with docker to leverage the
embedded DNS server and other features.

This PR provides podman with the same functionality impemented in docker
Fixes: #1567

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/podman Issues or PRs related to podman size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2020
@aojea
Copy link
Contributor Author

aojea commented Aug 18, 2020

/assign @BenTheElder @amwat

@aojea
Copy link
Contributor Author

aojea commented Aug 18, 2020

/retest

@aojea
Copy link
Contributor Author

aojea commented Aug 19, 2020

in functionality terms, the only thing missing is the HA deployment, haproxy is hardcoded to use the docker resolver 127.0.0.1:53 , in podman it has the ip of the gw 🤷

@aojea aojea force-pushed the podmanNetwork branch 2 times, most recently from 9cd0819 to 5dfd856 Compare August 19, 2020 08:09
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2020
@aojea aojea force-pushed the podmanNetwork branch 4 times, most recently from 6c79c38 to e83290e Compare August 23, 2020 10:17
@aojea
Copy link
Contributor Author

aojea commented Aug 23, 2020

missing the port forwarding on ipv6 only :)

@aojea aojea force-pushed the podmanNetwork branch 8 times, most recently from 3b69af9 to 5cd0388 Compare August 25, 2020 11:13
@BenTheElder
Copy link
Member

It creates one network per cluster. The network has the same lifecycle than the nodes

are we unable to use one network?

using one network means less CIDRs need to be allocated by the user and I would expect it to generally be easier to work with.

@aojea
Copy link
Contributor Author

aojea commented Aug 26, 2020

It creates one network per cluster. The network has the same lifecycle than the nodes

are we unable to use one network?

using one network means less CIDRs need to be allocated by the user and I would expect it to generally be easier to work with.

podman only support single stack networks, ipv4 or ipv6, that means that for the one bridge model we have to create one per each family
podman does not isolate container networks, like docker, and I found this model more powerful for networking setups and I like it more.
I think that each provider is different with different user base, and I'm start to think that podman will be more attractive in the networking area, since it has less constraints and it's Linux only,

@aojea
Copy link
Contributor Author

aojea commented Aug 26, 2020

/retest

@BenTheElder
Copy link
Member

/hold
we chatted a bit about the approach last night, need to summarize here but TLDR need to rethink per-cluster network.
let's see how #1831 lands in 0.9 and visit this in 0.10 along with a broader aim to stabilize podman

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020
@aojea
Copy link
Contributor Author

aojea commented Nov 22, 2020

changed Podman to use same approach than docker
blocked by
containers/podman#8444

@aojea aojea force-pushed the podmanNetwork branch 5 times, most recently from 8d899a2 to a43878b Compare December 2, 2020 21:35
Antonio Ojea added 4 commits December 2, 2020 22:38
CI podman IPv6 multinode
a6cb2b7 
don't test podman HA clusters

podman ci for stable and testing versions

fix ci
fix ipv6 log error to consider both providers
f57621e
podman user defined network
e6078b4 
implement user defined networks in podman
podman hostport info for 2.2.0+
50d40e3
podman exclude ipv6 from the CI
847c825 
the problem is that the API is forwarded in localhost, and
portmapping doesn' t work from localhost to localhost
@aojea
Copy link
Contributor Author

aojea commented Dec 3, 2020

/hold cancel
Forget to remove it, this is ok for now, I will fix ipv6 later
Ping @BenTheElder

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2020
@aojea aojea mentioned this pull request Dec 3, 2020
Merged
BenTheElder
BenTheElder reviewed Dec 4, 2020
View reviewed changes
pkg/cluster/internal/create/actions/config/config.go
@@ -201,7 +201,7 @@ func getKubeadmConfig(cfg *config.Cluster, data kubeadm.ConfigData, node nodes.N
// configure the right protocol addresses
if cfg.Networking.IPFamily == "ipv6" {
if nodeAddressIPv6 == "" {
return "", errors.Errorf("failed to get IPV6 address; is the docker daemon configured to use IPV6 correctly?")
return "", errors.Errorf("failed to get IPV6 address; is the container provider (docker,podman) configured to use IPV6 correctly?")
Copy link
Member

Choose a reason for hiding this comment

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

TODO: we can insert the name of the one we're using here, there's a .String() on provider

Copy link
Member

Choose a reason for hiding this comment

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

(we want to avoid useless detail / sending people looking at the wrong thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1960

@BenTheElder
Copy link
Member

one nit for a follow-up
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2020
@k8s-ci-robot
Copy link
Contributor

@aojea: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kind-conformance-parallel-ipv6 847c825 link /test pull-kind-conformance-parallel-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@BenTheElder BenTheElder merged commit 058a6cd into kubernetes-sigs:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder left review comments

@krzyzacy krzyzacy Awaiting requested review from krzyzacy

Assignees

@BenTheElder BenTheElder

@amwat amwat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/docker Issues or PRs related to docker area/provider/podman Issues or PRs related to podman cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[podman] External Load Balancer for HA cluster
4 participants
@aojea @BenTheElder @k8s-ci-robot @amwat