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

Include SMI metrics as part of Linkerd install #4109

Merged
merged 9 commits into from
Mar 2, 2020

Conversation

adleong
Copy link
Member

@adleong adleong commented Feb 27, 2020

Adds the SMI metrics API to the Linkerd install flow. This installs the SMI metrics controller deployment, the SMI metrics ApiService object, and supporting RBAC, and config resources.

This is the first step toward having Linkerd consume the SMI metrics API in the CLI and web dashboard. For a sneak peek at that work, try out the linkerd alpha stat command in the smi-metrics branch.

Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
@Pothulapati
Copy link
Contributor

I don't have strong opinions here, but Should this be an add-on i.e optional? like we did for tracing at #3955 :)

@adleong
Copy link
Member Author

adleong commented Feb 27, 2020

@Pothulapati good question! Since the plan is to eventually switch the CLI and web dashboard over to this API, I think it makes sense to have it as a core component rather than an addon.

@Pothulapati
Copy link
Contributor

@adleong Ah! Makes Sense :)

Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

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

Code change LGTM 👍.

What is the plan on managing the divergence from upstream?

cli/cmd/install_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Looks good! Have a quick question about the install process and if we need to account for multi-stage installs.

cli/cmd/install.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
@adleong
Copy link
Member Author

adleong commented Feb 28, 2020

@ihcsim This effectively pins our dependency on deislabs/smi-metrics:v0.2.0 and on the Linkerd smi-metrics-config copied from the smi metrics repo. When we want to pick up a new version, we will need to bump the image version and copy the new config over.

cli/cmd/install.go Outdated Show resolved Hide resolved
@ihcsim
Copy link
Contributor

ihcsim commented Mar 2, 2020

@adleong While it's not great that we have to manually merge new YAML and maintain the custom changes in these templates down the road, I can't think of a better way for this atm. I was hoping to write a script that can auto-generate all these smi-metrics templates whenever we need to upgrade to a new version, by using helm template to render the smi-metrics original chart, then use kustomize to inject the custom labels, annotations, name prefix and some other fields into the rendered output. The proxy and proxy-init spec remains the main challenge, because we can't just run helm template on the partials chart which consists many install-time configuration. Otherwise, the rendered partial chart can be used by kustomize to inject the proxy spec into the original smi-metrics templates.

Even adding smi-metrics as a subchart to the Linkerd chart in the requirements.yaml isn't great, because it means the unit test won't pass without first running helm dep update.

Signed-off-by: Alex Leong <alex@buoyant.io>
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Looks good!

@adleong adleong merged commit 71d6a00 into master Mar 2, 2020
@adleong adleong deleted the alex/install-smi-metrics branch March 2, 2020 18:11
adleong added a commit that referenced this pull request Mar 2, 2020
Adds the SMI metrics API to the Linkerd install flow.  This installs the SMI metrics controller deployment, the SMI metrics ApiService object, and supporting RBAC, and config resources.

This is the first step toward having Linkerd consume the SMI metrics API in the CLI and web dashboard.

Signed-off-by: Alex Leong <alex@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants