-
Notifications
You must be signed in to change notification settings - Fork 16
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
✨ Install CAPI using operator after chart is installed #109
✨ Install CAPI using operator after chart is installed #109
Conversation
e75dac0
to
e3550a7
Compare
e3550a7
to
b56a7cc
Compare
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. |
a93ab06
to
2e33429
Compare
charts/rancher-turtles/values.yaml
Outdated
capi: | ||
enabled: true | ||
namespace: capi-system | ||
configSecret: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be included before 0.5.2 version is released?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/e2e/e2e_suite_test.go
Outdated
@@ -254,11 +254,50 @@ func initRancherTurtles(clusterProxy framework.ClusterProxy, config *clusterctl. | |||
} | |||
_, err := chart.Run(map[string]string{ | |||
"cluster-api-operator.cert-manager.enabled": "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the proxy flags for capi have a different name, have you considered unified naming patter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean capi-operator vs cluster-api-operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's better to keep cluster-api-operator.enabled
to match with other actions related to operator
}) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this test case should be added in the CAPI operator repository (nit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? what exactly has to be tested in CAPI operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a manual uninstall until this is fixed in operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
``` | ||
### 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you can, using infrastructure flag passed to helm values, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.github/workflows/test_chart.yaml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow is the closest to quickstart process of the rancher turtles. Does this mean that capi operator will be disabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are planning a v0.6.0 release for operator so fix can be included there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created kubernetes-sigs/cluster-api-operator#243 as I experienced this personally when trying out capi operator myself
2e33429
to
6cfa1f7
Compare
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. |
e99f3f0
to
d46d397
Compare
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>
009dbff
to
9dafa89
Compare
@richardcase @Danil-Grigorev please take a look again, all works in e2e |
Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>
9dafa89
to
d53167b
Compare
@@ -0,0 +1,71 @@ | |||
{{- if .Values.clusterAPI.enabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
{{- if .Values.clusterAPI.enabled }} | |
{{- if index .Values "cluster-api-operator" "enabled" }} |
1005895
to
000d85f
Compare
000d85f
to
9fe4175
Compare
Signed-off-by: Danil Grigorev <danil.grigorev@suse.com>
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>
9fe4175
to
a50bfec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
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:
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: