✨ Add Cluster API Visualizer to Tilt observability #6593

Jont828 · 2022-06-03T23:07:29Z

What this PR does / why we need it: This PR adds the Cluster API Visualizer app to Tilt under the observability section. It can be enabled by adding "cluster-api-visualizer" to the "deploy_observability" array. The Tilt UI looks as follows and the "View visualization" link opens it in the browser.

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 #

Jont828 · 2022-06-03T23:15:27Z

@killianmuldoon PTAL when you get the chance. I set it up so we read from the chart directory produced by tilt-prepare so far. The secret in the Helm chart requires the kubeconfig, which is computed at runtime, to be set as a value in the Helm chart. I tried to use the yaml output in .tiltbuild but it's not very straightforward to try to get the secret working.

fabriziopandini

Overall lgtm, only a couple of nits from my side, mostly to:

drop cluster-api in front of cluster-api-visualizer
use visualizer consistently (thus dropping the "visualize-cluster" variants).

Also, please add the new option in https://main.cluster-api.sigs.k8s.io/developer/tilt.html#create-a-tilt-settings-file where allowed values for deploy_observability are documented.

Tiltfile

killianmuldoon

/lgtm
/hold
@Jont828 Thanks for this! could you squash the commits when you get a chance?

Jont828 · 2022-06-29T21:12:51Z

@killianmuldoon Just squashed!

ykakarap

I tried this locally and looks like there is a bug.

If the list of deploy_observability only has visualizer in it then the visualizer resource fails with the following error:

Build Failed: namespaces "observability" not found

Seems to be working fine if any of the other observability items are in the list.

It would be nice to be able to use just the visualizer without having to deploy the other items as well.

Tiltfile

Jont828 · 2022-06-30T22:25:15Z

@ykakarap Just pushed some changes to create the observability namespace before deploying the visualizer chart. I tested it locally but let me know if it works on your end!

ykakarap

@ykakarap Just pushed some changes to create the observability namespace before deploying the visualizer chart. I tested it locally but let me know if it works on your end!

Tested it and it is working as expected.

/lgtm

Great work on this!

hack/observability/visualizer/values.yaml

sbueringer · 2022-07-01T09:21:40Z

hack/observability/visualizer/values.yaml

+  key: app
+  value: visualizer
+
+kubeconfig: "\"${KUBECONFIG_DATA}\""


How does this work?

Maybe I'm doing something wrong but the visualizer doesn't work out-of-the box on my machine

The tilt-prepare code wants to build the Helm chart into a YAML into .tiltbuild, but I had trouble getting that working. Instead, I'm not really using this values.yaml file and I'm using the helm() function in the Tiltfile to load the Helm chart. What's the error you're getting?

The tilt-prepare code wants to build the Helm chart into a YAML into .tiltbuild, but I had trouble getting that working.

That works for me. What error are you getting?
I would really prefer deploying this Helm Chart the same way we deploy all others.

What's the error you're getting?

I was trying to deploy the Helm Chart like the others. Which worked, but then kubeconfig was not set correctly, which kind of makes sense :)

WDYT about if we try to resolve this issue #6286 instead of passing in a kubeconfig via values / a Secret? It's just such an anti-pattern to use a kubeconfig from a Secret instead of a ServiceAccount with RBAC.

+1 to resolve #6228 first if this can avoid differences with other charts

I think the idea would be to introduce InClusterConfig() into the Visualizer. That would allow the binary access to the kubeconfig associated with the pod's service account.

The visualizer is using clusterctl as a library IIRC. Also IIRC the clusterctl lib currently only takes a kubeconfig as input which is something that we should probably fix/improve (I didn't take a closer look yet, will do that later/soon)

@Jont828 I think I have an idea how the visualizer could already have a good kubeconfig/inCluster discovery behavior even before clusterctl supports it.

If it's okay for you I can do a POC and open a PR for the visualizer. WDYT?

Sure thing! I'm happy to pair on this sometimes as well if that's easier.

I think there are two ways this can be resolved:

Workaround the current clusterctl implementation and use the clusterctl lib as is

Refactor clusterctl to e.g. take a rest.Config as parameter instead of a kubeconfig file

I think 1. can be a short-term solution and a quick win. 2. probably requires more time/discussion (I also commented one the issue).

I think 1. could be done like this:

On a high level in the visualizer main.go

restConfig := ctrl.GetConfigOrDie() # write restConfig in kubeconfig format to kubeconfigPath file c.ClusterClient = cluster.New(cluster.Kubeconfig{Path: kubeconfigPath}, configClient)

In my opinion GetConfigOrDie from controller-runtime has a very nice behavior and it would have the added benefit that the visualizer handles kubeconfig like every other controller. It's documented in detail here: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/config/config.go#L63-L91

Once 2. is implemented we can move the visualizer away from the workaround.

Thx for accepting the hacky workaround :). We should really improve the clusterctl lib so that you can get rid of it again.

sbueringer · 2022-07-13T04:52:24Z

Works for me.
Great work!

lgtm pending squash

Disclaimer: I had to delete the hack/observability/visualizer/charts folder manually, otherwise the new version of the chart wasn't pulled. I think that's probably because helm couldn't tell that the version changed based on the way it's hosted.
But I don't want to block the PR because of this.

sbueringer · 2022-07-13T04:53:32Z

cc @fabriziopandini @ykakarap @killianmuldoon PTAL

fabriziopandini · 2022-07-13T19:24:47Z

hack/observability/visualizer/values.yaml

+  # Overrides the image tag whose default is the chart appVersion.
+  tag: "v0.1.0-alpha.2"


I'm ok with this, but in the future, I would like the chart to always give us the latest version, so we are not required to keep this value up to date; see Jont828/cluster-api-visualizer#6

Sounds good, I'm working towards a v1 of the app with the clusterctl changes from CAPI 1.2 so I'll rework the Helm chart then.

fabriziopandini · 2022-07-13T19:29:56Z

Looks great! lgtm pending squash

sbueringer · 2022-07-14T05:39:12Z

/lgtm

killianmuldoon

/lgtm

Great work! 🙂

fabriziopandini · 2022-07-14T09:45:32Z

/lgtm
/approve

k8s-ci-robot · 2022-07-14T09:45:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbueringer · 2022-07-14T11:49:31Z

/hold cancel

Jont828 · 2022-07-14T19:56:04Z

Thanks to everyone for helping me test this out!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2022

k8s-ci-robot requested review from fabriziopandini and vincepri June 3, 2022 23:07

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 3, 2022

fabriziopandini reviewed Jun 13, 2022

View reviewed changes

Tiltfile Outdated Show resolved Hide resolved

Tiltfile Outdated Show resolved Hide resolved

Tiltfile Outdated Show resolved Hide resolved

Tiltfile Outdated Show resolved Hide resolved

Jont828 changed the title ~~✨ Add Cluster API Visualizer to Tilt observability [WIP]~~ ✨ Add Cluster API Visualizer to Tilt observability Jun 28, 2022

Jont828 force-pushed the tilt-visualizer branch from 0783f67 to f069e83 Compare June 28, 2022 22:40

killianmuldoon approved these changes Jun 29, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2022

k8s-ci-robot assigned killianmuldoon Jun 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2022

Jont828 force-pushed the tilt-visualizer branch from f069e83 to 83f3db6 Compare June 29, 2022 21:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2022

ykakarap reviewed Jun 30, 2022

View reviewed changes

killianmuldoon reviewed Jun 30, 2022

View reviewed changes

Tiltfile Outdated Show resolved Hide resolved

Jont828 force-pushed the tilt-visualizer branch from 83f3db6 to c8e4384 Compare June 30, 2022 22:24

ykakarap approved these changes Jul 1, 2022

View reviewed changes

k8s-ci-robot assigned ykakarap Jul 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2022

sbueringer reviewed Jul 1, 2022

View reviewed changes

Jont828 force-pushed the tilt-visualizer branch from c8e4384 to 0474056 Compare July 1, 2022 21:56

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2022

Jont828 force-pushed the tilt-visualizer branch from 0474056 to d23c9b5 Compare July 12, 2022 22:15

fabriziopandini reviewed Jul 13, 2022

View reviewed changes

Add visualizer app to CAPI Tilt

835c589

Jont828 force-pushed the tilt-visualizer branch from d23c9b5 to 835c589 Compare July 13, 2022 22:24

k8s-ci-robot assigned sbueringer Jul 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2022

killianmuldoon approved these changes Jul 14, 2022

View reviewed changes

k8s-ci-robot assigned fabriziopandini Jul 14, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2022

k8s-ci-robot merged commit e5f7e84 into kubernetes-sigs:main Jul 14, 2022

k8s-ci-robot added this to the v1.3 milestone Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Cluster API Visualizer to Tilt observability #6593

✨ Add Cluster API Visualizer to Tilt observability #6593

Jont828 commented Jun 3, 2022

Jont828 commented Jun 3, 2022

fabriziopandini left a comment •

edited

Loading

killianmuldoon left a comment

Jont828 commented Jun 29, 2022

ykakarap left a comment

Jont828 commented Jun 30, 2022

ykakarap left a comment

sbueringer Jul 1, 2022

sbueringer Jul 1, 2022

Jont828 Jul 1, 2022

sbueringer Jul 4, 2022 •

edited

Loading

fabriziopandini Jul 5, 2022

sbueringer Jul 6, 2022 •

edited

Loading

sbueringer Jul 6, 2022 •

edited

Loading

Jont828 Jul 6, 2022

sbueringer Jul 7, 2022 •

edited

Loading

sbueringer Jul 13, 2022

sbueringer commented Jul 13, 2022

sbueringer commented Jul 13, 2022

fabriziopandini Jul 13, 2022 •

edited

Loading

Jont828 Jul 13, 2022

fabriziopandini commented Jul 13, 2022

sbueringer commented Jul 14, 2022

killianmuldoon left a comment

fabriziopandini commented Jul 14, 2022

k8s-ci-robot commented Jul 14, 2022

sbueringer commented Jul 14, 2022

Jont828 commented Jul 14, 2022

		# Overrides the image tag whose default is the chart appVersion.
		tag: "v0.1.0-alpha.2"

✨ Add Cluster API Visualizer to Tilt observability #6593

✨ Add Cluster API Visualizer to Tilt observability #6593

Conversation

Jont828 commented Jun 3, 2022

Jont828 commented Jun 3, 2022

fabriziopandini left a comment • edited Loading

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

Jont828 commented Jun 29, 2022

ykakarap left a comment

Choose a reason for hiding this comment

Jont828 commented Jun 30, 2022

ykakarap left a comment

Choose a reason for hiding this comment

sbueringer Jul 1, 2022

Choose a reason for hiding this comment

sbueringer Jul 1, 2022

Choose a reason for hiding this comment

Jont828 Jul 1, 2022

Choose a reason for hiding this comment

sbueringer Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Jul 5, 2022

Choose a reason for hiding this comment

sbueringer Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Jont828 Jul 6, 2022

Choose a reason for hiding this comment

sbueringer Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 13, 2022

Choose a reason for hiding this comment

sbueringer commented Jul 13, 2022

sbueringer commented Jul 13, 2022

fabriziopandini Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Jont828 Jul 13, 2022

Choose a reason for hiding this comment

fabriziopandini commented Jul 13, 2022

sbueringer commented Jul 14, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 14, 2022

k8s-ci-robot commented Jul 14, 2022

sbueringer commented Jul 14, 2022

Jont828 commented Jul 14, 2022

fabriziopandini left a comment •

edited

Loading

sbueringer Jul 4, 2022 •

edited

Loading

sbueringer Jul 6, 2022 •

edited

Loading

sbueringer Jul 6, 2022 •

edited

Loading

sbueringer Jul 7, 2022 •

edited

Loading

fabriziopandini Jul 13, 2022 •

edited

Loading