cass-operator icon indicating copy to clipboard operation
cass-operator copied to clipboard

Create the additional seeds service only if necessary

Open adejanovski opened this issue 3 years ago • 18 comments

Following up on this issue, it would be nice to avoid creating the additionalSeeds service unless it's necessary.

Quote from the original issue:

Current additional-seeds-service allows modifying external seeds without causing STS restart, and even without modifying the CassandraDatacenter object at all. The path to multicluster-services allows to modify the service to THE external path to seeds, without cass-operator needing to do anything. That seems to be the way in multicluster-services Kubernetes sig also, keeping single-dc targets detached from the actual replication. cass-operator can read new IPs from that service without itself modifying it. Maybe one could disable / enable that feature by modifying "additionalSeeds" to *[]string or something and check if it's empty, create the service and if it wasn't set, don't create the service.

┆Issue is synchronized with this Jira Story by Unito ┆Issue Number: CASS-39

adejanovski avatar Jun 13 '22 13:06 adejanovski

This was the old behavior, but it was modified for the k8ssandra-operator requirements.

burmanm avatar Jun 13 '22 14:06 burmanm

We can revert to the old behavior since k8ssandra-operator is managing the endpoints for the additional-seeds-service.

jsanda avatar Jun 23 '22 03:06 jsanda

Please add your planning poker estimate with ZenHub @burmanm

jsanda avatar Jun 23 '22 03:06 jsanda

From a design perspective, there's only one question here. Are we happy with the concept that, if a user adds additionalSeeds to a running cluster, it will trigger a rolling restart of the existing nodes?

I don't think there is a way around this given that the additional seeds service will not exist until it is strictly required. It therefore won't be able to appear in the list of seeds maintained by Cassandra. So there will be some cassandra.yaml work here too.

Miles-Garnsey avatar Oct 28 '22 06:10 Miles-Garnsey

There shouldnt be any new work for changes to cassandra.yaml.

jsanda avatar Oct 28 '22 06:10 jsanda

There shouldnt be any new work for changes to cassandra.yaml.

Don't the additional seeds need to be added/not added according to whether the service exists?

Miles-Garnsey avatar Oct 28 '22 07:10 Miles-Garnsey

Don't the additional seeds need to be added/not added according to whether the service exists?

Yes and that requires a change and rolling restart. This was also the original reason the service always exists, so that adding a DC wouldn't cause the original DC to restart.

burmanm avatar Oct 28 '22 10:10 burmanm

It does require a rolling restart. Can't we add additional seeds up front if we are dealing with a K8ssandraCluster deployment so as to avoid rolling restarts? Then don't add additional seeds otherwise to avoid the warnings in the logs which confuse users.

jsanda avatar Nov 07 '22 13:11 jsanda

Can I get an update on whether we want to proceed with this ticket @adejanovski and @jsanda?

Miles-Garnsey avatar Dec 14 '22 04:12 Miles-Garnsey

I do want to proceed. The warnings in the logs is source of confusion that comes up frequently for users only using cass-operator.

We just need to make sure to preserve existing behavior for k8ssandra-operator. That is, for a K8ssandraCluster the additional seeds service should be created up front.

jsanda avatar Dec 14 '22 04:12 jsanda

Awesome, moving this to ready so I can get it onto my todo list.

Miles-Garnsey avatar Dec 14 '22 04:12 Miles-Garnsey

I do want to proceed. The warnings in the logs is source of confusion that comes up frequently for users only using cass-operator.

This is something that could be fixed in the mgmt-api also, since it iterates over the service targets. If there's no IPs, then it should not add them as seeds either.

Only complain if none of the targets have any IPs.

burmanm avatar Dec 14 '22 05:12 burmanm

That's a good point actually, we're probably going to see errors in mgmt api I assume if it goes looking for a non-existent service... Might need to make changes there too.

Miles-Garnsey avatar Dec 14 '22 06:12 Miles-Garnsey

Couldn't it be fixed with changes only in management-api as @burmanm suggested?

jsanda avatar Dec 14 '22 13:12 jsanda

The ticket says [Create the additional seeds service only if necessary](https://github.com/k8ssandra/cass-operator/issues/347#top), which is created by cass-operator, so I don't imagine so...

Miles-Garnsey avatar Dec 15 '22 00:12 Miles-Garnsey

The motivation for the ticket is to avoid the warnings. If we can just do the change in the management-api and avoid the warnings then that would address the ultimate goal. We

jsanda avatar Dec 15 '22 00:12 jsanda

So this ticket was raised as a follow up from this one which I raised. MY motivation was originally about simplifying the services deployed, but it sounds like maybe folks are looking for a variety of different things here.

I'm sure we can suppress the warnings in management API if that's preferred, we should just update this ticket name to reflect what we're really trying to achieve.

Miles-Garnsey avatar Dec 15 '22 01:12 Miles-Garnsey