Add instance deletion - Githubissues

zugao commented 2 years ago

Summary

Added business logic for a postgresql instance deletion.

Checklist

[x] Categorize the PR by setting a good title and adding one of the labels: bug, enhancement, documentation, change, breaking, dependency as they show up in the changelog
[x] PR contains the label area:operator
[x] PR contains tests
[x] I have not made any changes in the charts/ directory.

Kidswiss commented 2 years ago

Currently the operator crashes when deleting an instance:

2022-05-31T09:36:59.586Z | INFO | provider-postgresql.operator | standalone/controller.go:96 | Deleting instance | {"controller": "postgresqlstandalone.postgresql.appcat.vshn.io", "controllerGroup": "postgresql.appcat.vshn.io", "controllerKind": "PostgresqlStandalone", "postgresqlStandalone": {"name":"my-instance","namespace":"default"}, "namespace": "default", "name": "my-instance", "reconcileID": "2f9312a6-a3e2-4555-9fa2-3fc1fbbd821e"}
2022-05-31T09:36:59.634Z | INFO | provider-postgresql.operator | controller/controller.go:117 | Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference | {"controller": "postgresqlstandalone.postgresql.appcat.vshn.io", "controllerGroup": "postgresql.appcat.vshn.io", "controllerKind": "PostgresqlStandalone", "postgresqlStandalone": {"name":"my-instance","namespace":"default"}, "namespace": "default", "name": "my-instance", "reconcileID": "2f9312a6-a3e2-4555-9fa2-3fc1fbbd821e"}
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=0x0 pc=0x13c2a46]

goroutine 192 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:118 +0x1f4
panic({0x1506200, 0x23d59b0})
    /usr/local/Cellar/go/1.18.2/libexec/src/runtime/panic.go:838 +0x207
github.com/vshn/appcat-service-postgresql/operator/standalone.(*DeleteStandalonePipeline).isHelmReleaseDeleted(0xc000576570?, {0xc00017d8f8?, 0xc000576570?})
    /Users/simonbeck/repos/appcat/appcat-service-postgresql/operator/standalone/delete.go:90 +0x6
github.com/ccremer/go-command-pipeline.If.func1({0x19128c8, 0xc0001d6270})
    /Users/simonbeck/go/pkg/mod/github.com/ccremer/go-command-pipeline@v0.17.0/predicate.go:46 +0x47
github.com/ccremer/go-command-pipeline.(*Pipeline).doRun(0xc00017db48, {0x19128c8, 0xc0001d6270})
    /Users/simonbeck/go/pkg/mod/github.com/ccremer/go-command-pipeline@v0.17.0/pipeline.go:137 +0x170
github.com/ccremer/go-command-pipeline.(*Pipeline).RunWithContext(0xc00017db48, {0x19128c8, 0xc0001d6270})
    /Users/simonbeck/go/pkg/mod/github.com/ccremer/go-command-pipeline@v0.17.0/pipeline.go:116 +0x45
github.com/vshn/appcat-service-postgresql/operator/standalone.(*DeleteStandalonePipeline).RunPipeline(0xc00039bd80, {0x19128c8, 0xc0001d6270})
    /Users/simonbeck/repos/appcat/appcat-service-postgresql/operator/standalone/delete.go:43 +0x525
github.com/vshn/appcat-service-postgresql/operator/standalone.(*PostgresStandaloneReconciler).DeleteDeployment(0xc0002ed0f0, {0x19128c8, 0xc0001d6270}, 0xc0005244e0)
    /Users/simonbeck/repos/appcat/appcat-service-postgresql/operator/standalone/controller.go:97 +0x137
github.com/vshn/appcat-service-postgresql/operator/standalone.(*PostgresStandaloneReconciler).Reconcile(0xc0002ed0f0, {0x19128c8?, 0xc0001d6000?}, {{{0xc0002ead90?, 0x10?}, {0xc0002ead70?, 0x40d7e7?}}})
    /Users/simonbeck/repos/appcat/appcat-service-postgresql/operator/standalone/controller.go:64 +0x225
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x1912820?, {0x19128c8?, 0xc0001d6000?}, {{{0xc0002ead90?, 0x160ee40?}, {0xc0002ead70?, 0x4041f4?}}})
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:121 +0xc8
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc0001eb860, {0x1912820, 0xc0001565c0}, {0x1557dc0?, 0xc00039bc80?})
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:320 +0x33c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc0001eb860, {0x1912820, 0xc0001565c0})
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:273 +0x1d9
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:234 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
    /Users/simonbeck/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.12.0/pkg/internal/controller/controller.go:230 +0x325

zugao commented 2 years ago

Currently the operator crashes when deleting an instance:

I updated the code and removed that pointer, can you retry again?

zugao commented 2 years ago

If the function gets more logic, hopefully the author and reviewer ensure that the test code is added/changed >accordingly as well. But now, it seems over-engineered, we have 20+ LoC to maintain and understand the test code for 1 loc of business >code... I'd prefer having to read less code and not do too much engineering for "eventualities".

Well the only time you will have to read the code is when it fails, but if you insist we can remove the unit test.

zugao commented 2 years ago

Thanks for the good work! 😄

Finally I merged it :D

vshn / appcat-service-postgresql

Add instance deletion #68

Summary

Checklist