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

Merged

virajrk merged 29 commits into istio-ecosystem:master from virajrk:NLBMigration2

Feb 11, 2025

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 72.15190% with 22 lines in your changes missing coverage. Please review.

Project coverage is 77.04%. Comparing base (faabd12) to head (6a0ad67).
Report is 3 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
- Coverage   77.06%   77.04%   -0.02%     
==========================================
  Files          75       75              
  Lines       10541    10596      +55     
==========================================
+ Hits         8123     8164      +41     
- Misses       1995     2011      +16     
+ Partials      423      421       -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 Outdated

               	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

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

February 5, 2025 00:41

shriramsharma reviewed

View reviewed changes

admiral/pkg/clusters/admiralDatabaseClient.go Outdated

-              	for range time.Tick(time.Minute * time.Duration(syncTime)) {
-              		ReadAndUpdateSyncAdmiralConfig(dbClient)
+              	ctxDynamicConfig, cancel := context.WithCancel(context.Background())

Collaborator

shriramsharma Feb 10, 2025

nitpick, can you pass the ctx as a param from where this func is getting called

shriramsharma approved these changes

View reviewed changes

Viraj Kulkarni added 17 commits

February 10, 2025 16:21


          DB Connected.

13d374e

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


          First parameter overwrite set

bdb55f3

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


          Refactoring

eaa7e3f

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


          Refactoring and some tests

f753f30

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


          Add changes to check if DB config changes.

fd4dcdc

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


          clean up

004bec4

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


          clean up & some unit test

1f2d811

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


          clean up & some unit test

83e0cb2

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


          dynamo db test

a14b0aa

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


          unit test cover update scenario

4a1b06f

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


          unit test cover DB fetch scenario

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


          review part 1

f5b2745

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


          review part 2

14e3632

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


          review part 3

b80d931

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


          review part 4

afbad5b

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


          rebase

0a0ca5c

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


          nlb migration eod

fc26351

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

Viraj Kulkarni added 12 commits

February 10, 2025 16:21


          refactoring and cleaning

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


          refactoring and cleaning & unit testing

9c7b709

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


          refactoring unit test

4a7aa5f

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


          more unit test

68b867b

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


          log clean up

19a5904

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


          fixing rebase

cc2687c

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


          code review

1ff84cd

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


          refactor + cherry pick

0635d24

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


          code review

6b317cb

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


          code review

eb9c4de

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


          code review

6e7233b

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


          code review

6a0ad67

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

virajrk force-pushed the NLBMigration2 branch from 3a3dc9e to 6a0ad67 Compare

February 11, 2025 00:22

shriramsharma approved these changes

View reviewed changes

vinay-g approved these changes

View reviewed changes

virajrk marked this pull request as ready for review

February 11, 2025 17:31

virajrk merged commit 94daadf into istio-ecosystem:master

3 checks passed

virajrk deleted the NLBMigration2 branch

February 11, 2025 17:32

virajrk restored the NLBMigration2 branch

February 11, 2025 17:33

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

Labels

None yet