Antrea secondary network implementation for SRIOV #2651

arunvelayutham · 2021-08-26T06:58:05Z

Implement secondary network for Antrea CNI configured pods using SRIOV interface. (CNF use case)

Signed-off-by: root arunkumar.velayutham@intel.com

arunvelayutham · 2021-08-26T07:05:43Z

build/yamls/antrea.yml

@@ -3246,6 +3246,11 @@ metadata:
    app: antrea
  name: antrea-agent
 rules:
+- apiGroups: ["k8s.cni.cncf.io"]


@antoninbas , @jianjuns Yaml file patch using generate-manifest.sh is not working good for RBAC changes. I would like to discuss with both of you to see how we can handle the antea.yml changes required for k8s.cni.cncf.io apiGroups access (required for net-attach-def access from client side)

arunvelayutham · 2021-08-26T07:08:15Z

build/yamls/patches/netattachdef/net-attach-def.yml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1


This patch needs review and get it addressed as in a way to patch properly at antrea.yml. (As of now, it replaces entire RBAC section for antrea-agent).

I feel like this doesn't need to be done conditionally though patch, we can give the required permissions (to access Network Attachment Definitions) to the Agent by modifying: https://github.com/antrea-io/antrea/blob/main/build/yamls/base/agent-rbac.yml

Agreed. thanks for the suggestion. taken care.

arunvelayutham · 2021-08-26T07:11:34Z

build/images/scripts/install_whereabouts_config

+
+set -u -e
+
+# Inspired by: https://github.com/intel/multus-cni/blob/83556f49bd6706a885eda847210b542669279cd0/images/entrypoint.sh#L161-L222


Required to install kubeconfig for whereabouts to talk to API server. this script will be called as part of init_container scope to be installed at the defined location (as per whereabouts requirements).
NOTE: Script copied/taken from whereabouts-cni repo.

arunvelayutham · 2021-08-26T07:14:20Z

cmd/antrea-agent/agent.go

@@ -309,6 +322,30 @@ func run(o *Options) error {

 	go networkPolicyController.Run(stopCh)

+	if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport) {
+		//TBD: Initialize Secondary Network Infra here.


Secondary network infra code changes will be merged separate (which is responsible to create the OVS bridges and config - not required for SRIOV based secondary network config).

I would move this block to a separate func.

arunvelayutham · 2021-08-26T07:16:09Z

pkg/agent/cnf/cnipodcache/cni_info_cache.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cnipodcache


CNI cacheStore implementation to cache CNIserver information of a pod, which is required to be used by podwatch pkg while configuring secondary network

arunvelayutham · 2021-08-26T07:17:50Z

pkg/agent/cnf/ipamglobal/ipam_global_delegator.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ipamglobal


Global IPAM implementation to interface with whereabouts-cni config and assign Cluster wide unique IP address (maintained by whereabouts at api server or etcd - configurable)

Could we just name the package "ipam", and the file ipam_delegator.go? I feel no need to emphasize on "global".

arunvelayutham · 2021-08-26T07:20:49Z

pkg/agent/cniserver/server.go

@@ -105,7 +107,7 @@ type CNIServer struct {
 	hostProcPathPrefix   string
 	kubeClient           clientset.Interface
 	containerAccess      *containerAccessArbitrator
-	podConfigurator      *podConfigurator
+	PodConfigurator      *podConfigurator


this is a temporary/interim change until we redo the cniserver package and split them into 3 different pkg, make it reusable for primary and secondary interface needs.

To avoid touch too many code, could you add a func like GetPodConfigurator() to CNIServer instead?

arunvelayutham · 2021-08-26T07:21:36Z

pkg/agent/cniserver/server.go

@@ -461,6 +463,15 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (*
 	klog.Infof("CmdAdd for container %v succeeded", cniConfig.ContainerId)
 	// mark success as true to avoid rollback
 	success = true
+
+	if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport) {


There is where we cache CNI server provided pod information into cnipodcache.

Could we add and pass a boolean like secInterfaceEnabled to CNIServer, instead of checking the feature gate inside the CNIServer code? I would avoid featuregate in all code.

Or based on whether the cniPodInfoStore argument passed to Initialize() is nil, we can save the value to a boolean flag of CNIServer struct, like cacheCNIPodInfo, and check s.cacheCNIPodInfo here.

codecov-commenter · 2021-08-26T08:39:16Z

Codecov Report

Merging #2651 (9aa489e) into main (0f5df48) will decrease coverage by 8.85%.
The diff coverage is 5.12%.

❗ Current head 9aa489e differs from pull request most recent head 7e0a14e. Consider uploading reports for the commit 7e0a14e to get more accurate results

@@            Coverage Diff             @@
##             main    #2651      +/-   ##
==========================================
- Coverage   59.23%   50.37%   -8.86%     
==========================================
  Files         302      300       -2     
  Lines       25873    35722    +9849     
==========================================
+ Hits        15325    17994    +2669     
- Misses       8892    15993    +7101     
- Partials     1656     1735      +79

Flag	Coverage Δ
e2e-tests	`50.37% <5.12%> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/cniserver/sriov.go	`0.00% <0.00%> (ø)`
pkg/agent/cniserver/sriov_linux.go	`0.00% <0.00%> (ø)`
pkg/agent/config/node_config.go	`88.88% <ø> (-11.12%)`	⬇️
pkg/agent/secondarynetwork/cnipodcache/cache.go	`0.00% <0.00%> (ø)`
pkg/agent/secondarynetwork/ipam/ipam_delegator.go	`0.00% <0.00%> (ø)`
pkg/agent/secondarynetwork/podwatch/controller.go	`0.00% <0.00%> (ø)`
pkg/features/antrea_features.go	`22.22% <ø> (+5.55%)`	⬆️
pkg/util/k8s/client.go	`36.36% <0.00%> (-14.86%)`	⬇️
pkg/agent/cniserver/server.go	`40.58% <19.23%> (-28.31%)`	⬇️
pkg/agent/route/route_linux.go	`29.91% <54.83%> (-16.30%)`	⬇️
... and 304 more

antoninbas

some initial comments

antoninbas · 2021-08-26T22:17:42Z

build/yamls/patches/netattachdef/net-attach-def.yml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1


I feel like this doesn't need to be done conditionally though patch, we can give the required permissions (to access Network Attachment Definitions) to the Agent by modifying: https://github.com/antrea-io/antrea/blob/main/build/yamls/base/agent-rbac.yml

antoninbas · 2021-08-26T22:18:43Z

build/yamls/patches/netattachdef/net-attach-def.yml

+  - '*'
+  verbs:
+  - '*'


we should specify just the resource type we need (network-attachment-definition) and just the permissions / verbs we need

yes, Taken care (I used "*" only for dev needs to ensure that initial agent able to fetch the net-attach-def CR info from APIserver).

antoninbas · 2021-08-26T22:20:22Z

build/images/scripts/install_whereabouts_config

+WHEREABOUTS_KUBECONFIG_LITERAL=$(echo "$WHEREABOUTS_KUBECONFIG" | sed -e s'|/host||')
+
+# ------------------------------- Generate a "kube-config"
+SERVICE_ACCOUNT_PATH=/var/run/secrets/kubernetes.io/serviceaccount


This will use the antrea-agent Pod's ServiceAccount, which has a lot of permissions. We should restrict the whereabouts permissions to what's actually needed (access to resources in the whereabouts.cni.cncf.io API group). Maybe @tnqn will have a suggestion on what's the best way to generate and install that Kubeconfig file (which is consumed by the whereabouts plugin).

Yet to take care on this concern and I fully agree with the suggestion. found new issue with whereabouts IP config and working on further to debug and address it.

antoninbas · 2021-08-26T22:24:25Z

cmd/antrea-agent/agent.go

+	if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport) {
+		//TBD: Initialize Secondary Network Infra here.
+
+		//Create NetworkAttachmentDefinitions client handle to access secondary network object definition from API Server.


nit: we typically leave a space between the // and the actual comment

antoninbas · 2021-08-26T22:26:37Z

cmd/antrea-agent/agent.go

+			informerFactory,
+			nodeConfig.Name,
+			cniPodInfoStore,
+			cniServer)


we should change that, there should be no need for the "pod watcher" to access the instance of the cniServer. I believe you are already working on that.

yes certainly. I'm using it as temporary solution for this PR and I will take care of this as part of CNIserver pkg refactor PR.

antoninbas · 2021-08-26T22:35:53Z

pkg/agent/cnf/podwatch/k8s/podwatch_controller.go

+			return errors.New("Secondary network annotation parsing failed")
+		} else {
+			for _, netinfo := range networklist {
+				klog.Infof("Secondary Network Information: %+v", netinfo)


we have transitioned to structured logging function (klog.InfoS, klog.ErrorS) for all new logs. Please update the PR accordingly. You can find some examples in the rest of the PR.

antoninbas · 2021-08-26T22:39:29Z

pkg/agent/cnf/cnipodcache/cni_info_cache.go

@@ -0,0 +1,80 @@
+// Copyright 2021 Antrea Authors


I would like to suggest a different package name instead of pkg/agent/cnf. Maybe pkg/agent/secondarynetworks?

cc @jianjuns

Agreed. Secondary interfaces might be used for other cases besides CNF.

How about secondaryinterfaces?

Also feel we can just name this file "cache.go", as it is already in the "cnipodcache" package.

antoninbas · 2021-08-26T22:42:37Z

pkg/agent/cnf/podwatch/k8s/podwatch_controller.go

+	//If container information is not available yet in the CNICache, return error to requeue work
+	podCniInfo := podWatcher.podCache.GetCniConfigInfoPerPod(pod.Name, pod.Namespace)
+	if len(podCniInfo) == 0 {
+		return errors.New("CNI Cache Information not found")


we will hit this "normal" case pretty often since there will be some delay between the Pod object being created in the k8s API and the CNI being invoked. We should avoid logging this error as it will be misleading to the user, but we also need to ensure that the Pod is added back to the workqueue so that there can be a retry later.

antoninbas · 2021-08-26T22:43:40Z

pkg/agent/cnf/podwatch/k8s/podwatch_controller.go

+							//TBD after secondary network specific OVS init code merge.
+							klog.Errorf("VethPair not supported. Failed to configure interfaces for container %s: %v", containerInfo.ContainerId, err)
+						}
+					} //containerInfo loop


we don't typically include such comments in the Antrea codebase. If there is too much indentation, it would probably make sense to move that logic to its own function.

antoninbas · 2021-08-26T22:44:32Z

pkg/features/antrea_features.go

+		FlowExporter:              {Default: false, PreRelease: featuregate.Alpha},
+		NetworkPolicyStats:        {Default: true, PreRelease: featuregate.Beta},
+		NodePortLocal:             {Default: false, PreRelease: featuregate.Alpha},
+		SecondaryInterfaceSupport: {Default: false, PreRelease: featuregate.Alpha},


I would suggest SecondaryNetworks as the Feature Gate name
In any case "Support" is not needed, it's implied for Feature Gates

How about SecondaryInterfaces? I just feel it is more about creating secondary interfaces, and we not necessarily handle networks.

jianjuns · 2021-08-26T22:23:37Z

pkg/agent/cnf/cnipodcache/cni_info_cache.go

+}
+
+//Add cniPodInfo to local cache store.
+func (cni *cniPodInfoCache) AddCniConfigInfo(cniConfig *CNIConfigInfo) {


Cni -> CNI. We use all upper case for abbrs.

Please change all occurrences.

pkg/agent/cnf/podwatch/k8s/podwatch_controller.go

jianjuns · 2021-08-28T01:15:04Z

cmd/antrea-agent/agent.go

+		}
+
+	} else {
+		err = cniServer.Initialize(ovsBridgeClient, ofClient, ifaceStore, entityUpdates)


I think we can reuse the same Initialize() func, but based on whether the cniPodInfoStore argument is nil, the func can know it needs to initialize CNI cache or not.

introduce another argument (and passing it nil for non secondary interface flow) impact significant of cniserver specific existing test code (also calls these Initialize function). To avoid additional changes, I spilt it separate function with cache.

Do not get why that is a problem. You can just pass false/nil for existing callers inc. test code. Why it is significant impact?

jianjuns · 2021-08-28T01:17:10Z

cmd/antrea-agent/agent.go

-	if err != nil {
-		return fmt.Errorf("error initializing CNI server: %v", err)
+
+	cniPodInfoStore := cnipodcache.NewCniPodInfoStore()


Probably assign cniPodInfoStore to cnipodcache.NewCniPodInfoStore inside the "if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport)" block.

jianjuns · 2021-08-28T01:20:22Z

cmd/antrea-agent/agent.go

@@ -309,6 +322,30 @@ func run(o *Options) error {

 	go networkPolicyController.Run(stopCh)

+	if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport) {
+		//TBD: Initialize Secondary Network Infra here.


I would move this block to a separate func.

jianjuns · 2021-08-28T01:21:38Z

pkg/agent/cnf/cnipodcache/cni_info_cache.go

+}
+
+//Add cniPodInfo to local cache store.
+func (cni *cniPodInfoCache) AddCniConfigInfo(cniConfig *CNIConfigInfo) {


Please change all occurrences.

jianjuns · 2021-08-28T01:23:41Z

pkg/agent/cnf/cnipodcache/cni_info_cache.go

@@ -0,0 +1,80 @@
+// Copyright 2021 Antrea Authors


Agreed. Secondary interfaces might be used for other cases besides CNF.

How about secondaryinterfaces?

jianjuns · 2021-08-28T01:31:22Z

pkg/agent/cnf/podwatch/podwatch_controller_init.go

@@ -0,0 +1,59 @@
+// Copyright 2021 Antrea Authors


I feel no need to add a separate file, and the func can be in podwatch_controller.go. But no strong opinion.

jianjuns · 2021-08-28T01:33:57Z

pkg/agent/cniserver/server.go

@@ -105,7 +107,7 @@ type CNIServer struct {
 	hostProcPathPrefix   string
 	kubeClient           clientset.Interface
 	containerAccess      *containerAccessArbitrator
-	podConfigurator      *podConfigurator
+	PodConfigurator      *podConfigurator


To avoid touch too many code, could you add a func like GetPodConfigurator() to CNIServer instead?

jianjuns · 2021-08-28T01:36:40Z

pkg/agent/cniserver/server.go

@@ -461,6 +463,15 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (*
 	klog.Infof("CmdAdd for container %v succeeded", cniConfig.ContainerId)
 	// mark success as true to avoid rollback
 	success = true
+
+	if features.DefaultFeatureGate.Enabled(features.SecondaryInterfaceSupport) {


Could we add and pass a boolean like secInterfaceEnabled to CNIServer, instead of checking the feature gate inside the CNIServer code? I would avoid featuregate in all code.

jianjuns · 2021-08-28T01:37:51Z

pkg/agent/cniserver/server.go

+	cniPodInfoStore cnipodcache.CniPodInfoStore,
+) error {
+	var err error
+	s.PodConfigurator, err = newPodConfiguratorWithCniCache(


Again, could we just add cniPodInfoStore argument to newPodConfigurator() instead of adding new func with code duplication?

jianjuns · 2021-08-28T01:39:07Z

pkg/features/antrea_features.go

+		FlowExporter:              {Default: false, PreRelease: featuregate.Alpha},
+		NetworkPolicyStats:        {Default: true, PreRelease: featuregate.Beta},
+		NodePortLocal:             {Default: false, PreRelease: featuregate.Alpha},
+		SecondaryInterfaceSupport: {Default: false, PreRelease: featuregate.Alpha},


How about SecondaryInterfaces? I just feel it is more about creating secondary interfaces, and we not necessarily handle networks.

arunvelayutham · 2021-09-02T04:33:05Z

build/images/scripts/install_whereabouts_config

+WHEREABOUTS_KUBECONFIG_LITERAL=$(echo "$WHEREABOUTS_KUBECONFIG" | sed -e s'|/host||')
+
+# ------------------------------- Generate a "kube-config"
+SERVICE_ACCOUNT_PATH=/var/run/secrets/kubernetes.io/serviceaccount


Yet to take care on this concern and I fully agree with the suggestion. found new issue with whereabouts IP config and working on further to debug and address it.

arunvelayutham · 2021-09-02T04:35:21Z

build/yamls/antrea-aks.yml

@@ -3393,6 +3393,27 @@ rules:
  - get
  - watch
  - list
+- apiGroups:


make manifest (after updating base rbac file) updated all the antrea-*.yml file. But, not sure if we really wanted to enable secondarynetwork config on all the variants?. if not, I can remove these changes.

arunvelayutham · 2021-09-02T04:37:47Z

build/yamls/patches/netattachdef/net-attach-def.yml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1


Agreed. thanks for the suggestion. taken care.

arunvelayutham · 2021-09-02T04:39:05Z

build/yamls/patches/netattachdef/net-attach-def.yml

+  - '*'
+  verbs:
+  - '*'


yes, Taken care (I used "*" only for dev needs to ensure that initial agent able to fetch the net-attach-def CR info from APIserver).

arunvelayutham · 2021-09-02T04:41:41Z

cmd/antrea-agent/agent.go

+		}
+
+	} else {
+		err = cniServer.Initialize(ovsBridgeClient, ofClient, ifaceStore, entityUpdates)


introduce another argument (and passing it nil for non secondary interface flow) impact significant of cniserver specific existing test code (also calls these Initialize function). To avoid additional changes, I spilt it separate function with cache.

arunvelayutham · 2021-09-02T04:43:28Z

cmd/antrea-agent/agent.go

+			informerFactory,
+			nodeConfig.Name,
+			cniPodInfoStore,
+			cniServer)


yes certainly. I'm using it as temporary solution for this PR and I will take care of this as part of CNIserver pkg refactor PR.

arunvelayutham · 2021-09-02T04:45:04Z

pkg/agent/cniserver/server.go

@@ -461,6 +468,15 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (*
 	klog.Infof("CmdAdd for container %v succeeded", cniConfig.ContainerId)
 	// mark success as true to avoid rollback
 	success = true
+
+	if s.secNetworkEnabled {


took care as suggested by @jianjuns (do not use feature gate all over the code. used bool variable to decide whether we need to cache the cni info or not)

arunvelayutham · 2021-09-02T04:52:52Z

pkg/agent/secondarynetwork/podwatch/podwatch_controller_init.go

@@ -0,0 +1,59 @@
+// Copyright 2021 Antrea Authors


@jianjuns separate file for init was introduced based on secondary network infra (OVS) init needs. I will revisit on this file needs while pulling in the OVS bridge init changes in.

Then it should not be called "podwatch_controller_init.go", but maybe just "init.go" under "/secondarynetwork"? For now, I still suggest to move the code to "controller.go".

arunvelayutham · 2021-09-02T04:54:26Z

pkg/features/antrea_features.go

+		FlowExporter:              {Default: false, PreRelease: featuregate.Alpha},
+		NetworkPolicyStats:        {Default: true, PreRelease: featuregate.Beta},
+		NodePortLocal:             {Default: false, PreRelease: featuregate.Alpha},
+		SecondaryNetwork:          {Default: true, PreRelease: featuregate.Alpha},


renamed as SecondaryNetwork (more relevant to state it as network instead of interface, in my opinion - as we do configure secondary network to address CNF use cases with this feature gate)

I think this should be SecondaryNetworks...

jianjuns · 2021-09-08T23:58:20Z

cmd/antrea-agent/agent.go

@@ -309,6 +322,30 @@ func run(o *Options) error {

 	go networkPolicyController.Run(stopCh)

+	if features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) {
+		// TBD: Initialize Secondary Network Infra here.


Again, suggest to add a func for this block.

jianjuns · 2021-09-09T00:02:31Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+		return nil
+	}
+
+	//get POD/Container information (container id, namespace, etc.,) from CNI info cache (added by CNI Server CmdAdd upon


Add a space after "//".

jianjuns · 2021-09-09T00:03:08Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+	}
+
+	//get POD/Container information (container id, namespace, etc.,) from CNI info cache (added by CNI Server CmdAdd upon
+	//successful pod and primary network creation).


jianjuns · 2021-09-09T00:05:34Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+	var ret bool
+	pod := obj.(*corev1.Pod)
+
+	secondaryNetwork, ret := checkForPodSecondaryNetworkAttachement(pod)


In case, you missed the previous comments:

You can check the old version of the Pod obj to know if the CNF annotation got removed from the Pod.
Check: https://github.com/antrea-io/antrea/blob/main/pkg/agent/nodeportlocal/k8s/npl_controller.go#L211

jianjuns · 2021-09-11T00:02:00Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+		networklist, err := parsePodSecondaryNetworkAnnotation(secondaryNetwork)
+		if err != nil {
+			return errors.New("Secondary network annotation parsing failed")
+		} else {


Nit: you can remove the "else {}".

jianjuns · 2021-09-11T00:03:50Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+			for _, netinfo := range networklist {
+				klog.InfoS("Secondary Network Information:", netinfo)
+				if len(netinfo.NetworkName) > 0 {
+					netDefCRD, err := podWatcher.netAttachDefClient.NetworkAttachmentDefinitions(pod.Namespace).Get(context.TODO(), netinfo.NetworkName, metav1.GetOptions{})


We do not support changes of NetworkAttachmentDefinition, or NetworkAttachmentDefinition created after Pods?

jianjuns · 2021-09-11T00:10:00Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+					for _, containerInfo := range podCNIInfo {
+						cmdArgs = &invoke.Args{Command: string("ADD"), ContainerID: containerInfo.ContainerId,
+							NetNS: containerInfo.ContainerNetNS, IfName: containerInfo.ContainerIfDev,
+							Path: string("/opt/cni/bin/")}


Define a constant for this path.

jianjuns · 2021-09-11T00:10:30Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+							podSriovVFDeviceID, err := cniserver.GetPodContainerDeviceIDs(pod.Name, pod.Namespace)
+							if err != nil {
+								klog.ErrorS(err, "handleAddUpdatePod: GetPodContainerDeviceIDs failed for ", pod.Name, pod.Namespace)
+								//return nil here. we do not want to retry/requeue work if VF id fetch fails (user to check if VFs are created at node).


Add a space after //.

node -> Node

jianjuns · 2021-09-11T00:13:40Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+
+						ipamResult, err = ipam.GetIPAMGlobalAddress(cniconfig, cmdArgs)
+						if err != nil {
+							return errors.New("Secondary network global IPAM failed")


Question - how Multus handles such errors. Does it report the failure back to users via some ways (e.g. annotation, events, etc.)?

jianjuns · 2021-09-16T17:10:58Z

pkg/agent/secondarynetwork/ipam/ipam_delegator.go

+)
+
+const (
+	ipamGlobal     = "whereabouts"


Could we remove "Global" from (variable, func) names? It will be confusing, as we will support IPAM in Antrea too (see here: #2740).

If you think that is better, we can call it ipamWhereabouts.

@annakhm @gran-vmv

annakhm · 2021-09-16T20:12:42Z

pkg/agent/secondarynetwork/ipam/ipam_delegator.go

+	RawExec: &invoke.RawExec{Stderr: os.Stderr},
+}
+
+func delegateCommon(delegatePlugin string, exec invoke.Exec, cniPath string) (string, invoke.Exec, error) {


Looks like there is a fair amount of code duplication with /agent/cniserver/ipam/ipam_delegator.go, is there an option to reuse the code?

apologies for the delayed reply on this comment (though I communicate with Jianjun/Antonin over Slack on the inputs for their comments).
The reason I didn't reuse the existing implementation for ipam was due to,

Antonin and Jianjun suggested to keep the secondary interface changes as isolated as feasible.

during initial development, I thought we will have more to be done w.r.t whereabouts CNI parsing the retrieved IPAM context from networkAttachementDefinition. But, that wasn't the case when I finalized the implementation.

Indeed, I see that we would be able to reuse the IPAM package with changes/modifications to the existing implementation. If this is fine, I can get it addressed asap.

Yeah, from what I saw a few funcs here are exactly the same as those in cniserver/ipam, so probably we should reuse them.

So, you plan to refactor this part and share code with /agent/cniserver/ipam/ipam_delegator.go in a later PR?

yes @jianjuns , @annakhm I will do this as separate PR. Need to ensure that the existing IPAM code refactor will not impact primary interface IPAM use case, while it will be used for both the needs (host-local and whereabouts).

arunvelayutham · 2021-09-22T02:49:56Z

build/images/scripts/install_whereabouts_config

+WHEREABOUTS_KUBECONFIG_LITERAL=$(echo "$WHEREABOUTS_KUBECONFIG" | sed -e s'|/host||')
+
+# ------------------------------- Generate a "kube-config"
+SERVICE_ACCOUNT_PATH=/var/run/secrets/whereabouts


whereabouts restricted to use separate serviceaccount/secrets.

jianjuns · 2021-09-23T18:36:08Z

pkg/agent/secondarynetwork/cnipodcache/cache.go

+}
+
+// Add CNIPodInfo to local cache store.
+func (pod *CNIPodInfoCache) AddCNIConfigInfo(CNIConfig *CNIConfigInfo) {


But why you use "pod" for short name of CNIPodInfoCache, but not "cache" or just "c"?

CNIConfig -> cniConfig (as it is not a global variable).

jianjuns · 2021-09-23T18:39:25Z

pkg/agent/secondarynetwork/cnipodcache/types.go

+package cnipodcache
+
+type CNIConfigInfo struct {
+	CniVersion     string


CniVersion -> CNIVersion

We use all upper cases for abbrs.

jianjuns · 2021-09-23T18:39:36Z

pkg/agent/secondarynetwork/cnipodcache/types.go

+	CniVersion     string
+	PodName        string
+	PodNameSpace   string
+	ContainerId    string


ContainerId -> ContainerID.

jianjuns · 2021-09-23T18:42:39Z

pkg/agent/secondarynetwork/ipam/ipam_delegator.go

+	RawExec: &invoke.RawExec{Stderr: os.Stderr},
+}
+
+func delegateCommon(delegatePlugin string, exec invoke.Exec, cniPath string) (string, invoke.Exec, error) {


jianjuns · 2021-09-23T18:44:49Z

pkg/agent/secondarynetwork/podwatch/k8s/podwatch_controller.go

+	netDev                        = "eth1"
+)
+
+type PodWatchController struct {


I remember Antonin suggested to rename it to PodController.

And in my mind, we should put this file under /podwatch directly (remove /k8s), and rename it to "controller.go".

jianjuns · 2021-09-23T18:46:32Z

pkg/agent/secondarynetwork/podwatch/podwatch_controller_init.go

@@ -0,0 +1,59 @@
+// Copyright 2021 Antrea Authors


Then it should not be called "podwatch_controller_init.go", but maybe just "init.go" under "/secondarynetwork"? For now, I still suggest to move the code to "controller.go".

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/server.go

cmd/antrea-agent/agent.go

pkg/features/antrea_features.go

jianjuns · 2021-09-29T00:04:35Z

pkg/agent/secondarynetwork/podwatch/types.go

+	Type       string     `json:"type,omitempty"`
+	IPAM       IPAMConfig `json:"ipam,omitempty"`
+	// Options to be passed in by the runtime.
+	//RuntimeConfig RuntimeConfig `json:"runtimeConfig"`


Would you just remove it?

I will do this clean-up as part of another PR which does veth pair and OVS init handle

Sure, but why not remove these commented lines in this PR?

pkg/agent/secondarynetwork/podwatch/types.go

pkg/agent/secondarynetwork/podwatch/controller.go

antoninbas · 2021-09-29T00:07:39Z

build/yamls/patches/netattachdef/net-attach-def.yml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1


I think this file needs to be removed? Comments have been addressed and it doesn't seem to be used anywhere anymore.

antoninbas · 2021-09-29T00:11:40Z

build/yamls/base/kustomization.yml

+- whereabouts-rbac.yml
 - agent.yml
+- whereabouts-secret.yml


I thought we discussed that there should be a flag for generate-manifest.sh, that would take care of applying all the yaml patches (except changes to agent-rbac.yml). Wasn't that the case @jianjuns?

Initially I had a flag defined for whereabouts (as part of rbac changes). But, since I got inputs to put them directly on the agent-rbac file, I thought we are doing the same for other changes as well.
Is that ok, If I do it in separate PR right after this one?.

antoninbas · 2021-09-29T00:12:44Z

cmd/antrea-agent/agent.go

@@ -309,6 +321,29 @@ func run(o *Options) error {

 	go networkPolicyController.Run(stopCh)

+	if features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) {
+


unnecessary empty line

antoninbas · 2021-09-29T00:13:05Z

cmd/antrea-agent/agent.go

+		// Create NetworkAttachmentDefinitions client handle to access secondary network object definition from API Server.
+		netAttachDefClient, err := k8s.CreateNetworkAttachDefClient(o.config.ClientConnection, o.config.KubeAPIServerOverride)
+		if err != nil {
+			return fmt.Errorf("networkAttachmentDefinition creation failed. %v", err)


Suggested change

return fmt.Errorf("networkAttachmentDefinition creation failed. %v", err)

return fmt.Errorf("networkAttachmentDefinition client creation failed: %v", err)

antoninbas · 2021-09-29T00:15:08Z

cmd/antrea-agent/agent.go

@@ -309,6 +321,29 @@ func run(o *Options) error {

 	go networkPolicyController.Run(stopCh)

+	if features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) {
+
+		// Create NetworkAttachmentDefinitions client handle to access secondary network object definition from API Server.


Suggested change

// Create NetworkAttachmentDefinitions client handle to access secondary network object definition from API Server.

// Create the NetworkAttachmentDefinitions client which handles access to secondary network object definitions from the API Server.

antoninbas · 2021-09-29T00:34:25Z

pkg/agent/secondarynetwork/podwatch/controller.go

+        podSriovVFDeviceID, err := cniserver.GetPodContainerDeviceIDs(pod.Name, pod.Namespace)
+        if err != nil {
+               klog.ErrorS(err, "handleAddUpdatePod: GetPodContainerDeviceIDs failed for ", pod.Name, pod.Namespace)
+               // return nil here. we do not want to retry/requeue work if VF id fetch fails (user to check if VFs are created at node).


s/node/Node
I'm not sure I see the harm in requeueing here. Yes, we will keep logging the error periodically, but maybe it's not bad. And if VFs are eventually created, then we will be able to recover. Otherwise, the used as to delete the Pod and recreate it, or something similar.

antoninbas · 2021-09-29T00:35:00Z

pkg/agent/secondarynetwork/podwatch/controller.go

+
+               ipamResult, err = ipam.GetIPAMSubnetAddress(cniconfig, cmdArgs)
+               if err != nil {
+                      return errors.New("Secondary network global IPAM failed")


nit: we do not capitalize error messages like this one

antoninbas · 2021-09-29T00:35:12Z

pkg/agent/secondarynetwork/podwatch/controller.go

+               for _, ip := range result.IPs {
+                      ip.Interface = current.Int(1)
+               }
+               // If secondary interface name not provided in pod annotation, assign default name.


antoninbas · 2021-09-29T00:35:29Z

pkg/agent/secondarynetwork/podwatch/controller.go

+               if len(netinfo.NetworkName) > 0 {
+                        netDefCRD, err := pc.netAttachDefClient.NetworkAttachmentDefinitions(pod.Namespace).Get(context.TODO(), netinfo.NetworkName, metav1.GetOptions{})
+                        if err != nil {
+                               klog.Errorf("NetworkAttachmentDefinitions for %s returns err: %s", netinfo.NetworkName, err)


use structured logging

antoninbas · 2021-09-29T00:35:35Z

pkg/agent/secondarynetwork/podwatch/controller.go

+                        }
+                        cniconfig, err := netdefutils.GetCNIConfig(netDefCRD, "")
+                        if err != nil {
+                               klog.Errorf("net-attach-def: CNI config spec read error %v", err)


cmd/antrea-agent/agent.go

pkg/agent/secondarynetwork/cnipodcache/types.go

pkg/agent/secondarynetwork/podwatch/controller.go

antoninbas · 2021-12-24T00:30:55Z

pkg/agent/secondarynetwork/podwatch/controller.go

+)
+
+const (
+	controllerName = "AntreaAgentPodController"


one more comment: not a big fan of this name, it doesn't imply that this is specific to secondary networks

"SecondaryNetworkController"?

antoninbas · 2021-12-24T00:32:52Z

@arunvelayutham Our jenkins CI jobs are currently broken (at leasts some of them are). However, I see that you have some Github CI jobs currently failing for this PR.

arunvelayutham · 2021-12-26T20:45:31Z

/test-all

arunvelayutham · 2022-01-05T22:38:43Z

/test-all

arunvelayutham · 2022-01-05T23:41:14Z

/test-all

arunvelayutham · 2022-01-05T23:51:25Z

/test-all

arunvelayutham · 2022-01-06T19:38:50Z

/test-all

arunvelayutham · 2022-01-07T00:45:50Z

/test-all-features-conformance

arunvelayutham · 2022-01-07T00:54:18Z

/test-conformance

arunvelayutham · 2022-01-11T20:03:45Z

/test-all

arunvelayutham · 2022-01-12T01:00:10Z

/test-all

Signed-off-by: arunvelayutham <arunkumar.velayutham@intel.com>

arunvelayutham · 2022-01-18T18:02:27Z

/test-all

arunvelayutham · 2022-01-18T19:25:19Z

/test-all

arunvelayutham · 2022-01-18T21:46:36Z

/test-all-features-conformance

jianjuns · 2022-01-18T23:53:00Z

/test-integration

jianjuns · 2022-01-19T02:06:39Z

/test-all

jianjuns · 2022-01-19T02:10:36Z

/test-integration

jianjuns · 2022-01-19T05:57:29Z

/test-e2e

tnqn · 2022-01-19T14:43:08Z

/test-e2e

Implement SR-IOV secondary network for Antrea CNI configured Pods This commit implements SR-IOV secondary network that creates Pod secondary network interfaces with SR-IOV VFs. At this moment, the implementation supports only baremetal Nodes, not SR-IOV interfaces on VM Nodes. Signed-off-by: arunvelayutham <arunkumar.velayutham@intel.com>

arunvelayutham marked this pull request as draft August 26, 2021 07:00

arunvelayutham commented Aug 26, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from c977d6f to ec21933 Compare August 26, 2021 07:57

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from ec21933 to 42f0692 Compare August 26, 2021 20:48

antoninbas reviewed Aug 26, 2021

View reviewed changes

jianjuns reviewed Aug 26, 2021

View reviewed changes

arunvelayutham marked this pull request as ready for review August 26, 2021 23:07

jianjuns reviewed Aug 28, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch 2 times, most recently from c0b4929 to 658c301 Compare September 2, 2021 04:48

arunvelayutham commented Sep 2, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 658c301 to a9e2b0a Compare September 8, 2021 05:28

jianjuns reviewed Sep 9, 2021

View reviewed changes

jianjuns reviewed Sep 11, 2021

View reviewed changes

jianjuns reviewed Sep 16, 2021

View reviewed changes

annakhm reviewed Sep 16, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch 2 times, most recently from 49f43cb to 9b0c0a0 Compare September 22, 2021 02:38

arunvelayutham commented Sep 22, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 9b0c0a0 to 81f4b7b Compare September 22, 2021 20:45

jianjuns reviewed Sep 23, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 81f4b7b to 54b9c12 Compare September 24, 2021 06:58

jianjuns reviewed Sep 29, 2021

View reviewed changes

pkg/agent/secondarynetwork/podwatch/controller.go Outdated Show resolved Hide resolved

pkg/agent/secondarynetwork/podwatch/controller.go Outdated Show resolved Hide resolved

pkg/agent/secondarynetwork/podwatch/controller.go Outdated Show resolved Hide resolved

antoninbas reviewed Sep 29, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 54b9c12 to ee696e3 Compare October 1, 2021 01:29

jianjuns reviewed Oct 1, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch 2 times, most recently from b075a3a to e9853b7 Compare October 1, 2021 16:07

antoninbas reviewed Dec 24, 2021

View reviewed changes

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch 2 times, most recently from c766b26 to 042049f Compare December 26, 2021 20:43

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 042049f to e9e90f8 Compare December 26, 2021 20:48

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from e9e90f8 to aae7453 Compare January 5, 2022 23:30

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from aae7453 to f3a8068 Compare January 5, 2022 23:49

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from f3a8068 to 67ef0b8 Compare January 6, 2022 19:29

Intel SRIOV as a Secondary Network Interface Implementation

7e0a14e

Signed-off-by: arunvelayutham <arunkumar.velayutham@intel.com>

arunvelayutham force-pushed the antrea-secondary-network-sriov-support branch from 67ef0b8 to 7e0a14e Compare January 18, 2022 18:11

jianjuns approved these changes Jan 19, 2022

View reviewed changes

jianjuns merged commit f9dba58 into antrea-io:main Jan 19, 2022


		set -u -e

		# Inspired by: https://github.com/intel/multus-cni/blob/83556f49bd6706a885eda847210b542669279cd0/images/entrypoint.sh#L161-L222

		@@ -309,6 +321,29 @@ func run(o *Options) error {

		go networkPolicyController.Run(stopCh)

		if features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) {

	return fmt.Errorf("networkAttachmentDefinition creation failed. %v", err)
	return fmt.Errorf("networkAttachmentDefinition client creation failed: %v", err)

	// Create NetworkAttachmentDefinitions client handle to access secondary network object definition from API Server.
	// Create the NetworkAttachmentDefinitions client which handles access to secondary network object definitions from the API Server.

Antrea secondary network implementation for SRIOV #2651

Antrea secondary network implementation for SRIOV #2651

Conversation

arunvelayutham commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 26, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 26, 2021 •

edited

Loading