🐛Fix a stacktrace in LB logic by removing listener name from an error message when not set #1853

huxcrux · 2024-02-01T16:01:42Z

What this PR does / why we need it:
When a load balancer listener fails to be created an stacktrace apperas due to an error message trying to include the name of the listener that failed to be created.

Stacktrace:

I0201 14:57:33.013415       1 controller.go:115] "Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference" controller="openstackcluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="OpenStackCluster" OpenStackCluster="default/hux-lab1" namespace="default" name="hux-lab1" reconcileID="d33d5039-62bd-4efe-a004-f939ad3ac7e2"
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1c393e0]

goroutine 539 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116 +0x1fa
panic({0x1e601c0, 0x365ba70})
	/usr/local/go/src/runtime/panic.go:884 +0x213
sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/loadbalancer.(*Service).getOrUpdateAllowedCIDRS(0xc000bbc360, 0xc000a01000, 0xc000f54000)
	/workspace/pkg/cloud/services/loadbalancer/loadbalancer.go:290 +0x9e0
sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/loadbalancer.(*Service).ReconcileLoadBalancer(0xc000bbc360, 0xc000a01000, {0xc000c834c0, 0x10}, 0x192b)
	/workspace/pkg/cloud/services/loadbalancer/loadbalancer.go:163 +0xa28
sigs.k8s.io/cluster-api-provider-openstack/controllers.reconcileNetworkComponents({0x24d05e0, 0xc000cdd480}, 0xc000b88820, 0xc000a01000)
	/workspace/controllers/openstackcluster_controller.go:567 +0x12d8
sigs.k8s.io/cluster-api-provider-openstack/controllers.reconcileNormal({0x24d05e0, 0xc000cdd480}, 0x0?, 0xc000a01000)
	/workspace/controllers/openstackcluster_controller.go:300 +0xf7
sigs.k8s.io/cluster-api-provider-openstack/controllers.(*OpenStackClusterReconciler).Reconcile(0xc000a80120, {0x24c6f58, 0xc000c033e0}, {{{0xc000a143c8?, 0x0?}, {0xc000a143c0?, 0xc00116bd48?}}})
	/workspace/controllers/openstackcluster_controller.go:138 +0x736
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x24ccc10?, {0x24c6f58?, 0xc000c033e0?}, {{{0xc000a143c8?, 0xb?}, {0xc000a143c0?, 0x0?}}})
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119 +0xc8
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc00066e000, {0x24c6eb0, 0xc00002bae0}, {0x1f2a740?, 0xc0005300e0?})
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 +0x3ca
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc00066e000, {0x24c6eb0, 0xc00002bae0})
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266 +0x1d9
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:223 +0x587

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
A part of the problem mentioned in #1687

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

netlify · 2024-02-01T16:01:48Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`403c98f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65bceeea388d7e000893c875
😎 Deploy Preview	https://deploy-preview-1853--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot · 2024-02-01T16:01:52Z

Hi @huxcrux. 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.

Details

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/test-infra repository.

EmilienM · 2024-02-02T03:30:16Z

pkg/cloud/services/loadbalancer/loadbalancer.go

 		listener, err := s.loadbalancerClient.UpdateListener(listener.ID, listenerUpdateOpts)
 		if err != nil {
-			record.Warnf(openStackCluster, "FailedUpdateListener", "Failed to update listener %s: %v", listener.Name, err)
+			record.Warnf(openStackCluster, "FailedUpdateListener", "Failed to update listener: %v", err)


instead of using Name, could we just use ID? So the user has still a way to find which LB wasn't update.

The problem here is that listener is nil due to

cluster-api-provider-openstack/pkg/clients/loadbalancer.go

Line 126 in 4fa8cd1

return nil, err

I can verify this but I think this will result in the same problem

Right, so we would need to save the name or ID before the call. I mean the stack trace would happen on 288 if it was nil there already, so we know the ID existed before the call.
Not the nicest thing to do I guess but it would work...

listenerID := listener.ID listener, err := s.loadbalancerClient.UpdateListener(...)

Or perhaps rather this?

updatedListener, err := s.loadbalancerClient.UpdateListener(...)

listenerName is included when this function is called. I change to code to use that variable instead to avoid adding any variables just used for a potential error message

NVM I scrolled a bit to quick in my IDE, I use the first of your suggested fixes to avoid updating multiple other places or overrideing listneer with updatedListener after the null check

lentzi90 · 2024-02-02T08:17:18Z

/ok-to-test

k8s-ci-robot · 2024-02-02T14:59:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huxcrux, mdbooth

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mdbooth]

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

dulek · 2024-02-02T15:01:47Z

/lgtm

EmilienM · 2024-02-02T15:15:28Z

/unhold

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 1, 2024

k8s-ci-robot requested review from lentzi90 and mdbooth February 1, 2024 16:01

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 1, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 1, 2024

EmilienM reviewed Feb 2, 2024

View reviewed changes

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 2, 2024

huxcrux force-pushed the lb-stacktrace-fix branch 2 times, most recently from 406bf1d to d931504 Compare February 2, 2024 13:05

Remove listener.name from from error message when not set

403c98f

huxcrux force-pushed the lb-stacktrace-fix branch from d931504 to 403c98f Compare February 2, 2024 13:32

mdbooth approved these changes Feb 2, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2024

k8s-ci-robot assigned dulek Feb 2, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2024

k8s-ci-robot merged commit 84e1f4e into kubernetes-sigs:main Feb 2, 2024

huxcrux deleted the lb-stacktrace-fix branch February 2, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛Fix a stacktrace in LB logic by removing listener name from an error message when not set #1853

🐛Fix a stacktrace in LB logic by removing listener name from an error message when not set #1853

Uh oh!

huxcrux commented Feb 1, 2024 •

edited

Loading

Uh oh!

netlify bot commented Feb 1, 2024 •

edited

Loading

Uh oh!

k8s-ci-robot commented Feb 1, 2024

Uh oh!

EmilienM Feb 2, 2024

Uh oh!

huxcrux Feb 2, 2024 •

edited

Loading

Uh oh!

lentzi90 Feb 2, 2024

Uh oh!

huxcrux Feb 2, 2024 •

edited

Loading

Uh oh!

lentzi90 commented Feb 2, 2024

Uh oh!

k8s-ci-robot commented Feb 2, 2024

Uh oh!

dulek commented Feb 2, 2024

Uh oh!

EmilienM commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

🐛Fix a stacktrace in LB logic by removing listener name from an error message when not set #1853

🐛Fix a stacktrace in LB logic by removing listener name from an error message when not set #1853

Uh oh!

Conversation

huxcrux commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Uh oh!

k8s-ci-robot commented Feb 1, 2024

Uh oh!

EmilienM Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

huxcrux Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lentzi90 Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

huxcrux Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lentzi90 commented Feb 2, 2024

Uh oh!

k8s-ci-robot commented Feb 2, 2024

Uh oh!

dulek commented Feb 2, 2024

Uh oh!

EmilienM commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

huxcrux commented Feb 1, 2024 •

edited

Loading

netlify bot commented Feb 1, 2024 •

edited

Loading

huxcrux Feb 2, 2024 •

edited

Loading

huxcrux Feb 2, 2024 •

edited

Loading