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

Create the additional seeds service only if necessary #347

Open
adejanovski opened this issue Jun 13, 2022 · 18 comments
Open

Create the additional seeds service only if necessary #347

adejanovski opened this issue Jun 13, 2022 · 18 comments
Assignees
Labels
assess Issues in the state 'assess'

Comments

@adejanovski
Copy link
Contributor

adejanovski commented Jun 13, 2022

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

@sync-by-unito sync-by-unito bot changed the title Create the additional seeds service only if necessary K8SSAND-1560 ⁃ Create the additional seeds service only if necessary Jun 13, 2022
@burmanm
Copy link
Contributor

burmanm commented Jun 13, 2022

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

@jsanda
Copy link
Contributor

jsanda commented Jun 23, 2022

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

@jsanda
Copy link
Contributor

jsanda commented Jun 23, 2022

Please add your planning poker estimate with ZenHub @burmanm

@adejanovski adejanovski added zh:Product-Backlog Issues in the ZenHub pipeline 'Product-Backlog' and removed zh:Assess/Investigate labels Jun 23, 2022
@adejanovski adejanovski added zh:Product-Backlog Issues in the ZenHub pipeline 'Product-Backlog' and removed zh:Product-Backlog Issues in the ZenHub pipeline 'Product-Backlog' labels Aug 30, 2022
@Miles-Garnsey Miles-Garnsey self-assigned this Oct 27, 2022
@adejanovski adejanovski added zh:Ready and removed zh:Product-Backlog Issues in the ZenHub pipeline 'Product-Backlog' labels Oct 27, 2022
@Miles-Garnsey
Copy link
Member

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.

@jsanda
Copy link
Contributor

jsanda commented Oct 28, 2022

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

@Miles-Garnsey
Copy link
Member

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?

@burmanm
Copy link
Contributor

burmanm commented Oct 28, 2022

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.

@jsanda
Copy link
Contributor

jsanda commented Nov 7, 2022

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.

@adejanovski adejanovski moved this to Ready in K8ssandra Nov 8, 2022
@adejanovski adejanovski moved this from Ready to Assess/Investigate in K8ssandra Nov 16, 2022
@Miles-Garnsey
Copy link
Member

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

@jsanda
Copy link
Contributor

jsanda commented Dec 14, 2022

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.

@Miles-Garnsey Miles-Garnsey moved this from Assess/Investigate to Ready in K8ssandra Dec 14, 2022
@adejanovski adejanovski added the ready Issues in the state 'ready' label Dec 14, 2022
@Miles-Garnsey
Copy link
Member

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

@burmanm
Copy link
Contributor

burmanm commented Dec 14, 2022

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.

@Miles-Garnsey
Copy link
Member

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.

@jsanda
Copy link
Contributor

jsanda commented Dec 14, 2022

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

@Miles-Garnsey
Copy link
Member

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...

@jsanda
Copy link
Contributor

jsanda commented Dec 15, 2022

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

@Miles-Garnsey
Copy link
Member

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.

@adejanovski
Copy link
Contributor Author

Alternatively, could we point the additional-seed-service to use the same endpoint as the seed-service if it's empty?
This would remove the unnecessary noise without removing legit warnings in the logs.

@adejanovski adejanovski moved this from Ready to Assess/Investigate in K8ssandra Sep 3, 2024
@adejanovski adejanovski added assess Issues in the state 'assess' and removed ready Issues in the state 'ready' labels Sep 3, 2024
@sync-by-unito sync-by-unito bot changed the title K8SSAND-1560 ⁃ Create the additional seeds service only if necessary Create the additional seeds service only if necessary Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assess Issues in the state 'assess'
Projects
No open projects
Status: Assess/Investigate
Development

No branches or pull requests

4 participants