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

Rancher: Fix error messages and expose underlying error. #6363

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Rancher: Fix error messages and expose underlying error. #6363

merged 5 commits into from
Jan 15, 2024

Conversation

ElanHasson
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Error messages were incorrect regardless of which annotation was failing.

Also exposing the underlying parse error so it can actually be fixed.

Which issue(s) this PR fixes:

n/a

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Errors parsing resource annotations for the Rancher provider will now emit the correct error message as well as expose the underlying parser error.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@ElanHasson
Fix error messages and expose underlying error.
3c4b180 
Error messages were incorrect regardless of which annotation was failing.

Also exposing the underlying parse error so it can actually be fixed.
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 11, 2023
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Dec 11, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 11, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @ElanHasson!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 11, 2023
@k8s-ci-robot k8s-ci-robot added area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 11, 2023
@Shubham82
Copy link
Contributor

Thanks @ElanHasson
It looks good to me.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2023
@Shubham82
Copy link
Contributor

cc @ctrox @gajicdev @pawelkuc @thirdeyenick
PTAL!

ctrox
ctrox reviewed Dec 11, 2023
View reviewed changes
cluster-autoscaler/cloudprovider/rancher/rancher_nodegroup.go Outdated
@@ -458,7 +458,7 @@ func parseResourceAnnotations(annotations map[string]string) (corev1.ResourceLis

cpuResources, err := resource.ParseQuantity(cpu)
if err != nil {
return nil, fmt.Errorf("unable to parse cpu resources: %s", cpu)
return nil, fmt.Errorf("unable to parse cpu resources: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still include the string cpu in the error message in addition to err? That might also give a hint why the parsing has failed. Same goes for the other errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

I'm not following..CPU is in the error for CPU?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the variable cpu a few lines above:

cpu, ok := annotations[resourceCPUAnnotation]

If it can't be parsed by resource.ParseQuantity(cpu), it would be helpful if the error message would contain the value. So the error would be something like:

fmt.Errorf("unable to parse cpu resources from %q: %w", cpu, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, yeah 👍🏻
I thought you meant the literal string 'cpu', which is what I removed previously for non-CPU values.

Wilo fix shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now.

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect, thanks!

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2023
@Shubham82
Copy link
Contributor

Thanks @ElanHasson @ctrox

@Shubham82
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2023
@ElanHasson
Copy link
Contributor Author

Hopefully this helps others as I've been trying to get the granular scaling stuff to work and kept seeing unable to parse "8" CPU lol.

@ElanHasson
Copy link
Contributor Author

Hi @ctrox @Shubham82

Any word on when this can get merged?

@Shubham82
Copy link
Contributor

@towca could you please approve this PR so that it will merge?

Thanks

@towca
Copy link
Collaborator

towca commented Jan 15, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ctrox, ElanHasson, towca

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit 13c5875 into kubernetes:master Jan 15, 2024
6 checks passed
@ElanHasson ElanHasson deleted the patch-1 branch January 17, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctrox ctrox ctrox approved these changes

@BigDarkClown BigDarkClown Awaiting requested review from BigDarkClown

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@Shubham82 Shubham82

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler area/provider/rancher cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ElanHasson @k8s-ci-robot @Shubham82 @towca @ctrox