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

Fix error handling in node label update path #1887

Closed
vikasmb opened this issue Feb 25, 2022 · 2 comments · Fixed by #1892
Closed

Fix error handling in node label update path #1887

vikasmb opened this issue Feb 25, 2022 · 2 comments · Fixed by #1892
Labels

Comments

@vikasmb
Copy link
Contributor

vikasmb commented Feb 25, 2022

What happened:

In

log.Errorf("Failed to update node %s with label %q: %q, error: %v", c.myNodeName, key, value, err)
if error occurs in node label update, it needs to return error to the caller.

Instead, it logs the error and continues on to indicate that it updated the node with the label in

log.Debugf("Updated node %s with label %q: %q", c.myNodeName, key, value)

Attach logs

What you expected to happen:
If error occurs in node label update, it needs to return error to the caller.
How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

  • Kubernetes version (use kubectl version):
  • CNI Version 1.10.2
  • OS (e.g: cat /etc/os-release):
  • Kernel (e.g. uname -a):
@vikasmb vikasmb added the bug label Feb 25, 2022
@M00nF1sh
Copy link
Contributor

M00nF1sh commented Feb 25, 2022

we should consider use TwoWayStrategicMergePatch instead of update. (which saves request payload & we can disable resource version check safely for this use case)

https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/76bd7e3fc59a3dffa28397db2991edfd1a5889c7/pkg/targetgroupbinding/resource_manager.go#L503

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants