tracing: new jaeger independent helm chart #5240

Pothulapati · 2020-11-17T08:25:41Z

This PR moves tracing into a jaeger chart with no proxy injection
templates. We still keep the dependency on partials, as we could use
common templates like resources, etc from there.

Signed-off-by: Tarun Pothulapati [email protected]

This PR moves tracing into a jaeger chart with no proxy injection templates. We still keep the dependency on partials, as we could use common templates like resources, etc from there. Signed-off-by: Tarun Pothulapati <[email protected]>

adleong

Thanks @Pothulapati, this is a great start! I think we'll want to move everything, including the chart, into the top level jaeger directory.

I noticed that you left the original tracing chart alone in this PR which I think is the right approach. There will need to be follow-up work to switch the main control plane to send traces to this collector instead and then to eventually delete the old tracing addon chart.

charts/jaeger/Chart.yaml

charts/jaeger/templates/tracing-rbac.yaml

jaeger/cmd/root.go

jaeger/values/values.go

Adds a new jaeger high level folder which contains the cli code. This contains its own `Values` based on `jaeger/values.yaml`. It re-uses a lot of code from cli install, and follows the code format and logic. A lot of code is copy pasted, and could have a refactor commit, to make it re-usable. Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

Also, added jaeger deps to the gitignore so that we are not storing dependencies in the repo Signed-off-by: Tarun Pothulapati <[email protected]>

Adds a new `Fs` field in `Chart` struct to represne the file system which should be used for the rendering of files. This is used with jaeger and other extensions to be able to render charts from different top level directories. Signed-off-by: Tarun Pothulapati <[email protected]>

alpeb

Looking good 👍
FYI, I'll be implementing the webhook on top of this branch.

Do you mind completing the entries in bin/helm-build for this new chart?

.gitignore

jaeger/charts/jaeger/templates/tracing.yaml

alpeb · 2020-11-18T15:30:45Z

jaeger/cmd/root.go

+	RootCmd.PersistentFlags().StringVarP(&controlPlaneNamespace, "linkerd-namespace", "L", defaultLinkerdNamespace, "Namespace in which Linkerd is installed [$LINKERD_NAMESPACE]")
+	RootCmd.PersistentFlags().StringVar(&kubeconfigPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests")
+	RootCmd.PersistentFlags().StringVar(&kubeContext, "context", "", "Name of the kubeconfig context to use")
+	RootCmd.PersistentFlags().StringVar(&impersonate, "as", "", "Username to impersonate for Kubernetes operations")
+	RootCmd.PersistentFlags().StringArrayVar(&impersonateGroup, "as-group", []string{}, "Group to impersonate for Kubernetes operations")
+	RootCmd.PersistentFlags().StringVar(&apiAddr, "api-addr", "", "Override kubeconfig and communicate directly with the control plane at host:port (mostly for testing)")


I didn't see controlPlaneNamespace nor apiAddr being used. Any plans on using them later on?

Yeah, controlPlaneNamespace is removed but other k8s related flags should be useful for the check stuff.

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong

Since this new chart is unused by the current CLI, merging this should be a safe thing to do. Let's prioritize getting the issues raised on this PR fixed up so that we can merge this to main before adding more functionality (such as validation, new flags, etc.). We can then work on that stuff in follow-up PRs.

adleong · 2020-11-19T00:32:37Z

pkg/charts/charts.go

@@ -22,14 +23,16 @@ type Chart struct {
 	Namespace string
 	RawValues []byte
 	Files     []*chartutil.BufferedFile
+	Fs        http.FileSystem


It looks like Fs and Dir are always combined. Any reason we can't just turn the Dir field into a http.FileSystem so that we don't need both? (I think os.File would probably be better, but that's a larger change)

This feels better, as not all code paths have to configure the filesystem field, they just use static.Templates which is the default i.e /charts and set the Dir based on their chart name.

If we combine them, Every Chart creator code-path will have to configure the http.FileSystem field to their chart directory. WDYT?

adleong · 2020-11-19T00:42:39Z

jaeger/charts/jaeger/templates/rbac.yaml

+metadata:
+  name: collector
+  namespace: {{.Values.namespace}}
+  labels:


what is the purpose of these labels?

These are not helpful for serviceAccounts, but I kept them as we had them for services and deployments.

Removing them.

jaeger/charts/jaeger/values.yaml

Signed-off-by: Tarun Pothulapati <[email protected]>

This branch removes dir field and makes users only specify the full path in `Fs` field. Helper functions are added to make this easy. Changes are also made in charts generation code to remove the passage of `Charts.yaml` for dependency charts i.e partials, etc as it confuses the generation. Only the top level charts will pass the `Chart.yaml` and for every other dep chart they only pass the templated files for rendering. Signed-off-by: Tarun Pothulapati <[email protected]>

alpeb · 2020-11-20T20:01:35Z

pkg/charts/charts.go

-		filename = filename[7:]
-	}
-	file, err := static.Templates.Open(filename)
+	file, err := fs.Open(filename)


Here you're reading directly from fs. When doing linkerd install, this ends up being static.WithDefaultChart(helmDefaultChartDir) which points directly to the "real" file system.
Before, we had static.Templates.Open(filename) which hits the "real" file system when !prod and the VFS when prod. So this is why it's failing the integration test (trying to hit the real FS instead of the VFS).

Yep, That makes sense, As static.Templates is replaced by the logic to read from a VFS with code generation which our helper code is not doing similarly.

adleong

Lets push the CLI and chart rendering stuff out of this PR so that we can merge the chart itself.

Then we'll need to open a new PR for the chart rendering changes. I didn't know about the VFS stuff we do here. I think the jaeger CLI package will need a function similar to the main CLI's static.Templates which returns a FileSystem which may be either a VFS of it charts directory or may point to the real FS.

Pothulapati · 2020-11-23T10:07:15Z

@adleong I am reverting the last commit, which involved the merging of c,Fs and c.DIr which added all this complexity.

I was trying to create a separate VFS generation specifically for jaeger chart by defining a JaegerTemplates but even that fails because of shurcooL/vfsgen#23

For Now, I will move jaeger chart to higher level charts directory so that even render works. We can raise separate PRs to fix this. WDYT?

This reverts commit 3631351. Signed-off-by: Tarun Pothulapati <[email protected]>

Currently, Our VFS is tied to the higher level `chats` directory Because of the limitation of the VFSGen project, It does not seem possible to generate multiple variables in the same pkg. This can be solved in the future PRs Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

alpeb

LGTM 👍 , looking forward for the followup PRs 😉

adleong · 2020-11-23T21:52:06Z

I've branched off of this PR to create #5275 as a replacement which moves the jaeger charts out of the main charts directory as we discussed. It deletes the jaeger CLI for now.

Pothulapati · 2020-11-24T12:09:45Z

Created #5278 with only the CLI changes. Will close this once both of them are merged!

tracing: new jaeger independent helm chart

2fdb9c9

This PR moves tracing into a jaeger chart with no proxy injection templates. We still keep the dependency on partials, as we could use common templates like resources, etc from there. Signed-off-by: Tarun Pothulapati <[email protected]>

adleong reviewed Nov 17, 2020

View reviewed changes

Pothulapati force-pushed the tarun/sep-tracing branch from cc9ad0e to dba1ae7 Compare November 18, 2020 15:40

Pothulapati added 5 commits November 18, 2020 21:12

nits around jaeger cli command

d5f334b

Signed-off-by: Tarun Pothulapati <[email protected]>

nits around naming, EOF, etc

df8bf1b

Signed-off-by: Tarun Pothulapati <[email protected]>

Move jaeger chart into the top level jaeger directory

db39229

Also, added jaeger deps to the gitignore so that we are not storing dependencies in the repo Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/sep-tracing branch from dba1ae7 to 7917ed8 Compare November 18, 2020 15:43

alpeb reviewed Nov 18, 2020

View reviewed changes

remove unnecessary logic everywhere

9a8676d

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/sep-tracing branch from 7917ed8 to 9a8676d Compare November 18, 2020 15:58

fix easy nits

2614b0c

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong reviewed Nov 19, 2020

View reviewed changes

Pothulapati added 4 commits November 19, 2020 08:04

EOF fixes

227a66c

Signed-off-by: Tarun Pothulapati <[email protected]>

Add Helm build steps for jaeger independent chart

48ad60c

Signed-off-by: Tarun Pothulapati <[email protected]>

final removal of unecessary stuff

61ff966

Signed-off-by: Tarun Pothulapati <[email protected]>

final linting pass

480b06f

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati marked this pull request as ready for review November 19, 2020 11:45

Pothulapati requested a review from a team as a code owner November 19, 2020 11:45

add check for jaeger install

550e2a3

Signed-off-by: Tarun Pothulapati <[email protected]>

easy cleanup in charts and instal.go

b1dbb1c

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/sep-tracing branch from d715e4e to a004d1f Compare November 20, 2020 18:15

Pothulapati force-pushed the tarun/sep-tracing branch from a004d1f to 3631351 Compare November 20, 2020 18:16

alpeb reviewed Nov 20, 2020

View reviewed changes

adleong reviewed Nov 20, 2020

View reviewed changes

Revert "remove Dir field and make Fs as the core path field"

dff6d23

This reverts commit 3631351. Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati added 2 commits November 23, 2020 10:31

add linkerd-jaeger build to build-cli-bin to test

38d34f8

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/sep-tracing branch from b9d0317 to 38d34f8 Compare November 23, 2020 10:32

Pothulapati added 2 commits November 23, 2020 10:35

update jeager charts relative paths

66bb298

Signed-off-by: Tarun Pothulapati <[email protected]>

dont touch FS in this PR.

770c7ff

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/sep-tracing branch from 642545b to 770c7ff Compare November 23, 2020 11:35

alpeb approved these changes Nov 23, 2020

View reviewed changes

adleong mentioned this pull request Nov 23, 2020

tracing: new jaeger independent helm chart #5275

Merged

adleong closed this Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: new jaeger independent helm chart #5240

tracing: new jaeger independent helm chart #5240

Pothulapati commented Nov 17, 2020 •

edited

Loading

adleong left a comment

alpeb left a comment •

edited

Loading

alpeb Nov 18, 2020

Pothulapati Nov 18, 2020

adleong left a comment

adleong Nov 19, 2020

Pothulapati Nov 19, 2020 •

edited

Loading

adleong Nov 19, 2020

Pothulapati Nov 19, 2020

alpeb Nov 20, 2020

Pothulapati Nov 23, 2020

adleong left a comment

Pothulapati commented Nov 23, 2020 •

edited

Loading

alpeb left a comment

adleong commented Nov 23, 2020

Pothulapati commented Nov 24, 2020

tracing: new jaeger independent helm chart #5240

tracing: new jaeger independent helm chart #5240

Conversation

Pothulapati commented Nov 17, 2020 • edited Loading

adleong left a comment

Choose a reason for hiding this comment

alpeb left a comment • edited Loading

Choose a reason for hiding this comment

alpeb Nov 18, 2020

Choose a reason for hiding this comment

Pothulapati Nov 18, 2020

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

adleong Nov 19, 2020

Choose a reason for hiding this comment

Pothulapati Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

adleong Nov 19, 2020

Choose a reason for hiding this comment

Pothulapati Nov 19, 2020

Choose a reason for hiding this comment

alpeb Nov 20, 2020

Choose a reason for hiding this comment

Pothulapati Nov 23, 2020

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

Pothulapati commented Nov 23, 2020 • edited Loading

alpeb left a comment

Choose a reason for hiding this comment

adleong commented Nov 23, 2020

Pothulapati commented Nov 24, 2020

Pothulapati commented Nov 17, 2020 •

edited

Loading

alpeb left a comment •

edited

Loading

Pothulapati Nov 19, 2020 •

edited

Loading

Pothulapati commented Nov 23, 2020 •

edited

Loading