New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for migration to NLB from CLB #381

Draft

virajrk wants to merge 27 commits into istio-ecosystem:master from virajrk:NLBMigration2

Collaborator

virajrk commented Jan 30, 2025

Adding support for migration to NLB from CLB.
Support this phase by phase.

virajrk force-pushed the NLBMigration2 branch from bcd51ec to 8ae166d Compare

January 30, 2025 00:45

codecov-commenter commented Jan 30, 2025 •

edited

Loading

Codecov Report

Attention: Patch coverage is 78.57143% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.13%. Comparing base (08133eb) to head (bfd426d).

Files with missing lines	Patch %	Lines
admiral/pkg/clusters/admiralDatabaseClient.go	71.05%	11 Missing ⚠️
admiral/pkg/clusters/types.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   77.08%   77.13%   +0.04%     
==========================================
  Files          75       75              
  Lines       10523    10567      +44     
==========================================
+ Hits         8112     8151      +39     
- Misses       1989     1996       +7     
+ Partials      422      420       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vinay-g requested changes

View reviewed changes

admiral/cmd/admiral/cmd/root.go Outdated

@@ @@ -281,6 +281,7 @@ func GetRootCmd(args []string) *cobra.Command { @@
               	rootCmd.PersistentFlags().StringSliceVar(&params.NLBEnabledClusters, "nlb_enabled_clusters", []string{}, "Comma seperated list of enabled clusters to be enabled for NLB")
               	rootCmd.PersistentFlags().StringSliceVar(&params.NLBEnabledIdentityList, "nlb_enabled_identity_list", []string{}, "Comma seperated list of enabled idenity list to be enabled for NLB")
               	rootCmd.PersistentFlags().StringSliceVar(&params.CLBEnabledClusters, "clb_enabled_clusters", []string{}, "Comma seperated list of enabled clusters to be enabled for CLB")
+              	rootCmd.PersistentFlags().StringVar(&params.NLBIngressLabel, "nlb_ingress_label", common.NLBIstioIngressGatewayLabelValue, "The the value of the `app` label to use to match and find the service that represents the NLB ingress for cross cluster traffic")

Collaborator

vinay-g Jan 31, 2025

nitpick - 2 the

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

               func (dynamicConfigDatabaseClient *DynamicConfigDatabaseClient) Get(env, identity string) (interface{}, error) {
               	//Variable renaming is done to re-purpose existing interface
+              	err := checkIfDynamicConfigDatabaseClientIsInitialize(dynamicConfigDatabaseClient)

Collaborator

vinay-g Jan 31, 2025

nitpick: checkIfDynamicConfigDatabaseClientIsInitialized

admiral/pkg/clusters/serviceentry.go Show resolved Hide resolved

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              		for _, fetchService := range rr.remoteControllers[cluster].ServiceController.Cache.Get(common.NamespaceIstioSystem) {
+              			if fetchService.Labels[common.App] == lbLabel {
+              				log.Infof("task=%s, Processing LB migration for Cluster : %s", common.LBUpdateProcessor, cluster)
+              				go handleServiceEventForDeployment(ctx, fetchService, rr, cluster, rr.GetRemoteController(cluster).DeploymentController, rr.GetRemoteController(cluster).ServiceController, HandleEventForDeployment)

Collaborator

vinay-g Jan 31, 2025

would this cause CPU issues if many clusters are added/removed ?

Collaborator Author

virajrk Feb 3, 2025

Yes. Slightly uptick. Mainly idea is -

Migrate few clusters NLB
More clusters to NLB (Optional)
Migrate all clusters to NLB. In this case will flip label.

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go

               	return &dynamicConfigClient, nil
               }
-              func UpdateASyncAdmiralConfig(dbClient AdmiralDatabaseManager, syncTime int) {
+              func UpdateASyncAdmiralConfig(rr *RemoteRegistry, syncTime int) {

Collaborator

shriramsharma Feb 4, 2025

this func is running in a goroutine but we are not gracefully exiting it. Can we pass in the ctx and return gracefully when the program exits?

admiral/admiral/pkg/clusters/admiralReadWriteLeaseStateChecker.go

Line 67 in 3d33a99

case <-ctx.Done():

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              	var clusersToProcess []string
+              	if cache == nil || len(*cache) == 0 {
+              		*cache = updatedLB
+              		clusersToProcess = updatedLB

Collaborator

shriramsharma Feb 4, 2025

nitpick, this variable assignment is not needed. we can just return updatedLB

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              		*cache = updatedLB
+              		clusersToProcess = updatedLB
+              		return clusersToProcess
+              	} else {

Collaborator

shriramsharma Feb 4, 2025

nitpick, we can remove this else

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go

+              	}
+              }
+              func getLBToProcess(updatedLB []string, cache *[]string) []string {

Collaborator

shriramsharma Feb 4, 2025

I think I'm unable to follow this.
If NLB Cluster param has c1, c2 and NLB Cache has c1, c2, c3. This func returns c3
Why do we need to process c3 with NLBIngressLabel?
Sorry if I'm misunderstanding

Collaborator Author

virajrk Feb 5, 2025

This method little generic. It gets invoked for CLB as well as NLB.
For example if we remove cluster from NLB and add it CLB then this will take care.

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              	log.Infof("task=%s, Processing LB migration for %s. UpdateReceived=%s, ExistingCache=%s, ", common.LBUpdateProcessor, lbLabel, updatedLBs, existingCache)
+              	for _, cluster := range getLBToProcess(updatedLBs, existingCache) {
+              		for _, fetchService := range rr.remoteControllers[cluster].ServiceController.Cache.Get(common.NamespaceIstioSystem) {

Collaborator

shriramsharma Feb 4, 2025

can we add some nil checks to avoid panics?

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              		//Validate if New ClusterAdded
+              		for _, clusterFromAdmiralParam := range updatedLB {
+              			if !slices.Contains(*cache, clusterFromAdmiralParam) {
+              				clusersToProcess = append(clusersToProcess, clusterFromAdmiralParam)

Collaborator

shriramsharma Feb 4, 2025

we should add the new cluster in the cache, or is it being added somewhere else?

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

+              		//Validate if cluster Removed
+              		for _, clusterFromCache := range *cache {
+              			if !slices.Contains(updatedLB, clusterFromCache) {
+              				clusersToProcess = append(clusersToProcess, clusterFromCache)

Collaborator

shriramsharma Feb 4, 2025

same, we may need to remove it from the cache

Viraj Kulkarni added 20 commits

February 4, 2025 16:37


          DB Connected.

4421a65

Signed-off-by: Viraj Kulkarni <[email protected]>


          First parameter overwrite set

984ef11

Signed-off-by: Viraj Kulkarni <[email protected]>


          Refactoring

8f3cc2a

Signed-off-by: Viraj Kulkarni <[email protected]>


          Refactoring and some tests

138f10d

Signed-off-by: Viraj Kulkarni <[email protected]>


          Add changes to check if DB config changes.

ff2cff8

Signed-off-by: Viraj Kulkarni <[email protected]>


          clean up

4b82859

Signed-off-by: Viraj Kulkarni <[email protected]>


          clean up & some unit test

dbfd974

Signed-off-by: Viraj Kulkarni <[email protected]>


          clean up & some unit test

870da6e

Signed-off-by: Viraj Kulkarni <[email protected]>


          dynamo db test

5d49957

Signed-off-by: Viraj Kulkarni <[email protected]>


          unit test cover update scenario

f7d10c9

Signed-off-by: Viraj Kulkarni <[email protected]>


          unit test cover DB fetch scenario

17ccaed

Signed-off-by: Viraj Kulkarni <[email protected]>


          review part 1

bd74026

Signed-off-by: Viraj Kulkarni <[email protected]>


          review part 2

7acfee7

Signed-off-by: Viraj Kulkarni <[email protected]>


          review part 3

7c649e6

Signed-off-by: Viraj Kulkarni <[email protected]>


          review part 4

80f0465

Signed-off-by: Viraj Kulkarni <[email protected]>


          rebase

03cece8

Signed-off-by: Viraj Kulkarni <[email protected]>


          nlb migration eod

2c6dcd9

Signed-off-by: Viraj Kulkarni <[email protected]>


          refactoring and cleaning

74269e1

Signed-off-by: Viraj Kulkarni <[email protected]>


          refactoring and cleaning & unit testing

565e90c

Signed-off-by: Viraj Kulkarni <[email protected]>


          refactoring unit test

Signed-off-by: Viraj Kulkarni <[email protected]>

Viraj Kulkarni added 4 commits

February 4, 2025 16:41


          more unit test

e5e9150

Signed-off-by: Viraj Kulkarni <[email protected]>


          log clean up

eee44f3

Signed-off-by: Viraj Kulkarni <[email protected]>


          fixing rebase

348215b

Signed-off-by: Viraj Kulkarni <[email protected]>


          code review

4260c49

virajrk force-pushed the NLBMigration2 branch from bfd426d to 4260c49 Compare

February 5, 2025 00:41

Viraj Kulkarni added 3 commits

February 4, 2025 16:54


          refactor + cherry pick

61e4113


          code review

3ebc247


          code review

3c3ec6f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet