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 private network implementation for podman #9716

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

afbjorklund
Copy link
Collaborator

Most of it the same as docker, except for the options.

i.e. libnetwork "bridge" plugin vs. cni "bridge" plugin

Does not remove the network when deleting

Closes #9714

@afbjorklund afbjorklund requested a review from medyagh November 16, 2020 16:44
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2020
args = append(args, "--icc")

// adding MTU option because #9528
if mtu > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

could podman network benefit from specifying the MTU ? (this fixed the issue of minikube in cloud shell not being able to pull large images like golang:1.15 due to different cloud shell MTUs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might, but it will not benefit from "com.docker.network.driver.mtu" at any rate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be available in the future (like podman 3.0 or so), with a similar option

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment, to remember TODO add mtu for podman 3 (with an issue liunk)

@@ -136,12 +138,16 @@ type netInfo struct {
}

// if exists returns subnet, gateway and mtu
func dockerNetworkInspect(name string) (netInfo, error) {
func dockerNetworkInspect(ociBin string, name string) (netInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be renamed to networkInspect ? if it is working for both podman and docker ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is "docker" all over the place, I didn't rename any functions.

Copy link
Member

Choose a reason for hiding this comment

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

I mean if we are using it for both docker and podman now (unlike before) we should rename it to networkInspect

@afbjorklund
Copy link
Collaborator Author

Currently it is not possible to configure MTU:

// HostLocalBridge describes a configuration for a bridge plugin
// https://github.com/containernetworking/plugins/tree/master/plugins/main/bridge#network-configuration-reference
type HostLocalBridge struct {
        PluginType   string            `json:"type"`
        BrName       string            `json:"bridge,omitempty"`
        IsGW         bool              `json:"isGateway"`
        IsDefaultGW  bool              `json:"isDefaultGateway,omitempty"`
        ForceAddress bool              `json:"forceAddress,omitempty"`
        IPMasq       bool              `json:"ipMasq,omitempty"`
        MTU          int               `json:"mtu,omitempty"`
        HairpinMode  bool              `json:"hairpinMode,omitempty"`
        PromiscMode  bool              `json:"promiscMode,omitempty"`
        Vlan         int               `json:"vlan,omitempty"`
        IPAM         IPAMHostLocalConf `json:"ipam"`
}
// NewHostLocalBridge creates a new LocalBridge for host-local
func NewHostLocalBridge(name string, isGateWay, isDefaultGW, ipMasq bool, ipamConf IPAMHostLocalConf) *HostLocalBridge {
        hostLocalBridge := HostLocalBridge{
                PluginType:  "bridge",
                BrName:      name,
                IPMasq:      ipMasq,
                HairpinMode: true,
                IPAM:        ipamConf,
        }
        if isGateWay {
                hostLocalBridge.IsGW = true
        }
        if isDefaultGW {
                hostLocalBridge.IsDefaultGW = true
        }
        return &hostLocalBridge
}

So the feature needs to be added to podman first.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2020
Most of it the same as docker, except for the options.

i.e. libnetwork "bridge" plugin vs. cni "bridge" plugin
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2020
@afbjorklund
Copy link
Collaborator Author

Another request for MTU: containers/podman#8454

See #9716 (comment)

@medyagh
Copy link
Member

medyagh commented Nov 26, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, medyagh

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 [afbjorklund,medyagh]

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

@minikube-pr-bot
Copy link

kvm2 Driver
error collecting results for kvm2 driver: timing run 0 with Minikube (PR 9716): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/9716/minikube start --driver=kvm2]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/9716/minikube: exec format error
docker Driver
error collecting results for docker driver: timing run 0 with Minikube (PR 9716): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/9716/minikube start --driver=docker]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/9716/minikube: exec format error

@afbjorklund
Copy link
Collaborator Author

Support for MTU will be available in Podman 3.0, can revisit it again then...

containers/podman#8457

@medyagh medyagh merged commit 8fc54b4 into kubernetes:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dedicate network and static IP for podman driver
4 participants