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

Add debug logs tracking validation of ScaledObjects on webhook #6498

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

be0x74a
Copy link
Contributor

@be0x74a be0x74a commented Jan 25, 2025

Add debug logs tracking validation of ScaledObjects on webhook

Checklist

  • (N/A) When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • (N/A) Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • (N/A) A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • (N/A) A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #

@be0x74a be0x74a requested a review from a team as a code owner January 25, 2025 12:08
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

I'm wondering if it's safer just to have a map / slice with function names and avoid getting the function name from a program counter. In my past experience, various go compiler optimizations and inlining have made usage of FuncForPC occasionally a little unstable

@@ -588,3 +594,7 @@ func getHpaName(so ScaledObject) string {

return so.Spec.Advanced.HorizontalPodAutoscalerConfig.Name
}

func getFunctionName(i interface{}) string {
return runtime.FuncForPC(reflect.ValueOf(i).Pointer()).Name()
Copy link
Member

Choose a reason for hiding this comment

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

alternatively, if we go forward with FuncForPC, the code should probably check for nil ptr because FuncForPC can return nil, docs. This will future-proof the code a bit in case of go compiler optimizations changing this behavior

@be0x74a
Copy link
Contributor Author

be0x74a commented Jan 27, 2025

I'm wondering if it's safer just to have a map / slice with function names and avoid getting the function name from a program counter. In my past experience, various go compiler optimizations and inlining have made usage of FuncForPC occasionally a little unstable

Ah okay, wasn't aware of it. Since the lookup is being made only during the scope of one function, maybe it's better to follow your suggestion and create a map with the name + function which will be iterated on

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

apis/keda/v1alpha1/scaledobject_webhook.go Show resolved Hide resolved
@wozniakjan
Copy link
Member

wozniakjan commented Jan 27, 2025

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

@wozniakjan
Copy link
Member

just for the record, I think the linter errors relate to a known buggy implementation of the webhooks - #5962

apis/keda/v1alpha1/scaledobject_webhook.go:183:74: verifyReplicaCount - result 0 (error) is always nil (unparam)
func verifyReplicaCount(incomingSo *ScaledObject, action string, _ bool) error {
                                                                         ^
apis/keda/v1alpha1/scaledobject_webhook.go:192:70: verifyFallback - result 0 (error) is always nil (unparam)
func verifyFallback(incomingSo *ScaledObject, action string, _ bool) error {

imho it is not responsibility of this PR to address and fix those

@wozniakjan wozniakjan mentioned this pull request Jan 29, 2025
23 tasks
@JorTurFer JorTurFer merged commit 95dbbcb into kedacore:main Feb 3, 2025
20 of 21 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.

3 participants