Check the existence of CRD AntreaAgentInfo before operating on it #4762

wenyingd · 2023-03-24T14:07:20Z

Antrea Controller checks the existence of AntreaAgentInfo API before creating or deleting the resources for worker Node or ExternalNode.

luolanzone · 2023-03-30T02:41:52Z

pkg/monitor/controller.go

@@ -365,3 +379,14 @@ func (monitor *controllerMonitor) deleteAgentCRD(name string) error {
 	}
 	return nil
 }
+
+func (monitor *controllerMonitor) AgentInfoCRDInstalled() bool {
+	_, err := monitor.apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), antreaAgentInfoCRDName, metav1.GetOptions{})


Why this check is needed? I suppose CRD should be always there? Will CRD be deleted in any case?

When antrea is running with agentless mode, CRD of antreaagentinfo may not create as it is not needed.

tnqn · 2023-04-13T07:25:07Z

pkg/monitor/controller.go

@@ -365,3 +379,14 @@ func (monitor *controllerMonitor) deleteAgentCRD(name string) error {
 	}
 	return nil
 }
+
+func (monitor *controllerMonitor) AgentInfoCRDInstalled() bool {


clientset.Interface has Discovery() which can be used to check if an API exists. We already use it to check EndpointSlice API. This is an example:

antrea/pkg/agent/proxy/proxier.go

Lines 1158 to 1173 in f1545c1

func endpointSliceAPIAvailable(k8sClient clientset.Interface) (bool, error) {

resources, err := k8sClient.Discovery().ServerResourcesForGroupVersion(discovery.SchemeGroupVersion.String())

if err != nil {

// The group version doesn't exist.

if errors.IsNotFound(err) {

return false, nil

}

return false, fmt.Errorf("error getting server resources for GroupVersion %s: %v", discovery.SchemeGroupVersion.String(), err)

}

for _, resource := range resources.APIResources {

if resource.Kind == "EndpointSlice" {

return true, nil

}

}

return false, nil

}

it should be a private method, and something like antreaAgentInfoAPIAvailable reflects the implementation more accurately.

tnqn · 2023-04-13T07:27:55Z

pkg/monitor/controller.go

+	if !c.AgentInfoCRDInstalled() {
+		return nil
+	}


I think we should just not run these goroutines instead of check the API's existence on every Node event?

Is it possible that at time 1 the antreaagentinfo CRD is not deployed, but at time2 the CRD is applied. If we only have a single check at the initial run, the case is not supported.

The code doesn't automatically call deleteStaleAgentCRDs and syncNode when the CRD is added after starting controller, right? Then it would just lead to an incomplete situation that some Nodes are handled while others are not, so a restart is needed anyway. Besides, there seems no reason to apply CRD after running controller? Could other features consuming CRD work correctly? But anyway I feel not worth to add repeated API calls to partially support a scenario that we don't have yet.

And I think the point of the PR is to avoid adding a configuration that controls whether to run the AntreaAgentInfo controller, by auto-discovering it. If the auto-discovery becomes more complicated and even double APIs call of normal mode, it may be not worth.

pkg/monitor/controller.go

tnqn · 2023-04-13T12:04:50Z

pkg/monitor/controller.go

@@ -365,3 +379,14 @@ func (monitor *controllerMonitor) deleteAgentCRD(name string) error {
 	}
 	return nil
 }
+
+func (monitor *controllerMonitor) AgentInfoCRDInstalled() bool {


it should be a private method, and something like antreaAgentInfoAPIAvailable reflects the implementation more accurately.

pkg/monitor/controller.go

tnqn · 2023-04-14T10:56:49Z

pkg/monitor/controller.go

+			klog.ErrorS(err, "error getting server resources for GroupVersion", "groupVersion", v1beta1.SchemeGroupVersion.String())
+			return false
+		}
+		return false


It should retry on non-NotFound error, which may mean the APIServer is temporarily unavailable or there is a networking issue. Otherwise it may return wrong result.

updated accordingly.

tnqn · 2023-04-14T11:01:03Z

pkg/monitor/controller.go

-		go wait.Until(monitor.nodeWorker, time.Second, stopCh)
-		if monitor.externalNodeEnabled {
-			go wait.Until(monitor.externalNodeWorker, time.Second, stopCh)
+	if monitor.antreaAgentInfoAPIAvailable() {


We should log the result for visibility and troubleshooting. For example:

if monitor.antreaAgentInfoAPIAvailable() { klog.InfoS("The AntreaAgentInfo API is available, running node workers") monitor.deleteStaleAgentCRDs() ... } else { klog.InfoS("The AntreaAgentInfo API is unavailable, will not run node workers") }

pkg/monitor/controller.go

tnqn · 2023-04-25T13:20:21Z

pkg/monitor/controller_test.go

@@ -624,3 +642,38 @@ func TestEnqueueExternalNode(t *testing.T) {
 	obj, _ := controller.controllerMonitor.externalNodeQueue.Get()
 	assert.Equal(t, expectedkey, obj.(string))
 }
+
+func TestAgentInfoCRDInstalled(t *testing.T) {


test name inconsistent with func name

pkg/monitor/controller_test.go

Signed-off-by: wenyingd <wenyingd@vmware.com>

tnqn

LGTM

tnqn · 2023-04-26T03:22:44Z

/skip-all

…trea-io#4762) Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd force-pushed the antreaas branch from daa9e52 to 53e1e01 Compare March 27, 2023 02:36

luolanzone reviewed Mar 30, 2023

View reviewed changes

tnqn reviewed Apr 13, 2023

View reviewed changes

wenyingd force-pushed the antreaas branch 3 times, most recently from 25470da to c9b1e6a Compare April 13, 2023 11:55

tnqn reviewed Apr 13, 2023

View reviewed changes

wenyingd force-pushed the antreaas branch from c9b1e6a to d6ca464 Compare April 14, 2023 02:08

tnqn reviewed Apr 14, 2023

View reviewed changes

pkg/monitor/controller.go Outdated Show resolved Hide resolved

wenyingd force-pushed the antreaas branch 2 times, most recently from 07922a2 to 73015ee Compare April 14, 2023 10:51

tnqn reviewed Apr 14, 2023

View reviewed changes

wenyingd force-pushed the antreaas branch from 73015ee to fe624f3 Compare April 14, 2023 14:52

tnqn reviewed Apr 17, 2023

View reviewed changes

pkg/monitor/controller.go Outdated Show resolved Hide resolved

pkg/monitor/controller.go Outdated Show resolved Hide resolved

wenyingd force-pushed the antreaas branch 2 times, most recently from 650d2a7 to b739e68 Compare April 17, 2023 15:31

wenyingd requested a review from tnqn April 24, 2023 05:33

tnqn reviewed Apr 25, 2023

View reviewed changes

Check the existence of CRD AntreaAgentInfo before operating on it

58aa235

Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd force-pushed the antreaas branch from b739e68 to 58aa235 Compare April 26, 2023 02:02

tnqn approved these changes Apr 26, 2023

View reviewed changes

tnqn merged commit 97b8e4a into antrea-io:main Apr 26, 2023

jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023

Check the existence of CRD AntreaAgentInfo before operating on it (an…

2a2d6cb

…trea-io#4762) Signed-off-by: wenyingd <wenyingd@vmware.com>

vicky-liu added this to the Antrea v1.12 release milestone May 4, 2023

wenyingd deleted the antreaas branch May 30, 2023 06:49

ceclinux pushed a commit to ceclinux/antrea that referenced this pull request Jun 5, 2023

Check the existence of CRD AntreaAgentInfo before operating on it (an…

c1b8e82

…trea-io#4762) Signed-off-by: wenyingd <wenyingd@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check the existence of CRD AntreaAgentInfo before operating on it #4762

Check the existence of CRD AntreaAgentInfo before operating on it #4762

wenyingd commented Mar 24, 2023 •

edited

Loading

luolanzone Mar 30, 2023

wenyingd Apr 3, 2023

tnqn Apr 13, 2023

wenyingd Apr 13, 2023

tnqn Apr 13, 2023

tnqn Apr 14, 2023

wenyingd Apr 14, 2023

tnqn Apr 13, 2023

wenyingd Apr 13, 2023

tnqn Apr 13, 2023

wenyingd Apr 13, 2023

tnqn Apr 13, 2023

tnqn Apr 14, 2023

wenyingd Apr 14, 2023

tnqn Apr 14, 2023

wenyingd Apr 14, 2023

tnqn Apr 25, 2023

wenyingd Apr 26, 2023

tnqn left a comment

tnqn commented Apr 26, 2023

	func endpointSliceAPIAvailable(k8sClient clientset.Interface) (bool, error) {
	resources, err := k8sClient.Discovery().ServerResourcesForGroupVersion(discovery.SchemeGroupVersion.String())
	if err != nil {
	// The group version doesn't exist.
	if errors.IsNotFound(err) {
	return false, nil
	}
	return false, fmt.Errorf("error getting server resources for GroupVersion %s: %v", discovery.SchemeGroupVersion.String(), err)
	}
	for _, resource := range resources.APIResources {
	if resource.Kind == "EndpointSlice" {
	return true, nil
	}
	}
	return false, nil
	}

Check the existence of CRD AntreaAgentInfo before operating on it #4762

Check the existence of CRD AntreaAgentInfo before operating on it #4762

Conversation

wenyingd commented Mar 24, 2023 • edited Loading

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Apr 26, 2023

wenyingd commented Mar 24, 2023 •

edited

Loading