Promote feature gate NodePortLocal to GA #5491

hjiajing · 2023-09-16T08:36:14Z

No description provided.

test/e2e/nodeportlocal_test.go

build/charts/antrea/conf/antrea-agent.conf

cmd/antrea-agent/agent.go

luolanzone

LGTM, two nits

docs/node-port-local.md

cmd/antrea-agent/agent.go

antoninbas · 2023-09-21T18:28:44Z

@luolanzone was there consensus to promote this FeatureGate to GA? I can't find a reference to it in the meeting minutes: https://github.com/antrea-io/antrea/wiki/Community-Meetings#august-14-2023

luolanzone · 2023-09-25T01:08:21Z

@antoninbas it's not tracked by the community meeting. I found this feature might be able to promoted to GA after the community meeting ,so I started a slack thread to discuss it, and the team discussed this in a weekly sync meeting. You probably missed the sync meeting?

luolanzone · 2023-09-25T01:08:45Z

@hjiajing please help to resolve the code conflicts.

hjiajing · 2023-09-25T01:43:20Z

@luolanzone Thanks for the reminder, rebased.

luolanzone · 2023-09-25T03:31:11Z

@hjiajing please check the unit test failure, not sure if it's related with a fix here https://github.com/antrea-io/antrea/pull/5401/files#diff-81ab3cbfb811ae739c26bb47a543eace8d915f2d518aa9d711d89ae47f54f30eR162-R167.

luolanzone

Two nits.

docs/feature-gates.md

cmd/antrea-agent/agent.go

hjiajing · 2023-10-12T06:20:51Z

/test-e2e

luolanzone

LGTM

antoninbas

This PR should be updated to match #5401

#5401 has gone through several rounds of reviews, and while it is not finalized yet (see #5401 (comment)), we seem to have reach consensus on the approach. In particular, FeatureGate validation and deprecation warning should probably go into cmd/antrea-agent/options.go, for consistency.

antoninbas · 2023-10-20T20:30:46Z

cmd/antrea-agent/agent.go

+	enableNodePortLocal := o.config.NodePortLocal.Enable
+	if !features.DefaultFeatureGate.Enabled(features.NodePortLocal) {
+		enableNodePortLocal = false
+		klog.InfoS("Feature gate `enableNodePortLocal` is deprecated, please use option `nodePortLocal.enable` to disable NodePortLocal")


The name of the FeatureGate is NodePortLocal, not enableNodePortLocal

Move the feature check block to cmd/antrea-agent/options.go now. A new function validateNodePortLocalConfg is added to check the NodePortLocal configuration.

hjiajing · 2023-10-24T08:33:01Z

/test-e2e

cmd/antrea-agent/options.go

antoninbas · 2023-10-24T19:34:19Z

cmd/antrea-agent/options.go

@@ -738,3 +730,19 @@ func (o *Options) validateSecondaryNetworkConfig() error {

 	return nil
 }
+
+func (o *Options) valdateNodePortLocalConfig() error {
+	if !features.DefaultFeatureGate.Enabled(features.NodePortLocal) {


I think you should define o.enableNodePortLocal = *o.config.NodePortLocal.Enable && features.DefaultFeatureGate.Enabled(features.NodePortLocal), instead of mutating o.config.NodePortLocal.Enable. See #5401

Done, Add a new variable o.enableNodePortLocal

luolanzone · 2023-10-25T09:14:55Z

build/yamls/antrea.yml

@@ -5691,8 +5691,7 @@ data:

    nodePortLocal:
    # Enable NodePortLocal, a feature used to make Pods reachable using port forwarding on the host. To
-    # enable this feature, you need to set "enable" to true, and ensure that the NodePortLocal feature
-    # gate is also enabled (which is the default).
+    # enable this feature, you need to set "enable" to true.


Since it's GA now, I think the default should be true? @tnqn @antoninbas

Change the default value to true now.

Not really, we introduced the toggle intentionally to avoid unnecessary overhead added by the module as NPL would only be useful when users are using particular external LoadBalancers, which doesn't seem the majority case. Otherwise the feature gate alone can achieve what we need.

I took a look at the current code, even though the allocation of the node port is only triggered by the existence of the annotation, the code still needs to do some jobs to figure out the expectation first, which indicates some costs. And there could be some confusing logs generated as far as I can tell, for example, the following will be logged unconditionally as long as a Pod is added/updated even though there is no Service selecting it.

klog.V(2).Infof("Pod %s is selected by a Service for which NodePortLocal is enabled", key)

At the moment, I still prefer to keep it disabled by default and let users enable it on demand. If one day the feature becomes more common or its execution cost (when user doesn't annoate any Service) is proved to be trival, it will make sense to me to enable it by default.

Thanks for @tnqn 's comments. I set the default value to false now.

build/charts/antrea/conf/antrea-agent.conf

tnqn

Conflicts need to be resolved

tnqn · 2023-10-25T10:31:33Z

cmd/antrea-agent/options.go

@@ -93,6 +93,10 @@ type Options struct {
 	// enableEgress represents whether Egress should run or not, calculated from its feature gate configuration and
 	// whether the traffic mode supports it.
 	enableEgress bool
+	// enableNodePortLocal indicates whether NodePortLocal should be enabled or not, based on feature gate NodePortLocal
+	// and options NodePortLocal.Enable. This used to maintain compatibility with the NodePortLocal feature gate, which


Suggested change

// and options NodePortLocal.Enable. This used to maintain compatibility with the NodePortLocal feature gate, which

// and options NodePortLocal.Enable. This is used to maintain compatibility with the NodePortLocal feature gate, which

Added "is" after "This".

tnqn · 2023-10-25T10:38:18Z

docs/node-port-local.md

@@ -31,6 +31,8 @@ NodePortLocal was introduced in v0.13 as an alpha feature, and was graduated to
 beta in v1.4, at which time it was enabled by default. Prior to v1.4, a feature
 gate, `NodePortLocal`, must be enabled on the antrea-agent for the feature to
 work. Starting from Antrea v1.7, NPL is supported on the Windows antrea-agent.
+From Antrea v1.14, NPL is GA. To enable this feature, you need to ensure that the
+`nodePortLocal.enable` flag is set to true in Antrea Agent configuration.


It seems duplicate with the following section. It has mentioned nodePortLocal.enable, perhaps just say it's GA here?

Agree, removed the "To enable this" sentence.

tnqn · 2023-10-25T14:17:21Z

Conflicts need to be resolved

Reminder, the PR is not mergable even it's approved.

hjiajing · 2023-10-25T14:38:06Z

Conflicts need to be resolved

Reminder, the PR is not mergable even it's approved.

Thanks for the reminder, conflicts resolved.

tnqn

LGTM

antoninbas

LGTM

antoninbas · 2023-10-25T18:32:47Z

/test-all

antoninbas · 2023-10-25T18:33:35Z

@hjiajing the change is causing unit test failures, PTAL

Signed-off-by: hujiajing <hjiajing@vmware.com>

hjiajing · 2023-10-26T01:54:04Z

@hjiajing the change is causing unit test failures, PTAL

@antoninbas Thanks for the reminder, resovled.

hjiajing · 2023-10-26T01:54:37Z

/test-all

tnqn

LGTM

hjiajing requested review from tnqn and luolanzone September 16, 2023 08:36

hjiajing force-pushed the npl branch from 50d1541 to 1a37144 Compare September 16, 2023 10:32

luolanzone reviewed Sep 18, 2023

View reviewed changes

test/e2e/nodeportlocal_test.go Outdated Show resolved Hide resolved

build/charts/antrea/conf/antrea-agent.conf Show resolved Hide resolved

luolanzone reviewed Sep 18, 2023

View reviewed changes

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

luolanzone added this to the Antrea v1.14 release milestone Sep 18, 2023

luolanzone added the action/release-note Indicates a PR that should be included in release notes. label Sep 18, 2023

hjiajing force-pushed the npl branch from 1a37144 to a37cb8e Compare September 20, 2023 02:04

luolanzone reviewed Sep 20, 2023

View reviewed changes

docs/node-port-local.md Outdated Show resolved Hide resolved

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

luolanzone requested a review from antoninbas September 21, 2023 02:32

hjiajing force-pushed the npl branch from a37cb8e to 2fc0598 Compare September 22, 2023 01:16

hjiajing force-pushed the npl branch from 2fc0598 to b01a1d0 Compare September 25, 2023 01:42

luolanzone reviewed Oct 10, 2023

View reviewed changes

docs/feature-gates.md Outdated Show resolved Hide resolved

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

hjiajing force-pushed the npl branch 2 times, most recently from fa1a8ea to e2c9bc4 Compare October 12, 2023 06:20

luolanzone previously approved these changes Oct 16, 2023

View reviewed changes

antoninbas reviewed Oct 20, 2023

View reviewed changes

hjiajing dismissed luolanzone’s stale review via 147e45a October 24, 2023 08:26

hjiajing force-pushed the npl branch 2 times, most recently from 147e45a to e3296cd Compare October 24, 2023 08:31

antoninbas reviewed Oct 24, 2023

View reviewed changes

hjiajing force-pushed the npl branch from e3296cd to b711c38 Compare October 25, 2023 01:53

luolanzone reviewed Oct 25, 2023

View reviewed changes

build/charts/antrea/conf/antrea-agent.conf Show resolved Hide resolved

hjiajing force-pushed the npl branch 2 times, most recently from b5d1326 to 9420f6e Compare October 25, 2023 09:49

tnqn reviewed Oct 25, 2023

View reviewed changes

hjiajing force-pushed the npl branch from 9420f6e to 4cbbe01 Compare October 25, 2023 12:13

hjiajing force-pushed the npl branch 2 times, most recently from 9d001ec to c161e1c Compare October 25, 2023 14:37

tnqn previously approved these changes Oct 25, 2023

View reviewed changes

antoninbas previously approved these changes Oct 25, 2023

View reviewed changes

Promote feature gate NodePortLocal to GA

27de96a

Signed-off-by: hujiajing <hjiajing@vmware.com>

hjiajing dismissed stale reviews from antoninbas and tnqn via 27de96a October 26, 2023 01:53

hjiajing force-pushed the npl branch from c161e1c to 27de96a Compare October 26, 2023 01:53

tnqn approved these changes Oct 26, 2023

View reviewed changes

tnqn merged commit d66de44 into antrea-io:main Oct 26, 2023
43 of 44 checks passed

luolanzone mentioned this pull request Oct 31, 2023

[Antrea 2.0] Promote APIs to appropriate versions and feature gates to appropriate stages #5068

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote feature gate NodePortLocal to GA #5491

Promote feature gate NodePortLocal to GA #5491

hjiajing commented Sep 16, 2023

luolanzone left a comment

antoninbas commented Sep 21, 2023

luolanzone commented Sep 25, 2023

luolanzone commented Sep 25, 2023

hjiajing commented Sep 25, 2023

luolanzone commented Sep 25, 2023

luolanzone left a comment

hjiajing commented Oct 12, 2023

luolanzone left a comment

antoninbas left a comment

antoninbas Oct 20, 2023

hjiajing Oct 24, 2023

hjiajing commented Oct 24, 2023

antoninbas Oct 24, 2023

hjiajing Oct 25, 2023

luolanzone Oct 25, 2023

hjiajing Oct 25, 2023

tnqn Oct 25, 2023

hjiajing Oct 25, 2023

tnqn left a comment

tnqn Oct 25, 2023

hjiajing Oct 25, 2023

tnqn Oct 25, 2023

hjiajing Oct 25, 2023

tnqn commented Oct 25, 2023

hjiajing commented Oct 25, 2023

tnqn left a comment

antoninbas left a comment

antoninbas commented Oct 25, 2023

antoninbas commented Oct 25, 2023

hjiajing commented Oct 26, 2023 •

edited

Loading

hjiajing commented Oct 26, 2023

tnqn left a comment

	// and options NodePortLocal.Enable. This used to maintain compatibility with the NodePortLocal feature gate, which
	// and options NodePortLocal.Enable. This is used to maintain compatibility with the NodePortLocal feature gate, which

Promote feature gate NodePortLocal to GA #5491

Promote feature gate NodePortLocal to GA #5491

Conversation

hjiajing commented Sep 16, 2023

luolanzone left a comment

Choose a reason for hiding this comment

antoninbas commented Sep 21, 2023

luolanzone commented Sep 25, 2023

luolanzone commented Sep 25, 2023

hjiajing commented Sep 25, 2023

luolanzone commented Sep 25, 2023

luolanzone left a comment

Choose a reason for hiding this comment

hjiajing commented Oct 12, 2023

luolanzone left a comment

Choose a reason for hiding this comment

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

hjiajing commented Oct 24, 2023

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

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 commented Oct 25, 2023

hjiajing commented Oct 25, 2023

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Oct 25, 2023

antoninbas commented Oct 25, 2023

hjiajing commented Oct 26, 2023 • edited Loading

hjiajing commented Oct 26, 2023

tnqn left a comment

Choose a reason for hiding this comment

hjiajing commented Oct 26, 2023 •

edited

Loading