sdn: don't require netns on Update action #14446
Conversation
pkg/util/ovs/parse.go
Outdated
|
|
||
| func checkUnsupportedField(flow string, parsed *OvsFlow, field string) error { | ||
| if fieldSet(parsed, field) { | ||
| return fmt.Errorf("bad flow %q (field %q not supported)", flow, field) |
There was a problem hiding this comment.
Maybe rename this to "unimplemented"? That's what it really is. (The two fields I used it on, "out_port" and "out_group" have weird semantics that I didn't bother to implement because we aren't using them anywhere anyway.)
pkg/util/ovs/parse.go
Outdated
| if action == "" { | ||
| return nil, fmt.Errorf("cannot make field from empty action") | ||
| } | ||
| sep := strings.IndexAny(action, ":") |
There was a problem hiding this comment.
I think if there is a ( before the first : then you have to split there instead. Eg, ct(commit,exec(set_field:1->ct_mark),table=70) is a ct action, not a ct(commit,exec(set_field action.
Yeah, that complicates reassembly. (Though I think there aren't any situations where :( would be valid in an action, so you could say that if action.Value starts with ( then append it directly, else add a :.)
pkg/util/ovs/parse.go
Outdated
| flow = flow[end:] | ||
| } | ||
|
|
||
| if len(actions) > 0 { |
pkg/util/ovs/parse_test.go
Outdated
| }, | ||
| Actions: []OvsField{ | ||
| {Name: "move", Value: "NXM_NX_TUN_ID[0..31]->NXM_NX_REG0[]"}, | ||
| {Name: "goto_Table", Value: "10"}, |
There was a problem hiding this comment.
a rogue search-and-replace seems to have turned "goto_table" into "goto_Table" everywhere
pkg/sdn/plugin/ovscontroller.go
Outdated
| otx.AddFlow("table=20, priority=100, in_port=%d, arp, nw_src=%s, arp_sha=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, podIP, podMAC, vnid) | ||
| otx.AddFlow("table=20, priority=100, in_port=%d, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], goto_table:21", ofport, podIP, vnid) | ||
| otx.AddFlow("table=20, priority=100, in_port=%d, arp, nw_src=%s, arp_sha=%s, actions=load:%d->NXM_NX_REG0[], note:%s, goto_table:21", ofport, podIP, podMAC, vnid, note) | ||
| otx.AddFlow("table=20, priority=100, in_port=%d, ip, nw_src=%s, actions=load:%d->NXM_NX_REG0[], note:%s, goto_table:21", ofport, podIP, vnid, note) |
There was a problem hiding this comment.
You only need to add the note to the table=20 arp flow, not every flow for this pod. (Even if we needed info from some other flow later you could still get the ContainerID->port mapping from the first flow and then recognize the later flow by the port number.)
| note += fmt.Sprintf("%02x", b) | ||
| } | ||
| return note, nil | ||
| } |
There was a problem hiding this comment.
So this is equivalent to just adding a "." after every 2nd character in containerID, right? Except that it fails if containerID isn't actually a hex string.
There was a problem hiding this comment.
@danwinship yes. Though AFAIK container IDs will always be UUID format.
pkg/sdn/plugin/ovscontroller.go
Outdated
| return ofport, oc.setupPodFlows(ofport, podIP, podMAC, vnid) | ||
| note, err := getPodNote(containerID) | ||
| if err != nil { | ||
| return -1, err |
There was a problem hiding this comment.
might as well do this before setting up the ovs port
pkg/sdn/plugin/ovscontroller.go
Outdated
|
|
||
| // Find the table=20 flow with the given note and extract the podIP, ofport, and MAC from them | ||
| for _, flow := range flows { | ||
| parsed, err := ovs.ParseFlow(ovs.ParseForAdd, flow) |
There was a problem hiding this comment.
You're not doing an "add" here. There probably needs to be another ParseCmd value.
pkg/sdn/plugin/ovscontroller.go
Outdated
| continue | ||
| } | ||
| noteAction, ok := parsed.FindAction("note") | ||
| if !ok || !strings.HasPrefix(noteAction.Value, note) { |
There was a problem hiding this comment.
I've always felt guilty about assuming the note will always come back in lowercase (in ovscontroller.go:AlreadySetUp()) and was sort of hoping you'd cleverly avoid doing that here...
| @@ -126,7 +126,7 @@ func TestOVSHostSubnet(t *testing.T) { | |||
| }, | |||
| flowChange{ | |||
| kind: flowAdded, | |||
| match: []string{"table=50", "arp", "nw_dst=10.129.0.0/23", "192.168.1.2->tun_dst"}, | |||
| match: []string{"table=50", "arp", "arp_tpa=10.129.0.0/23", "192.168.1.2->tun_dst"}, | |||
There was a problem hiding this comment.
I think this fix goes with the previous commit?
In general kubelet doesn't really allow changes to pod attributes after the pod has started. We still want to allow VNID changes at least (for join/split) but bandwidth is not something we really want to support changing on-the-fly. It's hard to figure out how to change it if we can't get the pod's hostVethName, for which we need the pod's NetNS, which we can't get on Update() anymore due to CRI.
With the move to CRI, we don't get a Docker runtime from which we can grab the container's NetNS on update. But we don't actually need one, we can scrape OVS flows and grab the required details from there. The one downside is that if OpenShift crashed before pod flows were added, we cannot read the pod details on restart, and the pod will be left "ready" (because Docker started it) but with broken networking (because we can't fix it), but that's something that should be fixed upstream by ensuring that pods which have not successfully completed network setup are not considered "ready" by the runtime.
|
@danwinship I took all your comments... PTAL thanks! |
|
[test][testextended][extended:networking] |
|
Evaluated for origin test up to 805157b |
|
Evaluated for origin testextended up to 805157b |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/554/) (Base Commit: d973e45) (Extended Tests: networking) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1960/) (Base Commit: d973e45) |
|
[merge] |
|
Evaluated for origin merge up to 805157b |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/939/) (Base Commit: 9279133) (Image: devenv-rhel7_6333) |
With the move to CRI, we don't get a Docker runtime from which we
can grab the container's NetNS on update. But we don't actually
need one, we can scrape OVS flows and grab the required details
from there.
The one downside is that if OpenShift crashed before pod flows
were added, we cannot read the pod details on restart, and the pod
will be left "ready" (because Docker started it) but with broken
networking (because we can't fix it), but that's something that
should be fixed upstream by ensuring that pods which have not
successfully completed network setup are not considered "ready"
by the runtime.
@openshift/networking @danwinship
Fixes: https://trello.com/c/IP58IhfF/504-fix-pod-update-after-kube-16-rebase