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.

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

feat: Provide scaler for Amazon managed service for Prometheus #5373

Merged
merged 37 commits into from
Jan 18, 2024

Conversation

sguruvar
Copy link
Contributor

@sguruvar sguruvar commented Jan 15, 2024

Provide a description of what has been changed

Checklist

Fixes #2214

@sguruvar sguruvar requested a review from a team as a code owner January 15, 2024 02:02
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4_test.go Outdated Show resolved Hide resolved
pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Jan 15, 2024

Also, the changelog has to be updated

@JorTurFer
Copy link
Member

And last but not least, please fix DCO check. You can get more info about how to solve it clicking in the check link
image

pkg/scalers/aws_sigv4.go Outdated Show resolved Hide resolved
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.

We should also update documentation for 2.13: https://github.com/kedacore/keda-docs

@zroubalik zroubalik mentioned this pull request Jan 15, 2024
25 tasks
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

You have resolved the conversations but after the push I still see the code changes. Maybe you didn't commit the change by error

@sguruvar
Copy link
Contributor Author

sguruvar commented Jan 15, 2024

You have resolved the conversations but after the push I still see the code changes. Maybe you didn't commit the change by error

done

dttung2905 and others added 11 commits January 15, 2024 15:25
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: geoffrey1330 <israelgeoffrey13@gmail.com>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Geoffrey Israel <israelgeoffrey13@gmail.com>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Co-authored-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
…re#5369)

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
* chore: fix jetstream flaky test

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* use specific instances instead of default client

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

* Update mock responses

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

---------

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <gurusiva@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
…ged_prometheus_pod_identity_test.go

Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Siva Guruvareddiar and others added 4 commits January 15, 2024 15:30
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <gurusiva@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 17, 2024

/run-e2e aws
Update: You can check the progress here

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

new e2e are failing (with a panic) and I'd say that it's happening at workspace creation time

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 17, 2024

/run-e2e aws
Update: You can check the progress here

@JorTurFer
Copy link
Member

Error logs from e2e test are really weird:

=== RUN   TestScaler
    aws_managed_prometheus_test.go:138: --- setting up ---
--- FAIL: TestScaler (0.42s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x10f5ac8]

goroutine 132 [running]:
testing.tRunner.func1.2({0x126caa0, 0x2327760})
	/usr/local/go/src/testing/testing.go:1545 +0x1c4
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1548 +0x360
panic({0x126caa0?, 0x2327760?})
	/usr/local/go/src/runtime/panic.go:914 +0x218
command-line-arguments_test.TestScaler(0x4000475860)
	/__w/keda/keda/tests/scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go:143 +0x178
testing.tRunner(0x4000475860, 0x15808b0)
	/usr/local/go/src/testing/testing.go:1595 +0xe8
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1648 +0x33c
FAIL	command-line-arguments	0.491s
FAIL

It looks that the test can't start for any reason, but the issue looks in the test script side, not in KEDA. Could you take a look?

@sguruvar
Copy link
Contributor Author

Error logs from e2e test are really weird:

=== RUN   TestScaler
    aws_managed_prometheus_test.go:138: --- setting up ---
--- FAIL: TestScaler (0.42s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x10f5ac8]

goroutine 132 [running]:
testing.tRunner.func1.2({0x126caa0, 0x2327760})
	/usr/local/go/src/testing/testing.go:1545 +0x1c4
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1548 +0x360
panic({0x126caa0?, 0x2327760?})
	/usr/local/go/src/runtime/panic.go:914 +0x218
command-line-arguments_test.TestScaler(0x4000475860)
	/__w/keda/keda/tests/scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go:143 +0x178
testing.tRunner(0x4000475860, 0x15808b0)
	/usr/local/go/src/testing/testing.go:1595 +0xe8
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1648 +0x33c
FAIL	command-line-arguments	0.491s
FAIL

It looks that the test can't start for any reason, but the issue looks in the test script side, not in KEDA. Could you take a look?

Am unable to reproduce this. Test goes fine in my local when I try GOOS="darwin" go test -v -tags e2e -timeout 2m scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go and I could be able to see the AMP workspace created and removed after the test is done. I set the TF_AWS_ACCESS_KEY, TF_AWS_SECRET_KEY and TF_AWS_REGION.. Is there a good way to replicate this?

pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4_test.go Show resolved Hide resolved
@zroubalik
Copy link
Member

Error logs from e2e test are really weird:

=== RUN   TestScaler
    aws_managed_prometheus_test.go:138: --- setting up ---
--- FAIL: TestScaler (0.42s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x10f5ac8]

goroutine 132 [running]:
testing.tRunner.func1.2({0x126caa0, 0x2327760})
	/usr/local/go/src/testing/testing.go:1545 +0x1c4
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1548 +0x360
panic({0x126caa0?, 0x2327760?})
	/usr/local/go/src/runtime/panic.go:914 +0x218
command-line-arguments_test.TestScaler(0x4000475860)
	/__w/keda/keda/tests/scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go:143 +0x178
testing.tRunner(0x4000475860, 0x15808b0)
	/usr/local/go/src/testing/testing.go:1595 +0xe8
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1648 +0x33c
FAIL	command-line-arguments	0.491s
FAIL

It looks that the test can't start for any reason, but the issue looks in the test script side, not in KEDA. Could you take a look?

Am unable to reproduce this. Test goes fine in my local when I try GOOS="darwin" go test -v -tags e2e -timeout 2m scalers/aws/aws_managed_prometheus/aws_managed_prometheus_test.go and I could be able to see the AMP workspace created and removed after the test is done. I set the TF_AWS_ACCESS_KEY, TF_AWS_SECRET_KEY and TF_AWS_REGION.. Is there a good way to replicate this?

even if you are not able to reproduce, the test shouldn't end with panic, we should add a check there and fire proper error anyway.

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member

JorTurFer commented Jan 17, 2024

/run-e2e aws
Update: You can check the progress here

@JorTurFer
Copy link
Member

even if you are not able to reproduce, the test shouldn't end with panic, we should add a check there and fire proper error anyway.

I think that my previous code suggestion was wrong and that's why the test panic before any assertion error xD

@sguruvar
Copy link
Contributor Author

sguruvar commented Jan 17, 2024

even if you are not able to reproduce, the test shouldn't end with panic, we should add a check there and fire proper error anyway.

I think that my previous code suggestion was wrong and that's why the test panic before any assertion error xD

Looks like permission is missing to create the AMP workspace: operation error amp: CreateWorkspace, https response error StatusCode: 40***, RequestID: defa5098-e960-4***4-b684-a0c46dfdf6***d, AccessDeniedException: User: arn:aws:iam::***:user/e***e-test-user is not authorized to perform: aps:CreateWorkspace on resource: arn:aws:aps:***:***:/workspaces... can you add the permission to create and delete for this e2e-test-user ?

@JorTurFer
Copy link
Member

JorTurFer commented Jan 17, 2024

/run-e2e aws
Update: You can check the progress here

@JorTurFer
Copy link
Member

It seems that it works, so once all the commits are addressed, we can merge this

@sguruvar
Copy link
Contributor Author

sguruvar commented Jan 17, 2024

It seems that it works, so once all the commits are addressed, we can merge this

Thanks.. I believe all the comments are addressed. Can you help merge?

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.

Massive nit, please use go convetion when writing docs on fucntions, thanks!

pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
pkg/scalers/aws/aws_sigv4.go Outdated Show resolved Hide resolved
sguruvar and others added 5 commits January 18, 2024 06:59
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Siva Guruvareddiar <1725781+sguruvar@users.noreply.github.com>
Signed-off-by: Siva Guruvareddiar <sivagurunath@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Jan 18, 2024

/run-e2e aws
Update: You can check the progress here

@zroubalik zroubalik merged commit 49b0fd0 into kedacore:main Jan 18, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scaler for Amazon managed service for Prometheus
6 participants