Add Reaper Control Plane Deployment Mode #1388
Conversation
rzvoncek
force-pushed
the
radovan/reaper-control-plane-3
branch
from
a65db81
to
fbc98de
Compare
| return ErrNoReaperStorageRequest | ||
| } | ||
| } | ||
| if r.Spec.Reaper.StorageType == reaperapi.StorageTypeLocal && r.Spec.Reaper.DeploymentMode == reaperapi.DeploymentModePerDc { |
There was a problem hiding this comment.
I'm not sure if we need to enforce this here. Reaper won't come up, in this situation, so it's not the end of the world. On the other hand, it broke one e2e test, so perhaps it'll break stuff for users out there also.
There was a problem hiding this comment.
Funny it broke an e2e test given none of them should use local as storage mode 🤔
There was a problem hiding this comment.
I've actually added a local storage mode to one of the fixtures in the StatefulSet PR here. This is used by the CreateReaperAndDatacenter e2e test.
The idea of doing this was to cover the STS situation without adding a brand new e2e test, which I suppose we should try to avoid.
Here, for the CP mode, the change seems to me being big enough to warrant a new scenario tho.
rzvoncek
force-pushed
the
radovan/reaper-control-plane-3
branch
from
fbc98de
to
cc16a83
Compare
There was a problem hiding this comment.
This is coming to shape nicely!
I have a few suggestions that should hopefully be easy to implement in order to harmonize with how we do things elsewhere in the operator.
Could you create a ticket to update the docs as well, we'll need to make it easy for users to use this new feature.
Thanks!
| return ErrNoReaperStorageRequest | ||
| } | ||
| } | ||
| if r.Spec.Reaper.StorageType == reaperapi.StorageTypeLocal && r.Spec.Reaper.DeploymentMode == reaperapi.DeploymentModePerDc { |
There was a problem hiding this comment.
Funny it broke an e2e test given none of them should use local as storage mode 🤔
| @@ -41,6 +41,10 @@ func SuperuserSecretName(kc *api.K8ssandraCluster) string { | |||
| } | |||
|
|
|||
| func (r *K8ssandraClusterReconciler) reconcileReaperSecrets(ctx context.Context, kc *api.K8ssandraCluster, logger logr.Logger) result.ReconcileResult { | |||
| if kc.Spec.Reaper != nil && kc.Spec.Reaper.ReaperRef.Name != "" { | |||
| logger.Info("ReaperRef points to an existing Reaper, we don't need a new Reaper CQL secret") | |||
There was a problem hiding this comment.
suggestion: Same as before, I'm thinking the reference to an existing reaper isn't what drives the creation of a CQL user, but rather the storage attribute and the fact that we're using JMX (or not).
There was a problem hiding this comment.
I don't think it's as straightforward here. But perhaps I still don't understand things well enough.
We are here in the k8ssandra cluster reconciliation loop, and we're checking what the kc.reaper.Spec tells us.
We are deciding what to do with the CQL secret and the UI secret. We're not dealing with the JMX (except if the CQL and JMX secrets are one and the same).
We only want to reconcile(~create) the secrets if we're using a k8ssandra-cluster-specific Reaper.
We can have a k8ssandra-cluster-specific Reaper with local storage. In that case we don't need a CQL secret, only the UI secret. This is checked on L68.
I think that's why we cannot check for just the storage on L44. I'd really need the isControlPlane() function somewhere, but I cannot have that for other reasons. The ReaperRef presence is still my favourite alternative to that.
To sum up, I think I'd need clarification of:
- how the JMX secret comes into this?
- do we actually want to allow bundled Reapers with local storage?
There was a problem hiding this comment.
do we actually want to allow bundled Reapers with local storage?
Yes, why not? It's an efficient storage mechanism which doesn't require using Cassandra itself.
how the JMX secret comes into this?
The cases where we need Reaper to create users is for:
- authenticated access with JMX enabled
- using cassandra as storage for Reaper
One could have a centralized Reaper using JMX to communicate with the Cassandra clusters. It's not the default, but it's a possibility.
In this case, we still need a user and its secret to be created.
It is just a suggestion though and if that feels like some complications we want to deal with later, I'm good with that.
There was a problem hiding this comment.
do we actually want to allow bundled Reapers with local storage?
Yes, why not? It's an efficient storage mechanism which doesn't require using Cassandra itself.
I agree with this, so that's sorted.
About the JMX, I still don't get why we worry about it here when the reconcileReaperSecrets does not mention JMX secrets at all.
I'm indeed leaning towards not accepting this suggestion and keeping the condition as it is.
rzvoncek
force-pushed
the
radovan/reaper-control-plane-3
branch
2 times, most recently
from
1cf2b6a
to
4281da3
Compare
| if err := secret.ReconcileSecret(ctx, r.Client, cassandraUserSecretRef.Name, kcKey); err != nil { | ||
| logger.Error(err, "Failed to reconcile Reaper CQL user secret", "ReaperCassandraUserSecretRef", cassandraUserSecretRef) | ||
| return result.Error(err) | ||
| if kc.Spec.Reaper != nil && kc.Spec.Reaper.ReaperRef.Name == "" { |
There was a problem hiding this comment.
@adejanovski , I had to revert this back to the ReaperRef check.
If I checked only a storage type, then a bundled reaper that uses local storage was not working properly.
rzvoncek
force-pushed
the
radovan/reaper-control-plane-3
branch
from
8b8f0e6
to
259339b
Compare
rzvoncek
force-pushed
the
radovan/reaper-control-plane-3
branch
from
259339b
to
f7abe2f
Compare
|
What this PR does:
Which issue(s) this PR fixes:
Fixes #1277
Checklist