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

🐛 CAPD: fix panic in DockerMachinePool reconciliation #5167

Merged
merged 1 commit into from
Aug 27, 2021
Merged

🐛 CAPD: fix panic in DockerMachinePool reconciliation #5167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ type DockerMachinePoolReconciler struct {
// +kubebuilder:rbac:groups="",resources=secrets;,verbs=get;list;watch

func (r *DockerMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, rerr error) {
log := ctrl.LoggerFrom(ctx, "docker-machine-pool", req.NamespacedName)
log := ctrl.LoggerFrom(ctx)
Copy link
Member Author

@sbueringer sbueringer Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I dropped it to make it consistent with the DockerMachine controller. We add the MachinePool name a few lines below.

Copy link
Member

Choose a reason for hiding this comment

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

Why was this throwing a panic?

Copy link
Member Author

@sbueringer sbueringer Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

klog only accepts strings:

panic: key is not a string: {"Namespace":"machine-pool-fzbffp","Name":"machine-pool-omo8mt-dmp-0"}

goroutine 420 [running]:
k8s.io/klog/v2/klogr.flatten(0xc0039f0640, 0xa, 0xa, 0xc0037a86f0, 0x2)
	/go/pkg/mod/k8s.io/klog/[email protected]/klogr/klogr.go:158 +0x62e
k8s.io/klog/v2/klogr.klogger.Info(0x0, 0x0, 0xc0002f4840, 0x37, 0xc00349aea0, 0xb, 0x12, 0x1a2a357, 0x9, 0x1a8728d, ...)
	/go/pkg/mod/k8s.io/klog/[email protected]/klogr/klogr.go:200 +0x5c8
sigs.k8s.io/cluster-api/test/infrastructure/docker/exp/controllers.(*DockerMachinePoolReconciler)
	/workspace/test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go:75 +0xc25

https://github.com/kubernetes/klog/blob/v2.9.0/klogr/klogr.go#L158

The panic was not thrown here, but below in l.75 when we use the logger

Not sure why klog thinks it's a key.

Copy link
Member Author

@sbueringer sbueringer Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

o.O There's an uneven k/v count already before. Will try to figure out what's going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found at least one other bug, maybe two..


// Fetch the DockerMachinePool instance.
dockerMachinePool := &infrav1exp.DockerMachinePool{}
Expand Down