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

Revert "Revert "Allow to modify namespace when creating/updating/listing custom resources"" #3940

Conversation

VinaySagarGonabavi
Copy link
Contributor

@VinaySagarGonabavi VinaySagarGonabavi commented Aug 27, 2024

This reverts commit c6e7bd5.

Context:
Three types of custom resources, VitessCluster, VitessCell, and VitessKeyspace needs to be deployed in the same K8s namespace, paasta-vitesscluster. This change is a workaround to specify a namespace to perform k8s custom resource operations (create/update/list/delete)

What are the changes?
I added additional namespace argument to the core kubernetes_tools functions to make k8s api calls for custom objects. The default value when the argument is not passed is still set to the previous value.

Are these changes safe?
Since the default behavior of these functions don't change, and only is changed when explicitly passing a namespace value (only done in case of vitess custom resource operations), this change can be considered safe

Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm a little hesitant to go this route - i think that if we need to do manage multiple CRs from paasta, we'd probably want to make paasta multi-CR/CRD aware in a more complete way (e.g., revisit #3778 or #3777 and go from there)

paasta_tools/cleanup_kubernetes_cr.py Outdated Show resolved Hide resolved
paasta_tools/setup_kubernetes_cr.py Outdated Show resolved Hide resolved
paasta_tools/vitesskeyspace_tools.py Outdated Show resolved Hide resolved
@VinaySagarGonabavi
Copy link
Contributor Author

i'm a little hesitant to go this route - i think that if we need to do manage multiple CRs from paasta, we'd probably want to make paasta multi-CR/CRD aware in a more complete way (e.g., revisit #3778 or #3777 and go from there)

IMO, the two PRs are trying to solve a different problem for CRD's added to yelpsoa while vitess crds are deployed with flux?
At least for this situation, the problem we're trying to solve is to have the same namespace for different CR's created using different file prefixes https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/48754 and https://github.yelpcorp.com/sysgit/puppet/pull/12896

@nemacysts
Copy link
Member

i'm a little hesitant to go this route - i think that if we need to do manage multiple CRs from paasta, we'd probably want to make paasta multi-CR/CRD aware in a more complete way (e.g., revisit #3778 or #3777 and go from there)

IMO, the two PRs are trying to solve a different problem for CRD's added to yelpsoa while vitess crds are deployed with flux? At least for this situation, the problem we're trying to solve is to have the same namespace for different CR's created using different file prefixes https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/48754 and https://github.yelpcorp.com/sysgit/puppet/pull/12896

sorry, what i meant was that rather than hack around the problem of multiple CRs/CRDs for a workload how we've been doing it (e.g., managing some stuff in flux, having some vitess-specific CR code here, splitting vitess up like i just saw in #3935, etc) we should build this support in in a more native way rather than essentially special-casing vitess

we'd been discussing the vitess setup a bit somewhat recently in CI and @jfongatyelp had an interesting idea re: having something like a "wrapper" operator that creates the vitess CRs as well as supporting objects like HPAs and whatnot that then the real™ vitess operator handles as it would normally might be more maintainable for both CI and DRE. this is still kinda complex and will probably trip folks up at least once, but it might be better than adding all these special-case bits of logic for vitess if we don't/can't make paasta multi-CR/CRD-aware

@nemacysts
Copy link
Member

note: my comments in the original CEP about trying to figure out what the desired vitess soaconfigs would look like were to try to figure out what would need to be done to have paasta support running vitess as a native workload and attempt to frontload this discussion :)

@VinaySagarGonabavi
Copy link
Contributor Author

i'm a little hesitant to go this route - i think that if we need to do manage multiple CRs from paasta, we'd probably want to make paasta multi-CR/CRD aware in a more complete way (e.g., revisit #3778 or #3777 and go from there)

IMO, the two PRs are trying to solve a different problem for CRD's added to yelpsoa while vitess crds are deployed with flux? At least for this situation, the problem we're trying to solve is to have the same namespace for different CR's created using different file prefixes https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/48754 and https://github.yelpcorp.com/sysgit/puppet/pull/12896

sorry, what i meant was that rather than hack around the problem of multiple CRs/CRDs for a workload how we've been doing it (e.g., managing some stuff in flux, having some vitess-specific CR code here, splitting vitess up like i just saw in #3935, etc) we should build this support in in a more native way rather than essentially special-casing vitess

we'd been discussing the vitess setup a bit somewhat recently in CI and @jfongatyelp had an interesting idea re: having something like a "wrapper" operator that creates the vitess CRs as well as supporting objects like HPAs and whatnot that then the real™ vitess operator handles as it would normally might be more maintainable for both CI and DRE. this is still kinda complex and will probably trip folks up at least once, but it might be better than adding all these special-case bits of logic for vitess if we don't/can't make paasta multi-CR/CRD-aware

Definitely agree that we're special casing for vitess in some of the paasta changes (file prefix per cr, custom instance config loading, and now namespaces). Definitely a good idea to have a wrapper to accommodate such things in the long run. This is a larger issue and happy to discuss more on this. But, at least the priority for now has been to deploy it right with as minimal changes as possible to the core paasta code.

@VinaySagarGonabavi
Copy link
Contributor Author

Discontinuing this change as the paasta playground testing results showed that a split down Vitess Deployment cannot coordinate the topology even with https://github.com/planetscale/vitess-operator/blob/c85f611722ae24f7ce68a30db0b39170ee182aef/docs/architecture/federation.md#topology-reconciliation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants