operator: various sidecar fixes #1138
Conversation
chrisseto
requested review from
RafalKorepta,
andrewstucki and
gene-redpanda
as code owners
operator/internal/controller/decommissioning/statefulset_decomissioner.go
Show resolved
Hide resolved
| var _ suite.SetupAllSuite = (*StatefulSetDecommissionerSuite)(nil) | ||
|
|
||
| func (s *StatefulSetDecommissionerSuite) TestDecommission() { | ||
| s.T().Skip("we currently have issues with the eviction code in this test due to pod disruption budgets") |
There was a problem hiding this comment.
This test was disabled quite sometime ago 😬
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
from
46f507b to
cc15fa3
Compare
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
from
cc15fa3 to
136df65
Compare
operator/internal/controller/decommissioning/statefulset_decomissioner.go
Show resolved
Hide resolved
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
from
136df65 to
dfae5b0
Compare
| // vectorizedDecommissionerAdapter is a helper struct that implements various methods | ||
| // of mapping StatefulSets through Vectorized Clusters to arguments for the | ||
| // StatefulSetDecommissioner. | ||
| type vectorizedDecommissionerAdapter struct { |
There was a problem hiding this comment.
The bulk of this code is the same. I've moved it into it's own file because the diff would otherwise be interleaved and abysmal to read.
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
2 times, most recently
from
27c9f52 to
11bdc24
Compare
andrewstucki
left a comment
andrewstucki
left a comment
There was a problem hiding this comment.
LGTM. Generally I'm fine with this, and thanks for looking into/fixing this. The one thing I'm 🤷 about is the use of the closure for the admin client v. delegating to the factory itself, but I understand why we might want to do it that way given that we're indirecting through the statefulset itself.
You know me, I like closures and dislike interfaces 😛. In this case specifically, opted for a closure "adapter" because the controller really only needs a single method Oh and in the case of the sidecar, there's no STS indirection. We're just loading the redpanda.yaml and ignoring the STS all together. |
|
I'm going to hold off on merging this until I get the vectorized test finished/passing so I can hopefully avoid and additional backport pains. |
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
from
11bdc24 to
9a5a3d0
Compare
Prior to this commit the operator sidecar's decommissioner and pvcunbinder controllers did not work. This was due to: - RBAC issues, the sidecar did not correctly scope itself to a single namespace. - Incorrect label selectors hidden within the controllers in question. Additionally, the statefulset decommissioner's sole test case has been disabled for quite sometime. There's been zero test coverage of this functionality. This commit: - Restores the decommissioner's tests to a working state - Strips out the "fetcher" to reduce duplication and remove reliance on fetching live helm values. - Replaces baked in filtering with a label selector argument that will be constructed by the helm chart. A follow up commit with chart changes and acceptance tests will be submitted. It's been made separate to ease the process of backporting to the v2.x.x branches.
chrisseto
force-pushed
the
chris/p/sidecar-fixes
branch
from
9a5a3d0 to
12f4fb2
Compare
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Prior to this commit the operator sidecar's decommissioner and pvcunbinder
controllers did not work. This was due to:
Additionally, the statefulset decommissioner's sole test case has been disabled
for quite sometime. There's been zero test coverage of this functionality.
This commit:
fetching live helm values.
constructed by the helm chart.
A follow up commit with chart changes and acceptance tests will be submitted.
It's been made separate to ease the process of backporting to the v2.x
branches.