sdn: disable hostports when CRI-O is used #16463
Conversation
openshift-ci-robot
added
the
size/M
This is a workaround because CRIO's CNI driver has different hostport behavior than Kubernetes' CNI driver. Kube leaves all hostport handling to the CNI plugin itself, while CRIO does hostport handling internally, leading to duplicate work if the plugin also handles hostports. For now, detect the runtime based on socket path and disable openshift-sdn hostport handling if the runtime is CRIO. The real fix for this is to add hostport handling to Kube's CNI driver, but in the "split" mode discussed upstream where kube's CNI driver handles the port reservations on the host, while the plugin handles the actual iptables rules. CRIO should be converted to this scheme as well, and plugins will indicate with capabilities in the CNI JSON whether they support this scheme or not. At that point we can remove this hack and just have openshift-sdn advertise portmapping support via the CNI JSON.
dcbw
force-pushed
the
sdn-crio-hostports
branch
from
4f9aa7c
to
59b2b62
Compare
|
LGTM |
Are there kube and CNI bugs filed about this? |
| @@ -499,8 +504,10 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running | |||
| defer func() { | |||
| if !success { | |||
| m.ipamDel(req.SandboxID) | |||
| if err := m.hostportSyncer.SyncHostports(Tun0, m.getRunningPods()); err != nil { | |||
| glog.Warningf("failed syncing hostports: %v", err) | |||
| if m.hostportSyncer != nil { | |||
There was a problem hiding this comment.
chain of booleans. Can we check for enableHostports here?
| @@ -511,8 +518,10 @@ func (m *podManager) setup(req *cniserver.PodRequest) (cnitypes.Result, *running | |||
| return nil, nil, err | |||
| } | |||
| podPortMapping := kubehostport.ConstructPodPortMapping(&v1Pod, podIP) | |||
| if err := m.hostportSyncer.OpenPodHostportsAndSync(podPortMapping, Tun0, m.getRunningPods()); err != nil { | |||
| return nil, nil, err | |||
| if m.hostportSyncer != nil { | |||
| @@ -631,8 +640,10 @@ func (m *podManager) teardown(req *cniserver.PodRequest) error { | |||
| errList = append(errList, err) | |||
| } | |||
|
|
|||
| if err := m.hostportSyncer.SyncHostports(Tun0, m.getRunningPods()); err != nil { | |||
| errList = append(errList, err) | |||
| if m.hostportSyncer != nil { | |||
It's mostly about CRIO. In yesterday's SIG Net meeting Tim indicated he'd like to punt all this out to runtimes and plugins. Kube's CNI driver doesn't do anything related to hostports at all. And CRIO probably shouldn't either, that's what the CNI 'portmap' plugin is for. CRIO issue filed as cri-o/cri-o#941 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, dcbw, kargakis, rajatchopra The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
the
approved
|
Automatic merge from submit-queue |
For now...
@openshift/networking @knobunc @rajatchopra