✨ Install CAPI using operator after chart is installed #109

alexander-demicev · 2023-09-06T11:10:28Z

kind/feature

What this PR does / why we need it:

Add new values and template to helm chart so we can deploy CAPI with one command, assuming rancher and cert-manager are running users will be able to install extension, CAPI operator and CAPI like this:

helm repo add rancher-turtles https://rancher-sandbox.github.io/rancher-turtles
helm repo update
helm install rancher-turtles rancher-turtles/rancher-turtles --create-namespace -n rancher-turtles-system

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #18
Fixes #105

Special notes for your reviewer:

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

alexander-demicev · 2023-09-06T11:34:40Z

Should the kubeadm bootstrap and control plane also be included by default? or at least give an option to install them too. I'm fine with both approaches but would like to know what others think.

Danil-Grigorev · 2023-09-07T09:11:13Z

charts/rancher-turtles/values.yaml

+capi:
+  enabled: true
+  namespace: capi-system
+  configSecret:


Should this be included before 0.5.2 version is released?

v1alpha2 is for v0.6.0 we can reuse the same values, the difference will be in the provider manifest, we also have conversion webhooks to handle the transition.

Danil-Grigorev · 2023-09-07T09:13:19Z

test/e2e/e2e_suite_test.go

@@ -254,11 +254,50 @@ func initRancherTurtles(clusterProxy framework.ClusterProxy, config *clusterctl.
 	}
 	_, err := chart.Run(map[string]string{
 		"cluster-api-operator.cert-manager.enabled": "true",


As the proxy flags for capi have a different name, have you considered unified naming patter here?

you mean capi-operator vs cluster-api-operator?

I've just tried different approaches here, there will still be inconsistency. Helm doesn't like- character in go template so cluster-api.something is not an option, I changed it to clusterAPI and then cluster-api-operator.enabled to clusterAPIOperator.enabled, and if you want to pass values to capi operator chart you have to use chartname.something which is cluster-api-operator.something

I guess it's better to keep cluster-api-operator.enabled to match with other actions related to operator

Danil-Grigorev · 2023-09-07T09:17:44Z

test/e2e/e2e_suite_test.go

 	})
 	Expect(err).ToNot(HaveOccurred())
+


IMO this test case should be added in the CAPI operator repository (nit)

Can you elaborate? what exactly has to be tested in CAPI operator?

Is this supposed to be installed together with operator by the users? Why testing kubeadm provider here when it could be tested in the CAPI operator repository? Additionally with helm uninstall issue, this blocks other operations within e2e tests, it surely missing manual uninstall part in this specific test case. It is not scalable.

I will add a manual uninstall until this is fixed in operator

so after some thinking, I don't believe we have to uninstall manually providers in e2e, we tear down the entire management cluster and if we need to debug the test run and skip cleanup it's better to leave the providers running, wdyt?

This implementation fixes uninstall process in capi operator - kubernetes-sigs/cluster-api-operator#251 so no need to do some unnessesary cluster teardowns, we just need to merge it. Creating and tearing down management clusters is costly, not scalable and will prolong the e2e test execution.

Danil-Grigorev · 2023-09-07T09:20:57Z

README.md

+```
+### Installing CAPI providers
+
+The Rancher turtles extension does not install any CAPI providers, you will need to install them yourself using [CAPI operator](https://github.com/kubernetes-sigs/cluster-api-operator/tree/main/docs).


But you can, using infrastructure flag passed to helm values, no?

Not really, in this case, CAPI operator chart will deploy the core CAPI provider too. We tried to follow the same pattern as clusterctl here, our deployment model is a bit more complicated. What can help here is a clusterctl plugin for operator or alternatively rancher users will get a UI for providers

Danil-Grigorev · 2023-09-07T09:25:44Z

.github/workflows/test_chart.yaml

@@ -59,7 +59,7 @@ jobs:
        run: kind load docker-image ${{ env.MANIFEST_IMG }}:${{ env.TAG }}

      - name: Run chart-testing (install)
-        run: helm install rancher-turtles out/charts/rancher-turtles/ -n rancher-turtles-system --create-namespace --wait --set cluster-api-operator.cert-manager.enabled=true
+        run: helm install rancher-turtles out/charts/rancher-turtles/ -n rancher-turtles-system --create-namespace --wait --set cluster-api-operator.cert-manager.enabled=true --set capi.enabled=false


This workflow is the closest to quickstart process of the rancher turtles. Does this mean that capi operator will be disabled by default?

Uninstalling doesn't really work for us right now properly due to CAPI operator not cleaning up providers before removing the controller. We can fix it on the CAPI operator side(preferable way) or document that users have to remove all providers manually before deleting the chart.
I don't want CAPI operator blocking our progress on the extension so disabling CAPI is a temporary solution

We are planning a v0.6.0 release for operator so fix can be included there

Created kubernetes-sigs/cluster-api-operator#243 as I experienced this personally when trying out capi operator myself

richardcase · 2023-09-11T09:38:19Z

Should the kubeadm bootstrap and control plane also be included by default? or at least give an option to install them too. I'm fine with both approaches but would like to know what others think.

I think initially, let's just install the core controllers. We want to favour Rancher solutions so i think Kubeadm will always be an optional install.

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com> Install bootstrap and control plane providers using helm chart values Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

alexander-demicev · 2023-09-12T13:08:40Z

@richardcase @Danil-Grigorev please take a look again, all works in e2e

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

Danil-Grigorev · 2023-09-15T12:55:24Z

charts/rancher-turtles/templates/core-provider.yaml

@@ -0,0 +1,71 @@
+{{- if .Values.clusterAPI.enabled }}


I believe you’ve hit the issue with the naming here. I looked into this in the uninstall PR You can still use the
cluster-api-operator name like this:

Suggested change

{{- if .Values.clusterAPI.enabled }}

{{- if index .Values "cluster-api-operator" "enabled" }}

.github/workflows/test_chart.yaml

Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>

- Update test-chart workflow with capi operator values Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>

richardcase

Looking good!

alexander-demicev added the kind/enhancement Categorizes issue or PR as related to a new feature. label Sep 6, 2023

alexander-demicev requested a review from a team as a code owner September 6, 2023 11:10

alexander-demicev force-pushed the coreproviderchart branch from e75dac0 to e3550a7 Compare September 6, 2023 11:11

alexander-demicev mentioned this pull request Sep 6, 2023

Helm quick-start guide for operator, turtles and rancher rancher/turtles-docs#12

Merged

alexander-demicev force-pushed the coreproviderchart branch from e3550a7 to b56a7cc Compare September 6, 2023 11:26

alexander-demicev force-pushed the coreproviderchart branch 2 times, most recently from a93ab06 to 2e33429 Compare September 6, 2023 15:57

Danil-Grigorev reviewed Sep 7, 2023

View reviewed changes

alexander-demicev force-pushed the coreproviderchart branch from 2e33429 to 6cfa1f7 Compare September 8, 2023 09:19

alexander-demicev force-pushed the coreproviderchart branch 2 times, most recently from e99f3f0 to d46d397 Compare September 12, 2023 11:36

alexander-demicev added 2 commits September 12, 2023 15:02

Install CAPI using operator after chart is installed

692aefa

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com> Install bootstrap and control plane providers using helm chart values Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

Install CAPI and providers with operator in e2e

32f8885

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

alexander-demicev force-pushed the coreproviderchart branch 2 times, most recently from 009dbff to 9dafa89 Compare September 12, 2023 13:08

alexander-demicev requested review from richardcase and Danil-Grigorev September 12, 2023 13:08

Add dev docs about usage

d53167b

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>

alexander-demicev force-pushed the coreproviderchart branch from 9dafa89 to d53167b Compare September 12, 2023 15:51

Danil-Grigorev reviewed Sep 15, 2023

View reviewed changes

.github/workflows/test_chart.yaml Outdated Show resolved Hide resolved

Danil-Grigorev force-pushed the coreproviderchart branch from 1005895 to 000d85f Compare September 15, 2023 15:55

Danil-Grigorev changed the title ~~Install CAPI using operator after chart is installed~~ ✨ Install CAPI using operator after chart is installed Sep 15, 2023

Danil-Grigorev added the kind/feature label Sep 15, 2023

Danil-Grigorev force-pushed the coreproviderchart branch from 000d85f to 9fe4175 Compare September 15, 2023 16:01

Danil-Grigorev added 2 commits September 17, 2023 12:53

Use consistent naming with cluster-api-operator repo in helm charts

dd1f53e

Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>

Update readme describing use of capi-operator in turles

952bb8f

Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>

Update capi-operator value usage in e2e

a50bfec

- Update test-chart workflow with capi operator values Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>

Danil-Grigorev force-pushed the coreproviderchart branch from 9fe4175 to a50bfec Compare September 18, 2023 10:04

Danil-Grigorev approved these changes Sep 18, 2023

View reviewed changes

richardcase approved these changes Sep 18, 2023

View reviewed changes

richardcase merged commit 2d91005 into rancher:main Sep 18, 2023
8 checks passed

alexander-demicev deleted the coreproviderchart branch September 18, 2023 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Install CAPI using operator after chart is installed #109

✨ Install CAPI using operator after chart is installed #109

alexander-demicev commented Sep 6, 2023 •

edited by Danil-Grigorev

Loading

alexander-demicev commented Sep 6, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

alexander-demicev Sep 12, 2023

alexander-demicev Sep 12, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

alexander-demicev Sep 12, 2023

Danil-Grigorev Sep 15, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

Danil-Grigorev Sep 7, 2023

alexander-demicev Sep 7, 2023

alexander-demicev Sep 7, 2023

Danil-Grigorev Sep 7, 2023

richardcase commented Sep 11, 2023

alexander-demicev commented Sep 12, 2023

Danil-Grigorev Sep 15, 2023

richardcase left a comment

	{{- if .Values.clusterAPI.enabled }}
	{{- if index .Values "cluster-api-operator" "enabled" }}

✨ Install CAPI using operator after chart is installed #109

✨ Install CAPI using operator after chart is installed #109

Conversation

alexander-demicev commented Sep 6, 2023 • edited by Danil-Grigorev Loading

alexander-demicev commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Sep 11, 2023

alexander-demicev commented Sep 12, 2023

Choose a reason for hiding this comment

richardcase left a comment

Choose a reason for hiding this comment

alexander-demicev commented Sep 6, 2023 •

edited by Danil-Grigorev

Loading