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 reconciler kv in our reconciler loggers #5170

Merged

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Aug 27, 2021

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
This is the root cause of the panic in CAPD fixed here: #5167 (review)

With the current main version we have an uneven k/v count. This means as soon as we try to log something with a non-string as value klog will panic (as previously in CAPD)

I hope I got all, would be good if somebody could check again

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 27, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 27, 2021
@sbueringer sbueringer force-pushed the pr-fix-reconciler-log-name-kv branch from 9923a3d to 92c05ad Compare August 27, 2021 16:14
@sbueringer
Copy link
Member Author

sbueringer commented Aug 27, 2021

After we upgrade to the next controller-runtime release (for kubernetes-sigs/controller-runtime#1648) The logs will look like this:

I0827 18:07:09.474490   64958 controller.go:207] controller/dockercluster "msg"="Starting workers" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="DockerCluster" "worker count"=1

@sbueringer sbueringer force-pushed the pr-fix-reconciler-log-name-kv branch from 92c05ad to 2282884 Compare August 27, 2021 16:16
@fabriziopandini
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Aug 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit 764d1bd into kubernetes-sigs:master Aug 27, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Aug 27, 2021
@sbueringer sbueringer deleted the pr-fix-reconciler-log-name-kv branch August 27, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants