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(mysql/user): correctly compare upToDate resourceOptions #71

Merged
merged 1 commit into from
Mar 19, 2022

Conversation

Duologic
Copy link
Member

@Duologic Duologic commented Mar 16, 2022

Description of your changes

The pointer-compare caused a panic in my test cluster, had another look at the Postgresql
cod, there the actual values are used not the pointers to compare. Also it doesn't
initialize the integer values, so removed that too.

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Ran make reviewable test e2e

@Duologic

This comment was marked as resolved.

@Duologic Duologic force-pushed the duologic/fix_mysql_resourceoptions branch from 3eadece to ace4457 Compare March 16, 2022 15:06
@Duologic

This comment was marked as resolved.

Signed-off-by: Duologic <jeroen@simplistic.be>
@Duologic Duologic force-pushed the duologic/fix_mysql_resourceoptions branch from ace4457 to 0f12654 Compare March 17, 2022 08:55
@Duologic Duologic requested a review from jdotw March 17, 2022 08:56
@danports
Copy link

The SQL provider has been crashlooping on one of my clusters for a couple of days now - I'm guessing this issue is causing that?

Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 701 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x143f340, 0x23deea0})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/runtime/runtime.go:74 +0x85
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc000a49fe0})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/runtime/runtime.go:48 +0x75
panic({0x143f340, 0x23deea0})
        /opt/hostedtoolcache/go/1.17.7/x64/src/runtime/panic.go:1038 +0x215
github.com/crossplane-contrib/provider-sql/pkg/controller/mysql/user.upToDate(...)
        /home/runner/work/provider-sql/provider-sql/pkg/controller/mysql/user/reconciler.go:376
github.com/crossplane-contrib/provider-sql/pkg/controller/mysql/user.(*external).Observe(0xc0009420a0, {0x17e2920, 0xc000ba0b40}, {0x182bcc8, 0xc000b09e00})
        /home/runner/work/provider-sql/provider-sql/pkg/controller/mysql/user/reconciler.go:235 +0x644
github.com/crossplane/crossplane-runtime/pkg/reconciler/managed.(*Reconciler).Reconcile(0xc00093b900, {0x17e2958, 0xc000b71230}, {{{0x0, 0x0}, {0xc0007a5430, 0xd}}})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/github.com/crossplane/crossplane-runtime@v0.13.0/pkg/reconciler/managed/reconciler.go:577 +0xec7
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc00093b9a0, {0x17e28b0, 0xc000962800}, {0x148bfe0, 0xc000a49fe0})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:293 +0x303
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc00093b9a0, {0x17e28b0, 0xc000962800})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:248 +0x205
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1.1({0x17e28b0, 0xc000962800})
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:211 +0x46
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext.func1()
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:185 +0x25
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x7f020957a9f0)
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:155 +0x67
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0x0, {0x17ba280, 0xc000b71200}, 0x1, 0xc0006e2240)
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:156 +0xb6
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0x0, 0x3b9aca00, 0x0, 0x0, 0x0)
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:133 +0x89
k8s.io/apimachinery/pkg/util/wait.JitterUntilWithContext({0x17e28b0, 0xc000962800}, 0xc000b35a20, 0x0, 0x0, 0x0)
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:185 +0x99
k8s.io/apimachinery/pkg/util/wait.UntilWithContext({0x17e28b0, 0xc000962800}, 0x0, 0x0)
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/k8s.io/apimachinery@v0.20.1/pkg/util/wait/wait.go:99 +0x2b
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1
        /home/runner/work/provider-sql/provider-sql/.work/pkg/pkg/mod/sigs.k8s.io/controller-runtime@v0.8.0/pkg/internal/controller/controller.go:208 +0x531

@Duologic
Copy link
Member Author

Exactly, this PR attempts to fix it.

Copy link
Collaborator

@jdotw jdotw left a comment

Choose a reason for hiding this comment

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

nice!

@jdotw jdotw merged commit 916112c into master Mar 19, 2022
@jdotw jdotw deleted the duologic/fix_mysql_resourceoptions branch March 19, 2022 11:11
@danports
Copy link

Thanks @Duologic. How often do new Docker images get pushed? Apparently the latest build (provider-sql-controller:v0.4.0-2.g916112c) does not include this fix.

@Duologic
Copy link
Member Author

I'd expect that tag to include this fix, but I haven't tried it properly yet, will do early next week.

@Duologic
Copy link
Member Author

I'm seeing the same/similar error again too, my attempted fix didn't solve it. I'm looking at writing a unit test for it now so we can cover this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants