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

Implement adoption of k8s clusters #725

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kylewuolle
Copy link
Contributor

@kylewuolle kylewuolle commented Dec 7, 2024

This PR resolves issue #542 by:

  1. Renaming ManagedCluster to ClusterDeployment
  2. Creation of a new template called adopted cluster that allows creating services via a sveltoscluster for existing k8s clusters not created via 2a

I've added a PR to update the documentation : Mirantis/project-2a-docs#68

@kylewuolle kylewuolle force-pushed the adopted-cluster branch 6 times, most recently from bc012d7 to fab454f Compare December 9, 2024 20:09
@kylewuolle kylewuolle requested review from wahabmk and a team December 9, 2024 20:27
@kylewuolle kylewuolle force-pushed the adopted-cluster branch 2 times, most recently from 6193f19 to cf72c8f Compare December 10, 2024 00:21
@zerospiel
Copy link
Member

@kylewuolle can the PR be split into two different PRs (or at least 2 different commits within the PR): the one with the renaming and the one with the adopted templates feature? Otherwise, there are too many changes to review and each requires attention because one might be either a simple renaming (which is pretty simple to review) or a feature (which requires extra attention)

@kylewuolle
Copy link
Contributor Author

@kylewuolle can the PR be split into two different PRs (or at least 2 different commits within the PR): the one with the renaming and the one with the adopted templates feature? Otherwise, there are too many changes to review and each requires attention because one might be either a simple renaming (which is pretty simple to review) or a feature (which requires extra attention)

Done!

Copy link
Member

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

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

Mostly, minor comments. I have a generic question: so the whole approach is to provide a template with an external (third-party, BYO) cluster, so it could be attached to the HMC but without handling any actions related to the provisioning and its managing, right?

I also have a concern regarding the provider name infrastracture-internal, I am not aware how it should be named in terms of CAPI, I'd consult with other team members regarding it

api/v1alpha1/managedcluster_types.go Show resolved Hide resolved
internal/controller/clusterdeployment_controller.go Outdated Show resolved Hide resolved
internal/telemetry/event.go Outdated Show resolved Hide resolved
api/v1alpha1/indexers.go Outdated Show resolved Hide resolved
templates/provider/hmc/templates/deployment.yaml Outdated Show resolved Hide resolved
test/objects/clusterdeployment/clusterdeployment.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
config/dev/adopted-clusterdeployment.yaml Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
@kylewuolle
Copy link
Contributor Author

Mostly, minor comments. I have a generic question: so the whole approach is to provide a template with an external (third-party, BYO) cluster, so it could be attached to the HMC but without handling any actions related to the provisioning and its managing, right?
That's right so the idea is essentially to be able to create a sveltos cluster to deploy beachhead services and also allow for observability etc of existing k8s clusters which 2a does not provision or manage.

I also have a concern regarding the provider name infrastracture-internal, I am not aware how it should be named in terms of CAPI, I'd consult with other team members regarding it

Open to ideas for sure. I wanted to stay away from infrastructure-hmc since we'll be renaming that so not sure what else would be appropriate.

zerospiel
zerospiel previously approved these changes Dec 13, 2024
Copy link
Member

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

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

LGTM, I'd wait for the ideas regarding the infrastructure-internal provider name and maybe update the ClusterDeployment aliases to something like cldeploy (the only alias, cld might potentially interfere with other resources one day) but it is a personal preference (clusterd sounds similar to a cluster daemon)

Copy link
Member

@eromanova eromanova left a comment

Choose a reason for hiding this comment

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

Mostly minor comments.
Ideally, before creating the ClusterDeployment with the adopted template, we should ensure that Sveltos is ready (because the template includes objects from the Sveltos API). For example, consider the template as invalid if the Sveltos is not ready yet (by the template controller) and then validate it in the webhook on a clusterdeployment creation, similarly to the current approach with the providers' readiness. But sveltos is not a provider and we probably should have a separate annotation for that and implement a separate logic in the controller. Anyway, It’s worth gathering more opinions on this approach.

docs/dev.md Outdated Show resolved Hide resolved
templates/cluster/adopted-cluster/Chart.yaml Outdated Show resolved Hide resolved
templates/cluster/adopted-cluster/values.yaml Outdated Show resolved Hide resolved
@kylewuolle kylewuolle force-pushed the adopted-cluster branch 2 times, most recently from ef5ac7f to 5b42254 Compare December 22, 2024 20:45
zerospiel
zerospiel previously approved these changes Dec 23, 2024
README.md Outdated Show resolved Hide resolved
api/v1alpha1/indexers.go Outdated Show resolved Hide resolved
@mestadler
Copy link

@Kshatrix Agreed, this looks good from my perspective, please go ahead.

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

Successfully merging this pull request may close these issues.

Create unmanaged cluster controller and crd
4 participants