Skip to content

Fix crash in findFoundationDBClusterForNode when in global mode #2273

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dig-whois
Copy link

Description

When the operator is executed with cluster-label-key-for-node-trigger flag in global mode it goes to crash loop, because the namespace is empty and in FoundationDBClusterReconciler.findFoundationDBClusterForNode a null pointer is passed to k8s-client Lister. We fix this issue by passing the variable only if namespace is not empty.

Type of change

Please select one of the options below.

X Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • Other

Discussion

N/A

Testing

We ran it in different environments.

Do we need to perform additional testing once this is merged, or perform in a larger testing environment?

No

Documentation

Did you update relevant documentation within this repository?

If this change is adding new functionality, do we need to describe it in our user manual?

If this change is adding or removing subreconcilers, have we updated the core technical design doc to reflect that?

If this change is adding new safety checks or new potential failure modes, have we documented and how to debug potential issues?

Follow-up

Are there any follow-up issues that we should pursue in the future?

Does this introduce new defaults that we should re-evaluate in the future?

@johscheuer johscheuer self-requested a review May 9, 2025 21:07
@johscheuer johscheuer added the bug Something isn't working label May 10, 2025
Copy link
Member

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

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

The changes LGTM, could you add a unit tests where the reconciler namespace is unset?

@dig-whois
Copy link
Author

The changes LGTM, could you add a unit tests where the reconciler namespace is unset?

Sure, I can do that.

When the operator is execute with cluster-label-key-for-node-trigger flag in global mode it goes to crash loop, becasue the namespace is empty and in findFoundationDBClusterForNode a null pointer is passed to k8s-client Lister. We fix this issue by passing the variable only if namespace is not empty.
@dig-whois dig-whois force-pushed the fix-null-ptr-issue-in-global-mode branch from fc81ab9 to d5b6e28 Compare May 13, 2025 13:09
@dig-whois dig-whois requested a review from johscheuer May 13, 2025 13:11
@dig-whois dig-whois force-pushed the fix-null-ptr-issue-in-global-mode branch from d5b6e28 to 4234989 Compare May 13, 2025 13:18
Context("when finding fdb pods in a node", func() {
// All the nodes that are part of the cluster
var originalNodeList corev1.NodeList
// unrelatedNode is a node that does not holde a pod that is part of the cluster
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// unrelatedNode is a node that does not holde a pod that is part of the cluster
// unrelatedNode is a node that does not hold a pod that is part of the cluster

err := k8sClient.Delete(context.Background(), unrelatedNode)
Expect(err).NotTo(HaveOccurred())
})
When("wathcing all namespaces", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When("wathcing all namespaces", func() {
When("watching all namespaces", func() {

@dig-whois dig-whois force-pushed the fix-null-ptr-issue-in-global-mode branch from 4234989 to 6f4016a Compare May 13, 2025 20:41
@dig-whois dig-whois requested a review from johscheuer May 14, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants