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

Add a scaler based on number of objects in a GCP Storage bucket #2648

Merged
merged 17 commits into from
Mar 14, 2022
Merged

Add a scaler based on number of objects in a GCP Storage bucket #2648

merged 17 commits into from
Mar 14, 2022

Conversation

RamCohen
Copy link
Contributor

@RamCohen RamCohen commented Feb 16, 2022 •
edited by zroubalik
Loading

Add a scaler based on number of objects in a GCP Storage bucket

Fixes #2628

Signed-off-by: Ram Cohen ram.cohen@gmail.com

@RamCohen RamCohen requested a review from a team as a code owner February 16, 2022 11:08
@tomkerkhove
Copy link
Member

Would you mind opening a PR for the docs as well please?

zroubalik
zroubalik reviewed Feb 16, 2022
View reviewed changes
pkg/scalers/gcp_common.go Show resolved Hide resolved
pkg/scalers/gcp_storage_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/gcp_storage_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/gcp_storage_scaler.go Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Feb 16, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp*
Update: You can check the progres here

@zroubalik
Copy link
Member

@RamCohen That e2e with fake GCS is neat, though we have just get GCP account for KEDA related testing, so it might be great to rewrite the tests and point them against actual GCP storage? WDYT?

@RamCohen
Copy link
Contributor Author

Where can I see the credentials to use for the GCP account ?

@zroubalik
Copy link
Member

zroubalik commented Feb 16, 2022 •
edited
Loading

Where can I see the credentials to use for the GCP account ?

We usually have the credentials stored as GH Secrets and exposed via ENV variables, so the apporach would be to modify the test to expect those env variables. For example you can take a look on this PR for an inspiration: #2643

We haven't added the GCP credentials to GH yet, @tomkerkhove could you please assist with that? Thanks!

@tomkerkhove
Copy link
Member

I haven't used GCP before but if there are any docs on what I should follow then I can set it up.

@RamCohen
Copy link
Contributor Author

You need to create a service account in the 'IAM & Admin' panel, add a key to it by generating a new key and save the downloaded json file as the secret
You'll also need to add roles to that service account depending on what it should be allowed to do.
You can consider using separate accounts per scaler in order to have each one with just the roles needed for that scaler particular test

@RamCohen RamCohen mentioned this pull request Feb 20, 2022
Open
35 tasks
@RamCohen
Copy link
Contributor Author

See also instructions here: https://github.com/kedacore/sample-go-gcppubsub#readme

@tomkerkhove
Copy link
Member

You need to create a service account in the 'IAM & Admin' panel, add a key to it by generating a new key and save the downloaded json file as the secret You'll also need to add roles to that service account depending on what it should be allowed to do. You can consider using separate accounts per scaler in order to have each one with just the roles needed for that scaler particular test

Sure, but what permissions do you need? :) Is Monitoring Viewer enough?

tomkerkhove added a commit that referenced this pull request Feb 22, 2022
@tomkerkhove
feat: Provide e2e variables for GCP auth
edee249 
Relates to #2648

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
tomkerkhove added a commit that referenced this pull request Feb 22, 2022
@tomkerkhove
feat: Provide e2e variables for GCP auth
a022e91 
Relates to #2648

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
tomkerkhove added a commit that referenced this pull request Feb 22, 2022
@tomkerkhove
feat: Provide e2e variables for GCP auth
114f05f 
Relates to #2648

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
@tomkerkhove
Copy link
Member

Auth is added through #2662, but I did not add any other resources.

What would you need in GCP from us?

@RamCohen
Copy link
Contributor Author

RamCohen commented Feb 22, 2022 via email

@zroubalik
Copy link
Member

For pubsub e2e I need pubsub admin as I create and delete a topic on each test I we'll use the same topic which will be pre-created, pubsub editor should be enough The topic will need to have a message retention policy of 1 day to automatically clean up For GCS I need Storage admin as I create and delete a bucket each time If the test will use the same bucket we can have object admin permissions instead, but we'll need to make sure the bucket is empty before each test
…
On Tue, Feb 22, 2022 at 9:12 AM Tom Kerkhove @.> wrote: Auth is added through #2662 <#2662>, but I did not add any other resources. What would you need in GCP from us? — Reply to this email directly, view it on GitHub <#2648 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATJ7GUAENPIV74F6XVEJ6TU4MZOBANCNFSM5ORIVOZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.Message ID: @.>

Yeah, we should create all needed resources directly from the test via API. We should just point credentials and all should be done, @tomkerkhove we should add admin rights to the credentials.

zroubalik
zroubalik requested changes Feb 28, 2022
View reviewed changes
pkg/scalers/gcp_storage_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/gcp_storage_scaler.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

Also, could you please update Changelog?

@RamCohen
Copy link
Contributor Author

RamCohen commented Mar 1, 2022

I also need the GCP project ID

@zroubalik
Copy link
Member

Could you please rebase this PR to contain only relevant changes?

@zroubalik
Copy link
Member

I also need the GCP project ID

Do you need anything else than this? https://github.com/kedacore/keda/pull/2662/files

@RamCohen
Copy link
Contributor Author

RamCohen commented Mar 1, 2022

Yes, the GCP project ID

@tomkerkhove
Copy link
Member

Yes, the GCP project ID

That's cncf-keda-testing

@zroubalik
Copy link
Member

Yes, the GCP project ID

That's cncf-keda-testing

@tomkerkhove would be better to store this in GitHub as ENV as well.

@RamCohen
Copy link
Contributor Author

RamCohen commented Mar 1, 2022 via email

@zroubalik
Copy link
Member

zroubalik commented Mar 1, 2022 •
edited
Loading

The project id value can be extracted from the gcp key: JSON.parse(process.env['GCP_SP_KEY']).project_id

@RamCohen cool, so are you all set with this PR? If so, could you please resolve the conflict and we can run the test and do final reivew and merge. Thanks!

@tomkerkhove
Copy link
Member

Yes, the GCP project ID

That's cncf-keda-testing

@tomkerkhove Tom Kerkhove FTE would be better to store this in GitHub as ENV as well.

Yes, but not as a secret - No? It can be just a variable in the workflow.

RamCohen added 12 commits March 3, 2022 10:02
@RamCohen
Add a scaler based on number of GCP Storage bucket objects
3ace51f 
Fixes kedacore#2628

Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Added e2e test to gcp_storage scaler
241830f 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Fix static styling issues
b2617ff 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Update gcs-storage scaler test
1b0b636 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Review fixes
0c9d868 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Update google storage related packages
33618e1 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Precalculate metric name
de42e6a 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Updated changelog
46057da 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Remove option to specify an alternate metric name
ce54e9d 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Make e2e test work with the test GCP project
681f16c 
Also remove now unnecessary 'endpoint' property as we are now
using real GCS

Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Use methods from helpers utilities
f93b5cd 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Fix GCP authentication in test
8e434b2 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Mar 3, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp-storage.test.*
Update: You can check the progres here

@zroubalik
Copy link
Member

/run-e2e gcp-storage.test.* Update: You can check the progres here

The test fail with:

 ✔ creating the gcp-sdk pod should work.. (570ms)
error: unable to upgrade connection: container not found ("gcp-sdk-container")
error: unable to upgrade connection: container not found ("gcp-sdk-container")
Creating gs://keda-test-storage-bucket/...
You are attempting to perform an operation that requires a project id, with none configured. Please re-run gsutil config and make sure to follow the instructions for finding and entering your default project id.
command terminated with exit code ***
  ✖ initializing the gcp-sdk pod should work.. Setting GCP authentication on gcp-sdk should work..
BucketNotFoundException: 404 gs://keda-test-storage-bucket bucket does not exist.
command terminated with exit code ***
BucketNotFoundException: 404 gs://keda-test-storage-bucket bucket does not exist.

@RamCohen
Wait for gcp-sdk container to be ready in test
7ee6ab8 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
JorTurFer
JorTurFer approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
Thanks ❤️ ❤️ ❤️
Only 1 small nit: Could you update the changelog to

- **General:** Introduce new GCP Storage Scaler ([#2628](https://github.com/kedacore/keda/issues/2628))

@zroubalik
Copy link
Member

zroubalik commented Mar 4, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp-storage.test.*
Update: You can check the progres here

@RamCohen
Update changelog
67fe599 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@RamCohen
Copy link
Contributor Author

RamCohen commented Mar 5, 2022

/run-e2e gcp-storage.test.* Update: You can check the progres here

Test has error:
Creating gs://keda-test-storage-bucket/...
AccessDeniedException: 403 The billing account for the owning project is disabled in state absent

@JorTurFer
Copy link
Member

JorTurFer commented Mar 11, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp-storage.test.*
Update: You can check the progres here

@JorTurFer
Copy link
Member

JorTurFer commented Mar 12, 2022 •
edited
Loading

It seems that the problem was also the credentials

@RamCohen
Merge remote-tracking branch 'origin' into gcs
8733dec 
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Mar 12, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp-storage.test.*
Update: You can check the progres here

@RamCohen
Merge remote-tracking branch 'origin' into gcs
9d8addc 
Signed-off-by: Ram Cohen ram.cohen@gmail.com
@zroubalik
Copy link
Member

zroubalik commented Mar 14, 2022 •
edited by github-actions bot
Loading

/run-e2e gcp-storage.test.*
Update: You can check the progres here

zroubalik
zroubalik approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @RamCohen!

@zroubalik zroubalik merged commit b8913a1 into kedacore:main Mar 14, 2022
RamCohen added a commit to RamCohen/keda that referenced this pull request Mar 23, 2022
@RamCohen
Add a scaler based on number of objects in a GCP Storage bucket (keda…
375799a 
…core#2648)

Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Co-authored-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Ram Cohen <ram.cohen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@zroubalik zroubalik zroubalik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide scaler for Google cloud storage
4 participants
@RamCohen @tomkerkhove @zroubalik @JorTurFer