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

Handle the context in webhook #75

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MichelHollands
Copy link
Contributor

This will stop the request from adding an annotation to the statefulset if the context is cancelled.

@MichelHollands MichelHollands requested a review from a team as a code owner August 3, 2023 11:04
Signed-off-by: Michel Hollands <michel.hollands@gmail.com>
Signed-off-by: Michel Hollands <michel.hollands@gmail.com>
Signed-off-by: Michel Hollands <michel.hollands@gmail.com>
Comment on lines +143 to 149
select {
case <-ctx.Done():
return nil, ctx.Err()
default:
}

switch ar.Request.Resource.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? All the calls below have the contexts provided.

@colega
Copy link
Contributor

colega commented Aug 3, 2023

It is usually unneeded to add the ctx.Done() check in the code that is not doing something that should be immediately interrupted. Why do we need it here? It seems that all kubernetes client calls already have the context provided, so I'd expect them to check it. Don't they?

Comment on lines +143 to +147
select {
case <-ctx.Done():
return nil, ctx.Err()
default:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is shorter and equivalent:

Suggested change
select {
case <-ctx.Done():
return nil, ctx.Err()
default:
}
if err := ctx.Err(); err != nil {
return nil, err
}

@MichelHollands MichelHollands marked this pull request as draft August 3, 2023 13:18
@MichelHollands
Copy link
Contributor Author

@colega Sorry, I should have left this as a draft PR. We are having a problem with the prepareDownscale webhook on AWS and Azure where it returns the HPA controller was unable to update the target scale: Operation cannot be fulfilled on statefulsets.apps "ingester -zone-a": the object has been modified; please apply your changes to the latest version and try again. This was a test to see if somehow multiple hooks updated the same object and caused the error.

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.

None yet

2 participants