cache listeners on routing stage for later use #4599

nnnkkk7 · 2023-09-10T08:31:17Z

What this PR does / why we need it:

Cache AWS LoadBalancers Listeners on routing stage

Which issue(s) this PR fixes:

Fixes #4474

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

codecov · 2023-09-11T00:00:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a64f250) 30.82% compared to head (4e86b6e) 30.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4599      +/-   ##
==========================================
+ Coverage   30.82%   30.85%   +0.02%     
==========================================
  Files         221      221              
  Lines       25947    25947              
==========================================
+ Hits         7999     8005       +6     
+ Misses      17297    17291       -6     
  Partials      651      651

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

khanhtc1202 · 2023-09-13T09:07:11Z

pkg/app/piped/executor/ecs/ecs.go

+	for i, v := range currListenerArns {
+		metadataListeners[fmt.Sprintf(currentListenersKey+"-%d", i)] = v
+	}
+	if err := in.MetadataStore.Shared().PutMulti(ctx, metadataListeners); err != nil {


This only stores the listeners to metadata store, I think we have to add logic for query that data from store before fetch using API (client) at L419 👀

Thanks!
I added it. get listeners from cache

ffjlabo

Thank you so much for your contribution! 🥇
It's totally OK! I just commented on one nit point!

ffjlabo · 2023-11-01T05:55:37Z

pkg/app/piped/executor/ecs/ecs.go

+	// Store created listeners to use later.
+	addedListeners := make([]string, 0, len(currListenerArns))
+	addedListeners = append(addedListeners, currListenerArns...)
+	metadata := strings.Join(addedListeners, ",")
+	if err := in.MetadataStore.Shared().Put(ctx, currentListenersKey, metadata); err != nil {
+		in.LogPersister.Errorf("Unable to store created listeners to metadata store: %v", err)
 		return false
 	}


[nits]
Are these values of addedListeners the same as currListenerArns ?
If so, it might be better just to use currListenerArns to store as metadata 👀

Thank you! I fixed it. use currListenerArns instead of addedListeners

khanhtc1202 · 2023-11-02T07:46:22Z

/review

github-actions · 2023-11-02T07:46:51Z

PR Analysis

Main theme

type: Enhancement
description: The PR introduces enhancements to the ecs.go file in the piped/executor/ecs package.

PR summary

type: string
description: The PR enhances the code in ecs.go to store and retrieve traffic routing configuration and active listener information in the metadata store.

Type of PR

Refactoring

PR Feedback:

General suggestions

type: string
description: |-
The code changes look good overall. However, there are a couple of suggestions for improvement:

Consider using more descriptive variable names instead of abbreviations to improve code readability.
Ensure proper error handling and return appropriate error messages instead of returning false.

Code feedback

type: array
uniqueItems: true
items:
relevant file:
type: string
description: "pkg/app/piped/executor/ecs/ecs.go"
suggestion:
type: string
description: |-
Instead of using abbreviations, use more descriptive variable names. This improves code readability.
For example, replace the following:

  ```diff
  const (
  -	trafficRoutePrimaryMetadataKey = "primary-percentage"
  -	trafficRouteCanaryMetadataKey  = "canary-percentage"
  -	canaryScaleMetadataKey         = "canary-scale"
  +	primaryPercentageMetadataKey = "primary-percentage"
  +	canaryPercentageMetadataKey  = "canary-percentage"
  +	canaryScaleMetadataKey       = "canary-scale"
  )
  ```
  
  Additionally, ensure proper error handling and return appropriate error messages instead of returning `false`. This helps in debugging and improves resiliency. For example:
  
  ```diff
  if err != nil {
      in.LogPersister.Errorf("Failed to get current active listeners: %v", err)
  +    return false, fmt.Errorf("failed to get current active listeners: %w", err)
  }
  ```

relevant line:
type: string
description: |-
+ trafficRoutePrimaryMetadataKey = "primary-percentage"
+ trafficRouteCanaryMetadataKey = "canary-percentage"
+ canaryScaleMetadataKey = "canary-scale"

  +	in.LogPersister.Errorf("Failed to get current active listeners: %v", err)
  +	return false
  
  +	primaryPercentageMetadataKey = "primary-percentage"
  +	canaryPercentageMetadataKey  = "canary-percentage"
  +	canaryScaleMetadataKey       = "canary-scale"
  
  +	return false, fmt.Errorf("failed to get current active listeners: %w", err)

Security concerns:

type: string
description: "no"

ffjlabo

Thank you for your contribution 🚀

Signed-off-by: nnnkkk7 <[email protected]>

khanhtc1202 · 2023-11-28T10:15:41Z

Let's me hold this PR until PR #4688 merged 🙏

khanhtc1202

🚀 🚀 🚀

* cache current listeners for later use Signed-off-by: nnnkkk7 <[email protected]> * get listeners from cache Signed-off-by: nnnkkk7 <[email protected]> * use currListenerArns instead of addedListeners Signed-off-by: nnnkkk7 <[email protected]> --------- Signed-off-by: nnnkkk7 <[email protected]>

* cache current listeners for later use Signed-off-by: nnnkkk7 <[email protected]> * get listeners from cache Signed-off-by: nnnkkk7 <[email protected]> * use currListenerArns instead of addedListeners Signed-off-by: nnnkkk7 <[email protected]> --------- Signed-off-by: nnnkkk7 <[email protected]> Signed-off-by: sZma5a <[email protected]>

* cache current listeners for later use Signed-off-by: nnnkkk7 <[email protected]> * get listeners from cache Signed-off-by: nnnkkk7 <[email protected]> * use currListenerArns instead of addedListeners Signed-off-by: nnnkkk7 <[email protected]> --------- Signed-off-by: nnnkkk7 <[email protected]> Signed-off-by: 鈴木優耀 <[email protected]>

nnnkkk7 requested review from kentakozuka, khanhtc1202 and nghialv as code owners September 10, 2023 08:31

github-actions bot added the area/go label Sep 10, 2023

nnnkkk7 changed the title ~~cache current listeners for later use~~ cache listeners on routing stage for later use Sep 10, 2023

khanhtc1202 reviewed Sep 13, 2023

View reviewed changes

nnnkkk7 force-pushed the add-cache-for-aws-lb-listeners branch from 44882e2 to 730d6e9 Compare September 16, 2023 11:23

nnnkkk7 requested a review from khanhtc1202 September 16, 2023 11:24

khanhtc1202 requested a review from ffjlabo as a code owner November 1, 2023 04:19

ffjlabo reviewed Nov 1, 2023

View reviewed changes

nnnkkk7 force-pushed the add-cache-for-aws-lb-listeners branch from 2e77e94 to 87433db Compare November 19, 2023 03:55

nnnkkk7 requested a review from ffjlabo November 19, 2023 03:57

ffjlabo approved these changes Nov 20, 2023

View reviewed changes

ffjlabo force-pushed the add-cache-for-aws-lb-listeners branch 2 times, most recently from 3ef3dad to 366b0ba Compare November 24, 2023 06:07

nnnkkk7 added 3 commits November 27, 2023 19:04

cache current listeners for later use

34d14ac

Signed-off-by: nnnkkk7 <[email protected]>

get listeners from cache

0087a13

Signed-off-by: nnnkkk7 <[email protected]>

use currListenerArns instead of addedListeners

4e86b6e

Signed-off-by: nnnkkk7 <[email protected]>

ffjlabo force-pushed the add-cache-for-aws-lb-listeners branch from 366b0ba to 4e86b6e Compare November 27, 2023 10:04

khanhtc1202 added the do-not-merge/hold label Nov 28, 2023

khanhtc1202 removed the do-not-merge/hold label Dec 14, 2023

khanhtc1202 approved these changes Dec 14, 2023

View reviewed changes

khanhtc1202 merged commit b4003d7 into pipe-cd:master Dec 14, 2023

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cache listeners on routing stage for later use #4599

cache listeners on routing stage for later use #4599

Uh oh!

nnnkkk7 commented Sep 10, 2023

Uh oh!

codecov bot commented Sep 11, 2023 •

edited

Loading

Uh oh!

khanhtc1202 Sep 13, 2023

Uh oh!

nnnkkk7 Sep 16, 2023

Uh oh!

ffjlabo left a comment

Uh oh!

ffjlabo Nov 1, 2023

Uh oh!

nnnkkk7 Nov 19, 2023

Uh oh!

khanhtc1202 commented Nov 2, 2023

Uh oh!

github-actions bot commented Nov 2, 2023

Uh oh!

ffjlabo left a comment

Uh oh!

khanhtc1202 commented Nov 28, 2023

Uh oh!

khanhtc1202 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cache listeners on routing stage for later use #4599

cache listeners on routing stage for later use #4599

Uh oh!

Conversation

nnnkkk7 commented Sep 10, 2023

Uh oh!

codecov bot commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

khanhtc1202 Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

nnnkkk7 Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

ffjlabo left a comment

Choose a reason for hiding this comment

Uh oh!

ffjlabo Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

nnnkkk7 Nov 19, 2023

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 commented Nov 2, 2023

Uh oh!

github-actions bot commented Nov 2, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

Security concerns:

Uh oh!

ffjlabo left a comment

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 commented Nov 28, 2023

Uh oh!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 11, 2023 •

edited

Loading