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

fix: output the name of the worker to console when added #5120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: output the name of the worker to console when added #5120

wants to merge 1 commit into from

Conversation

BobVanB
Copy link
Contributor

@BobVanB BobVanB commented Feb 24, 2025 •
edited
Loading

Description

Change the output in the console for debugging purpose.

time="2025-02-24T05:59:44+01:00" level=debug msg="node added"
time="2025-02-24T05:59:44+01:00" level=debug msg="node added"
time="2025-02-24T05:59:44+01:00" level=debug msg="node added"

vs.

time="2025-02-24T05:59:44+01:00" level=debug msg="Node added: myworker1"
time="2025-02-24T05:59:44+01:00" level=debug msg="Node added: myworker2"
time="2025-02-24T05:59:44+01:00" level=debug msg="Node added: myworker3"

The output is rather fun if there are 400+ workers and you only see node added.

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign raffo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2025
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 24, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @BobVanB. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 24, 2025
@BobVanB BobVanB force-pushed the better_error_handling branch from 6dc2aa0 to 63d3415 Compare February 24, 2025 05:34
ivankatliarchuk
ivankatliarchuk suggested changes Feb 24, 2025
View reviewed changes
source/node.go
@@ -57,7 +57,11 @@ func NewNodeSource(ctx context.Context, kubeClient kubernetes.Interface, annotat
nodeInformer.Informer().AddEventHandler(
cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
log.Debug("node added")
node, ok := obj.(*v1.Node)
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

should be something like, pseudo code

if loglevel == debug
  do the stuff

source/node.go Outdated
log.Debug("node added")
node, ok := obj.(*v1.Node)
if !ok {
log.Debug("node added")
Copy link
Contributor

Choose a reason for hiding this comment

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

This construct is missing a return statement and should be an log.Error, otherwise we always going to have execution in if !ok block and next statement log.Debugf("node added: %s", node.ObjectMeta.Name)

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 @ivankatliarchuk,

The code does not return anything, that is why i don't throw an error.
Otherwise i have to check all the code paths if its handled correctly.

log.Debug("node added")

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an error case IF it's ever happens

log.Errorf("expected *v1.Node but got %T", obj)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it always going to be of type v1.Node?
If that is true, then i think you are right.

Copy link
Contributor

@ivankatliarchuk ivankatliarchuk Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it should be always v1.Node, if it's not, someone could catch an error and submit a fix

Copy link
Contributor

Choose a reason for hiding this comment

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

but consider a simple case

  • Type Mismatch: The value is a v1.Node (non-pointer) instead of *v1.Node (pointer).

source/node.go
@@ -57,7 +57,11 @@ func NewNodeSource(ctx context.Context, kubeClient kubernetes.Interface, annotat
nodeInformer.Informer().AddEventHandler(
cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
log.Debug("node added")
node, ok := obj.(*v1.Node)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test. There test helper for logs

external-dns/internal/testutils/log.go

Line 36 in be7f970

func LogsToBuffer(level log.Level, t *testing.T) *bytes.Buffer {

@ivankatliarchuk
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2025
@BobVanB BobVanB force-pushed the better_error_handling branch from 63d3415 to 3e21b25 Compare February 24, 2025 10:32
@ivankatliarchuk
Copy link
Contributor

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 26, 2025
@BobVanB
Copy link
Contributor Author

BobVanB commented Feb 26, 2025

Currently a little occupide, i will work on it when i have the time.
It is not something critical. Rather do it well and then merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivankatliarchuk ivankatliarchuk ivankatliarchuk requested changes

@szuecs szuecs Awaiting requested review from szuecs

Assignees

@ivankatliarchuk ivankatliarchuk

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BobVanB @k8s-ci-robot @ivankatliarchuk