Skip to content
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

Enabled to configure the interval of livestate store for Lambda #5269

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Enabled to configure the interval of livestate store for Lambda #5269

merged 4 commits into from
Oct 9, 2024

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Oct 8, 2024

What this PR does / why we need it:

  • Enabled to configure the interval of get livestate of Lambda in piped-config.

What for:

  • To reduce the number of API calls (for cost etc.)
    • LiveStateStore periodically calls a lot of APIs to get Lambda resources.
    • The number of API calls depends on the number of Lambda functions.
    • When a user has a lot of Lambda functions, the cost of AWS CloudTrail or filtering it will go high.

Which issue(s) this PR fixes:

N/A

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):

@t-kikuc
add liveStateInterval for Lambda
36fbee8 
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc
add docs of liveStateInterval
b06209f 
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
khanhtc1202
khanhtc1202 reviewed Oct 8, 2024
View reviewed changes
pkg/config/piped.go Outdated
// The interval to fetch the live state of Lambda functions.
// Default is 15s.
// To reduce AWS API calls, this interval should be larger.
LiveStateInterval Duration `json:"liveStateInterval,omitempty" default:"15s"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to make this general name like awsAPICallInterval. Since we are going to migrate platform to plugin, I want to ensure less configuration update as possible in the mean time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think liveStateInterval will not change while migrating to the plugin.

Plugin will change the name of the LiveState feature??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean if we allow some name like this in the configuration, then we would have something like driftDetectionInterval later, which I think should not be here (in the piped.platformProvider config). Livestate or Drift detection config should not be related directly to platform, is what I tried to explain. Name like awsAPICallInterval is more relevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I want to ensure we do not add names/configs that will be changed too much later due to the migrating cost when the user moves to use the plugin-piped. That's why I said, I don't think the used name is good.

Copy link
Contributor

@Warashi Warashi Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost agree with @khanhtc1202 that we can name this configuration a generic word, but I think awsAPICallInterval is a bit confusing because it sounds like a rate-limiting feature.
We call AWS APIs in other functionality, such as deployment, and they are not affected by this configuration.
What about like awsAPIPollingInterval?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name awsAPIPollingInterval is lgtm 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khanhtc1202 @Warashi
Thank you, I changed to awsAPIPollingInterval

@t-kikuc
fix a test
b9b564e 
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@codecov Codecov
Copy link

codecov bot commented Oct 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 24.27%. Comparing base (3dbf164) to head (f6d05f2).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/piped/livestatestore/lambda/lambda.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5269      +/-   ##
==========================================
+ Coverage   24.08%   24.27%   +0.18%     
==========================================
  Files         441      440       -1     
  Lines       47099    46754     -345     
==========================================
+ Hits        11345    11348       +3     
+ Misses      34850    34502     -348     
  Partials      904      904

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

@t-kikuc
Rename to 'awsAPIPollingInterval'
f6d05f2 
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc added the v0.49.2 label Oct 9, 2024
khanhtc1202
khanhtc1202 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Warashi
Warashi approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@t-kikuc t-kikuc merged commit f38cd84 into master Oct 9, 2024
15 of 18 checks passed
@t-kikuc t-kikuc deleted the livestate-interval branch October 9, 2024 03:14
@github-actions github-actions bot mentioned this pull request Oct 9, 2024
Merged
github-actions bot pushed a commit that referenced this pull request Oct 9, 2024
@t-kikuc @pipecd-bot
Enabled to configure the interval of livestate store for Lambda (#5269)
086f001 
* add liveStateInterval for Lambda

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add docs of liveStateInterval

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* fix a test

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Rename to 'awsAPIPollingInterval'

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
@github-actions github-actions bot mentioned this pull request Oct 9, 2024
Merged
t-kikuc added a commit that referenced this pull request Oct 9, 2024
@github-actions @Warashi
@khanhtc1202 @t-kikuc
Cherry-pick #5266 #5268 #5269 #5272 (#5273)
eb98030 
* Fix the workflow publishes quickstart manifests (#5266)

* Add pull-requests permission to the workflow

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

* Set the base branch to the default branch of the repository

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

* Use `github.ref_name` to determine the version of manifests

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

* Checkout master branch to make PR to master branch

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

---------

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Remove pipectl quickstart command (#5268)

* Remove pipectl quickstart command

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Remove the quickstart from docs

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

---------

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Enabled to configure the interval of livestate store for Lambda (#5269)

* add liveStateInterval for Lambda

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add docs of liveStateInterval

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* fix a test

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Rename to 'awsAPIPollingInterval'

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Update RELEASE to v0.49.2 and sync docs of v0.49.x (#5272)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

---------

Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com>
Co-authored-by: Tetsuya Kikuchi <97105818+t-kikuc@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Nov 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khanhtc1202 khanhtc1202 khanhtc1202 approved these changes

@Warashi Warashi Warashi approved these changes

@nghialv nghialv Awaiting requested review from nghialv nghialv is a code owner

@kentakozuka kentakozuka Awaiting requested review from kentakozuka kentakozuka is a code owner

@ffjlabo ffjlabo Awaiting requested review from ffjlabo ffjlabo is a code owner

Assignees
No one assigned
Labels
area/go cherry-pick v0.49.2
Projects
No open projects
v0.49.2
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t-kikuc @Warashi @khanhtc1202