Skip to content

Commit 6c42efd

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #18418 from danwinship/sdn-out-of-process-cleanups
Automatic merge from submit-queue (batch tested with PRs 18737, 18418). Minor cleanups to sdn-cni-plugin Based on belated comments on #18355: > So what if we just stuff the PodIP into external-ids? Then we don't need to dump and parse flows all the time. I had thought about that, but then we'd have to change ovs.Find() to return multiple columns (`ofport` and `external-ids`) and we'd have to parse the external-ids to separate out the sandboxID from the podIP. Though, admittedly, there's already code to parse external-ids in fake_ovs now anyway... Also, we're only parsing a single flow now, because we request "in_port=%d" in the dump-flows. > Any reason to do [hostVeth] in the environment rather than just stuff it into the JSON by extending CNIRequest? I actually did do it that way first, and then changed it. Something about adding a field to CNIRequest had weird unexpected side effects. IIRC, it would have required a whole bunch of changes to the unit tests or something like that. But I can change it if you prefer.
2 parents 0757a91 + 3fead8c commit 6c42efd

File tree

3 files changed

+24
-29
lines changed

3 files changed

+24
-29
lines changed

‎pkg/network/node/cniserver/cniserver.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ type CNIRequest struct {
7373
Env map[string]string `json:"env,omitempty"`
7474
// CNI configuration passed via stdin to the CNI plugin
7575
Config []byte `json:"config,omitempty"`
76+
// Host side of the veth pair (for an ADD command)
77+
HostVeth string `json:"hostVeth,omitempty"`
7678
}
7779

7880
// Request structure built from CNIRequest which is passed to the
@@ -246,9 +248,9 @@ func cniRequestToPodRequest(r *http.Request) (*PodRequest, error) {
246248
return nil, fmt.Errorf("missing CNI_NETNS")
247249
}
248250

249-
req.HostVeth, ok = cr.Env["OSDN_HOSTVETH"]
250-
if !ok && req.Command == CNI_ADD {
251-
return nil, fmt.Errorf("missing OSDN_HOSTVETH")
251+
req.HostVeth = cr.HostVeth
252+
if req.HostVeth == "" && req.Command == CNI_ADD {
253+
return nil, fmt.Errorf("missing HostVeth")
252254
}
253255

254256
cniArgs, err := gatherCNIArgs(cr.Env)

‎pkg/network/node/cniserver/cniserver_test.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,9 @@ func TestCNIServer(t *testing.T) {
102102
"CNI_CONTAINERID": "adsfadsfasfdasdfasf",
103103
"CNI_NETNS": "/path/to/something",
104104
"CNI_ARGS": "K8S_POD_NAMESPACE=awesome-namespace;K8S_POD_NAME=awesome-name",
105-
"OSDN_HOSTVETH": "vethABC",
106105
},
107-
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
106+
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
107+
HostVeth: "vethABC",
108108
},
109109
result: expectedResult,
110110
},
@@ -144,9 +144,9 @@ func TestCNIServer(t *testing.T) {
144144
"CNI_COMMAND": string(CNI_ADD),
145145
"CNI_CONTAINERID": "adsfadsfasfdasdfasf",
146146
"CNI_NETNS": "/path/to/something",
147-
"OSDN_HOSTVETH": "vethABC",
148147
},
149-
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
148+
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
149+
HostVeth: "vethABC",
150150
},
151151
result: nil,
152152
errorPrefix: "missing CNI_ARGS",
@@ -159,9 +159,9 @@ func TestCNIServer(t *testing.T) {
159159
"CNI_COMMAND": string(CNI_ADD),
160160
"CNI_CONTAINERID": "adsfadsfasfdasdfasf",
161161
"CNI_ARGS": "K8S_POD_NAMESPACE=awesome-namespace;K8S_POD_NAME=awesome-name",
162-
"OSDN_HOSTVETH": "vethABC",
163162
},
164-
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
163+
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
164+
HostVeth: "vethABC",
165165
},
166166
result: nil,
167167
errorPrefix: "missing CNI_NETNS",
@@ -174,14 +174,14 @@ func TestCNIServer(t *testing.T) {
174174
"CNI_CONTAINERID": "adsfadsfasfdasdfasf",
175175
"CNI_NETNS": "/path/to/something",
176176
"CNI_ARGS": "K8S_POD_NAMESPACE=awesome-namespace;K8S_POD_NAME=awesome-name",
177-
"OSDN_HOSTVETH": "vethABC",
178177
},
179-
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
178+
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
179+
HostVeth: "vethABC",
180180
},
181181
result: nil,
182182
errorPrefix: "unexpected or missing CNI_COMMAND",
183183
},
184-
// Missing OSDN_HOSTVETH
184+
// Missing HostVeth
185185
{
186186
name: "ARGS4",
187187
request: &CNIRequest{
@@ -194,7 +194,7 @@ func TestCNIServer(t *testing.T) {
194194
Config: []byte("{\"cniVersion\": \"0.1.0\",\"name\": \"openshift-sdn\",\"type\": \"openshift-sdn\"}"),
195195
},
196196
result: nil,
197-
errorPrefix: "missing OSDN_HOSTVETH",
197+
errorPrefix: "missing HostVeth",
198198
},
199199
}
200200

‎pkg/network/sdn-cni-plugin/openshift-sdn.go

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ func (p *cniPlugin) doCNI(url string, req *cniserver.CNIRequest) ([]byte, error)
9595
// Send the ADD command environment and config to the CNI server, returning
9696
// the IPAM result to the caller
9797
func (p *cniPlugin) doCNIServerAdd(req *cniserver.CNIRequest, hostVeth string) (types.Result, error) {
98-
req.Env["OSDN_HOSTVETH"] = hostVeth
98+
req.HostVeth = hostVeth
9999
body, err := p.doCNI("http://dummy/", req)
100100
if err != nil {
101101
return nil, err
@@ -117,20 +117,14 @@ func (p *cniPlugin) testCmdAdd(args *skel.CmdArgs) (types.Result, error) {
117117

118118
func (p *cniPlugin) CmdAdd(args *skel.CmdArgs) error {
119119
req := newCNIRequest(args)
120-
ifname := req.Env["CNI_IFNAME"]
121-
netns := req.Env["CNI_NETNS"]
122-
if ifname == "" || netns == "" {
123-
return fmt.Errorf("CNI request did not include required environment variables")
124-
}
125-
126120
config, err := cniserver.ReadConfig(cniserver.CNIServerConfigFilePath)
127121
if err != nil {
128122
return err
129123
}
130124

131125
var hostVeth, contVeth net.Interface
132-
err = ns.WithNetNSPath(netns, func(hostNS ns.NetNS) error {
133-
hostVeth, contVeth, err = ip.SetupVeth(ifname, int(config.MTU), hostNS)
126+
err = ns.WithNetNSPath(args.Netns, func(hostNS ns.NetNS) error {
127+
hostVeth, contVeth, err = ip.SetupVeth(args.IfName, int(config.MTU), hostNS)
134128
if err != nil {
135129
return fmt.Errorf("failed to create container veth: %v", err)
136130
}
@@ -163,20 +157,19 @@ func (p *cniPlugin) CmdAdd(args *skel.CmdArgs) error {
163157
// The only interface we report is the pod interface.
164158
result030.Interfaces = []*current.Interface{
165159
{
166-
Name: ifname,
160+
Name: args.IfName,
167161
Mac: contVeth.HardwareAddr.String(),
168-
Sandbox: netns,
162+
Sandbox: args.Netns,
169163
},
170164
}
171-
index := 0
172-
result030.IPs[0].Interface = &index
165+
result030.IPs[0].Interface = current.Int(0)
173166

174-
err = ns.WithNetNSPath(netns, func(ns.NetNS) error {
167+
err = ns.WithNetNSPath(args.Netns, func(ns.NetNS) error {
175168
// Set up eth0
176-
if err := ip.SetHWAddrByIP(ifname, result030.IPs[0].Address.IP, nil); err != nil {
169+
if err := ip.SetHWAddrByIP(args.IfName, result030.IPs[0].Address.IP, nil); err != nil {
177170
return fmt.Errorf("failed to set pod interface MAC address: %v", err)
178171
}
179-
if err := ipam.ConfigureIface(ifname, result030); err != nil {
172+
if err := ipam.ConfigureIface(args.IfName, result030); err != nil {
180173
return fmt.Errorf("failed to configure container IPAM: %v", err)
181174
}
182175

0 commit comments

Comments
 (0)