Create additional NEGs based on subnets in Node Topology CR. #2702

sawsa307 · 2024-10-18T20:18:05Z

Query Node Topology CR for the current set of NEGs in the cluster.
When ensureNetworkEndpointGroups(), ensure NEGs are properly provisioned in the non-default subnets as well.

sawsa307 · 2024-10-22T15:55:32Z

k8s-ci-robot · 2024-10-23T19:47:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sawsa307
Once this PR has been reviewed and has the lgtm label, please ask for approval from gauravkghildiyal. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/neg/types/types_test.go

pkg/utils/zonegetter/zone_getter.go

pkg/neg/syncers/transaction.go

pkg/neg/syncers/utils.go

pkg/neg/syncers/transaction.go

sawsa307 · 2024-10-31T22:38:00Z

/label tide/merge-method-squash

sawsa307

Just addressed all the comments, @gauravkghildiyal could you take another look when you have time? Thank you!

pkg/neg/types/types_test.go

pkg/utils/zonegetter/zone_getter.go

pkg/neg/syncers/transaction.go

pkg/neg/types/types_test.go

pkg/neg/syncers/transaction.go

pkg/utils/zonegetter/zone_getter.go

gauravkghildiyal · 2024-11-05T01:14:59Z

Also, is this TODO something that we plan on fixing independently? I guess as long as the MSC flags are disabled, this shouldn't cause any problems but please do confirm. Thanks!

sawsa307 · 2024-11-05T21:01:39Z

Also, is this TODO something that we plan on fixing independently? I guess as long as the MSC flags are disabled, this shouldn't cause any problems but please do confirm. Thanks!

Yes I'm planning to tackle those TODO in follow up PRs(like #2728), but if you would like, I can also include some of them in this PR as well.

pkg/utils/zonegetter/zone_getter.go

gauravkghildiyal · 2024-11-08T02:39:07Z

pkg/utils/zonegetter/zone_getter.go

+	nodeTopologyNotSynced := z.nodeTopologyInformer != nil && !z.nodeTopologyHasSynced()
+	if z.onlyIncludeDefaultSubnetNodes || z.nodeTopologyInformer == nil || nodeTopologyNotSynced {
+		if z.nodeTopologyInformer == nil || nodeTopologyNotSynced {
+			logger.Info("NodeTopologyInformer is not ready, fall back to use default subnet only")


I'd recommend

logger.Info("Falling back to only using default subnet", "z.onlyIncludeDefaultSubnetNodes", z.onlyIncludeDefaultSubnetNodes, <Rest of the variables influencing this decision>)

Updated, thanks!

It may be useful to know what was the reason why we are using "default subnet only", which means it's worth logging ALL the variables which went into that decision. So maybe we don't have an if !z.nodeTopologyHasSynced() { condition for the logs, and instead log:

logger.Info("Falling back to only using default subnet when listing nodes", "z.onlyIncludeDefaultSubnetNodes", z.onlyIncludeDefaultSubnetNodes, "z.nodeTopologyHasSynced()", nodeTopologyHasSynced)

On a related note, it plan on using or logging z.nodeTopologyHasSynced(), it may be useful to store it in a variable before. Reason being that since it's a function, it may return a different value between two independent invocations -- if that ever happens for some reason, our logs would simply end up confusing us.

* Remove NodeTopologyInformer from controller context hasSynced so it won't block controllers from starting up in case of the CRD or CR does not exist.

* Query Node Topology CR for the current set of NEGs in the cluster. * When ensureNetworkEndpointGroups(), ensure NEGs are properly provisioned in the non-default subnets as well.

pkg/utils/zonegetter/zone_getter.go

pkg/neg/syncers/transaction.go

swetharepakula · 2024-11-16T02:38:08Z

pkg/neg/syncers/transaction.go

+	namer   negtypes.NetworkEndpointGroupNamer
+	l4Namer namer.L4ResourcesNamer


do we need both. the namer should be L4 / L7 agnostic. The right namer should be set on transactionSyncer creation

We need both since they have different interface and naming schema.
For L4 NEGs, the NEG name is determined by L4Backend(), while NEG() is only for L7 NEGs. As a result, I also separated the naming schema for L4 and L7 non-default subnet NEGs as L4NonDefaultSubnetNEG and NonDefaultSubnetNEG.

Interestingly, because of this complication of having two separate L4 and L7 namers, even the existing code has a bug at:

ingress-gce/pkg/neg/manager.go

Line 246 in 169b2c0

!manager.namer.IsNEG(portInfo.NegName),

where this value determines the value of customName bool. The way our new logic is, this should lead to customName always being true for L4 NEGs and thus it would end up using the L7 custom namer for L4 NEGs.

Do we want to make L4 and L7 namers implement the following?

NonDefaultSubnetNEG(namespace, name, subnetName string, port int32) string NonDefaultSubnetCustomNEG(customNEGName, subnetName string) (string, error)

The port value will not be used for L4

Because we will set customNEG for L4 to always be false, the L4 NonDefaultSubnetCustomNEG should also never be used.

pkg/neg/syncers/transaction.go

pkg/utils/zonegetter/zone_getter.go

sawsa307 · 2024-11-19T01:04:57Z

@gauravkghildiyal @swetharepakula This PR is ready for another round of review, please take a look, thank you!

gauravkghildiyal · 2024-11-19T04:04:12Z

Github is just the worst at showing newly added comments so sharing the links:

sawsa307 · 2024-11-19T21:21:26Z

Github is just the worst at showing newly added comments so sharing the links:

Create additional NEGs based on subnets in Node Topology CR. #2702 (comment)

Create additional NEGs based on subnets in Node Topology CR. #2702 (comment)

Addressed both comments, please take a look! @gauravkghildiyal

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2024

k8s-ci-robot requested review from aojea and panslava October 18, 2024 20:18

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 18, 2024

sawsa307 marked this pull request as draft October 18, 2024 20:18

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024

sawsa307 force-pushed the create-non-default-neg branch from 2f593bb to 29bc49a Compare October 22, 2024 06:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 22, 2024

sawsa307 marked this pull request as ready for review October 22, 2024 15:53

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024

k8s-ci-robot assigned gauravkghildiyal Oct 22, 2024

sawsa307 force-pushed the create-non-default-neg branch from 29bc49a to 8b82474 Compare October 23, 2024 19:47

sawsa307 force-pushed the create-non-default-neg branch 2 times, most recently from 644e229 to 264bd6c Compare October 23, 2024 20:44

gauravkghildiyal reviewed Oct 23, 2024

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 31, 2024

sawsa307 commented Oct 31, 2024

View reviewed changes

gauravkghildiyal reviewed Nov 5, 2024

View reviewed changes

pkg/neg/types/types_test.go Outdated Show resolved Hide resolved

pkg/neg/syncers/transaction.go Outdated Show resolved Hide resolved

pkg/neg/syncers/transaction.go Outdated Show resolved Hide resolved

pkg/utils/zonegetter/zone_getter.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 5, 2024

sawsa307 force-pushed the create-non-default-neg branch 2 times, most recently from 4bc66aa to 6d44fe7 Compare November 7, 2024 03:50

gauravkghildiyal reviewed Nov 8, 2024

View reviewed changes

Update namer interface to include NonDefaultSubnetNEG

460f7d6

sawsa307 added 4 commits November 14, 2024 17:06

Exclude NodeTopologyInformer from hasSynced.

fd001e7

* Remove NodeTopologyInformer from controller context hasSynced so it won't block controllers from starting up in case of the CRD or CR does not exist.

Add NodeTopologyInformer to zoneGetter.

be3c372

Fall back to only include default if nodeTopology CR isn't ready.

6e42596

Create additional NEGs based on subnets in Node Topology CR.

055d691

* Query Node Topology CR for the current set of NEGs in the cluster. * When ensureNetworkEndpointGroups(), ensure NEGs are properly provisioned in the non-default subnets as well.

sawsa307 force-pushed the create-non-default-neg branch from 6d44fe7 to 055d691 Compare November 14, 2024 18:10

Use correct namer based on the NEG type.

927b120

swetharepakula reviewed Nov 16, 2024

View reviewed changes

pkg/utils/zonegetter/zone_getter.go Outdated Show resolved Hide resolved

sawsa307 added 3 commits November 18, 2024 22:49

fixup! Fall back to only include default if nodeTopology CR isn't ready.

2e9a180

fixup! Create additional NEGs based on subnets in Node Topology CR.

037a763

fixup! Use correct namer based on the NEG type.

cd6e627

sawsa307 requested review from gauravkghildiyal and swetharepakula November 19, 2024 01:04

sawsa307 added 2 commits November 19, 2024 21:12

fixup! Use correct namer based on the NEG type.

7724945

fixup! Fall back to only include default if nodeTopology CR isn't ready.

20d4578

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create additional NEGs based on subnets in Node Topology CR. #2702

Create additional NEGs based on subnets in Node Topology CR. #2702

sawsa307 commented Oct 18, 2024

sawsa307 commented Oct 22, 2024

k8s-ci-robot commented Oct 23, 2024

sawsa307 commented Oct 31, 2024

sawsa307 left a comment

gauravkghildiyal commented Nov 5, 2024

sawsa307 commented Nov 5, 2024

gauravkghildiyal Nov 8, 2024

sawsa307 Nov 19, 2024

gauravkghildiyal Nov 19, 2024

swetharepakula Nov 16, 2024

sawsa307 Nov 19, 2024

gauravkghildiyal Nov 19, 2024

sawsa307 commented Nov 19, 2024

gauravkghildiyal commented Nov 19, 2024

sawsa307 commented Nov 19, 2024

		namer negtypes.NetworkEndpointGroupNamer
		l4Namer namer.L4ResourcesNamer

Create additional NEGs based on subnets in Node Topology CR. #2702

Are you sure you want to change the base?

Create additional NEGs based on subnets in Node Topology CR. #2702

Conversation

sawsa307 commented Oct 18, 2024

sawsa307 commented Oct 22, 2024

k8s-ci-robot commented Oct 23, 2024

sawsa307 commented Oct 31, 2024

sawsa307 left a comment

Choose a reason for hiding this comment

gauravkghildiyal commented Nov 5, 2024

sawsa307 commented Nov 5, 2024

gauravkghildiyal Nov 8, 2024

Choose a reason for hiding this comment

sawsa307 Nov 19, 2024

Choose a reason for hiding this comment

gauravkghildiyal Nov 19, 2024

Choose a reason for hiding this comment

swetharepakula Nov 16, 2024

Choose a reason for hiding this comment

sawsa307 Nov 19, 2024

Choose a reason for hiding this comment

gauravkghildiyal Nov 19, 2024

Choose a reason for hiding this comment

sawsa307 commented Nov 19, 2024

gauravkghildiyal commented Nov 19, 2024

sawsa307 commented Nov 19, 2024