Upgrade whereabouts CNI to v0.5.4 and align secondary network IPAM with additional pluginArgs #3987
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3987 +/- ##
==========================================
- Coverage 64.58% 64.51% -0.08%
==========================================
Files 295 294 -1
Lines 43740 44314 +574
==========================================
+ Hits 28251 28590 +339
- Misses 13215 13420 +205
- Partials 2274 2304 +30
*This pull request uses carry forward flags. Click here to find out more.
|
|
/test-e2e |
| @@ -28,8 +28,9 @@ import ( | |||
| ) | |||
There was a problem hiding this comment.
But secondary network uses "pkg/agent/secondarynetwork/ipam". Why we need to register Whereabouts in cniserver/ipam?
There was a problem hiding this comment.
It uses both the IPAM packages @jianjuns.
"pkg/agent/secondarynetwork/ipam"(whereabouts) is used to get the IPAddress from a specific subnet based on the virtual network provided on the Pod spec Annotation. But the IPaddress assignment to the VF interface is actually handled using cniserver/ipam APIs (configureInterface).
I'm looking to confirm that this fix actually address the issue. But have some lab connectivity issues since yesterday evening and I'm working with my peers at Santa clara lab to recover it.
Once the lab connectivity is restored today, I will retest this and confirm.
There was a problem hiding this comment.
Register whereabouts as a valid IPAMDriver at cniserver/IPAM fixed the "Unsupported IPAM type whereabouts" issue (When pod controller uses the CNI server IPAM APIs to assign the IP address to the secondary network interface at the pod context - Though the Virtual network specific IP addressed was retrieved using secondarynetwork/ipam API which using whereabouts CNI for IPAM).
arunvelayutham
force-pushed
the
antrea-whereabout-cni-support
branch
from
546489e to
66af7fb
Compare
arunvelayutham
force-pushed
the
antrea-whereabout-cni-support
branch
2 times, most recently
from
e1284d7 to
8ff0fd8
Compare
arunvelayutham
changed the title
| @@ -365,4 +366,7 @@ func init() { | |||
|
|
|||
| // Host local plugin is fallback driver | |||
| RegisterIPAMDriver(AntreaIPAMType, &IPAMDelegator{pluginType: ipamHostLocal}) | |||
|
|
|||
| // Whereabouts plugin for secondary network configuration. | |||
| RegisterIPAMDriver(WhereaboutsIPAMType, &IPAMDelegator{pluginType: WhereaboutsIPAMType}) | |||
There was a problem hiding this comment.
Have you figured out why this is needed?
There was a problem hiding this comment.
I retested after removing the IPAM driver reg for whereabouts and with all the other fixes specific to whereabouts. It works good without registration. Not sure why we got the unsupport IPAM error earlier (before root cause other issues related to whereabouts configuration).
Thanks @jianjuns . I removed the IPAM reg part and pushed the updated changes.
With the updated changes, secondary interface is getting created successfully and got the IP assigned for the specific subnet (based on Annotation).
kubectl exec -it testpodsec15 -n kube-system -- ip addr show
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
inet 127.0.0.1/8 scope host lo
valid_lft forever preferred_lft forever
3: eth0@if113: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1450 qdisc noqueue
link/ether 7a:4c:50:d6:ea:24 brd ff:ff:ff:ff:ff:ff
inet 172.25.0.46/24 brd 172.25.0.255 scope global eth0
valid_lft forever preferred_lft forever
34: eth2: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1450 qdisc mq qlen 1000
link/ether 5e:eb:3e:08:16:04 brd ff:ff:ff:ff:ff:ff
inet 148.14.25.12/24 brd 148.14.25.255 scope global eth2
valid_lft forever preferred_lft forever
arunvelayutham
force-pushed
the
antrea-whereabout-cni-support
branch
2 times, most recently
from
7f4476e to
56982bd
Compare
| NetNS: podCNIInfo.ContainerNetNS, Path: cniPath, PluginArgs: [][2]string{ | ||
| {"K8S_POD_NAME", podCNIInfo.PodName}, | ||
| {"K8S_POD_NAMESPACE", podCNIInfo.PodNameSpace}, | ||
| {"K8S_POD_INFRA_CONTAINER_ID", podCNIInfo.ContainerID}, | ||
| }} |
There was a problem hiding this comment.
maybe add a comment to explain that these variables are required by the version of whereabouts we are using
There was a problem hiding this comment.
also just to confirm: whereabouts expect these args for DEL and not just for ADD?
| @@ -190,7 +194,7 @@ func (pc *PodController) handleAddUpdatePod(obj interface{}) error { | |||
| // Avoid processing Pod annotation, if we already have at least one secondary network successfully configured on this Pod. | |||
| // We do not support/handle Annotation updates yet. | |||
| if len(podCNIInfo.NetworkConfig) > 0 { | |||
| klog.InfoS("Secondary network already configured on this Pod. Annotation update not supported.", klog.KObj(pod)) | |||
| klog.InfoS("Secondary network already configured on this Pod. Annotation update not supported.", "Pod", klog.KObj(pod)) | |||
There was a problem hiding this comment.
| klog.InfoS("Secondary network already configured on this Pod. Annotation update not supported.", "Pod", klog.KObj(pod)) | |
| klog.InfoS("Secondary network already configured on this Pod and annotation update not supported, skipping update", "pod", klog.KObj(pod)) |
(We don't include a period at the end of log messages)
There was a problem hiding this comment.
yes. took care.
| @@ -295,7 +299,11 @@ func (pc *PodController) configureSecondaryInterface(pod *corev1.Pod, netinfo *S | |||
| if netinfo.InterfaceType == sriovInterfaceType { | |||
There was a problem hiding this comment.
is there a way to avoid code duplication by using a helper function to craft the cni plugin args?
There was a problem hiding this comment.
Sure. Moved the command building into a separate function whereaboutsCmdBuilder().
| @@ -329,7 +337,7 @@ func (pc *PodController) configureSecondaryInterface(pod *corev1.Pod, netinfo *S | |||
| func (pc *PodController) configureSecondaryNetwork(pod *corev1.Pod, networklist []*SecondaryNetworkObject, podCNIInfo *cnipodcache.CNIConfigInfo) error { | |||
|
|
|||
| for _, netinfo := range networklist { | |||
| klog.InfoS("Secondary Network Information:", netinfo) | |||
| klog.InfoS("Secondary Network Information:", "netInfo", netinfo) | |||
There was a problem hiding this comment.
| klog.InfoS("Secondary Network Information:", "netInfo", netinfo) | |
| klog.InfoS("Secondary Network Information", "info", netinfo) |
arunvelayutham
force-pushed
the
antrea-whereabout-cni-support
branch
from
56982bd to
31da3d8
Compare
| const ( | ||
| cmd_add = "ADD" | ||
| cmd_del = "DEL" | ||
| ) |
There was a problem hiding this comment.
I don't think this is necessary (in general I don't see a point in defining constants for a short hardcoded string that's never going to change).
On a side note (but not important if you remove those), we don't use snake case for names, only camel case.
| @@ -127,12 +132,24 @@ func generatePodSecondaryIfaceName(podCNIInfo *cnipodcache.CNIConfigInfo) string | |||
| return string("eth") + strconv.Itoa(rand.IntnRange(end_iface_index, max_rand_index)) | |||
| } | |||
|
|
|||
| func whereaboutsCmdBuilder(cmd string, interfaceName string, podCNIInfo *cnipodcache.CNIConfigInfo) *invoke.Args { | |||
There was a problem hiding this comment.
s/whereaboutsCmdBuilder/whereaboutsArgsBuilder
arunvelayutham
force-pushed
the
antrea-whereabout-cni-support
branch
from
31da3d8 to
bb7da9a
Compare
|
/test-all |
2 similar comments
|
/test-all |
|
/test-all |
…th additional pluginArgs. - Modify base Dockerfile to pull whereabouts-v0.5.4. - Update whereabouts clusterrole with additional API group resources. - Update whereabouts cmdArgs to provide PodName and PodNameSpace information. Signed-off-by: root <[email protected]>
Register Whereabouts CNI as a valid IPAMType for secondary network configuration. Whereabouts is the IPAM used to configure secondary/virtual networks for Service function chaining needs at this time.
Signed-off-by: [email protected]