Tracing Add-on For Linkerd #3955

Pothulapati · 2020-01-20T14:03:34Z

Follows #3794

This PR adds the tracing components as a Linkerd Add-On. The add-on model follows the same consistent approach both through Helm and Linkerd CLI.

The following changes are part of this PR:

Pre-Requisite Changes:
- Moved some common yaml templates like _nodeselectors.yaml,etc required by multiple sub-charts to partials for re-use. Also updating the relevant helm tests.
Helm Changes:
- A Tracing sub-chart is added with partials dependency (for injection). This sub-chart is also added as an optional dependency for linkerd2 chart.
- Values.yaml has been updated to include the tracing add-on config.
- linkerd-values cm has been added which stores the add-on configurations, and will only be present when there is at-least one Add-On.
Install Path:
- Updates installOptions to include a config option, through which the configuration for the add-ons is given to the CLI.
- During Install, A final step has been added to read config (if given), and the default Values will be overwritten by this.
- If there are any add-ons present, those sub-charts are loaded and rendered in this step, by passing both .Values.global and the add-on values to the sub-chart.
Upgrade Path:
- The upgrade path in the final stage, now first checks if the linkerd-values cm exists, and updates the default addon values with the values present in linkerd-values, this is done to have persistent config upgrades.
- It then checks if there is a config option present, and overwrite the default values further with the options from the config file. This allows users to change configuration of add-ons during upgrades.

The logic to Overwrite the values struct, is done by this fork of mergo at https://github.com/pothulapati/mergo, because of this issue/decision from the official project.

To try tracing as an add-on:

Create a new config file as config.yaml

tracing:
  enabled: true

Now Run,
linkerd install --config config.yaml --control-plane-tracing | kubectl apply -f -
and see the magic happen ;)

@ihcsim @alpeb @zaharidichev @grampelberg

As add-ons re-use the partials helm chart, all the templates needed by multiple charts should be present in partials This commit also updates the helm tests Signed-off-by: Tarun Pothulapati <[email protected]>

Tracing sub-chart includes open-census and jaeger components as a sub-chart which can be enabled as needed Signed-off-by: Tarun Pothulapati <[email protected]>

This includes new interface for add-ons to implement, with example tracing implementation Signed-off-by: Tarun Pothulapati <[email protected]>

Changes include: - Adds an optional Linkerd Values configmap which stores add-on configuration when add-ons are present. - Updates Linkerd install path to check for add-ons and render their sub-charts. - Adds a install Option called config, which is used to pass confiugration for add-ons. - Uses a fork of mergo, to over-write default Values with the Values struct generated from config. Signed-off-by: Tarun Pothulapati <[email protected]>

Upgrade path now checks for the linkerd-values cm, and overwrites the default values with it, if present. It then checks the config option, for any further overwrites Signed-off-by: Tarun Pothulapati <[email protected]>

also adds relevant nil checks Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-01-20T14:03:53Z

Process required to add new Add-Ons:

A sub-chart has to be created first at /charts/linkerd2/add-ons/<add-on>, and added as a dependency to the linkerd2 sub-chart, with the condition field as <add-on>.enabled from the linkerd2/values.yaml. The linkerd2/values.yaml will be the file where the default config for add-ons will also be present.
The sub-chart can optionally take a dependency on partials to have proxy injection by including the necessary templates.
The Values struct in pkg/charts/values.go should have the relevant field for add-on, with the same structure as that of the one present in Values.yaml.
The relevant add-on config struct will added at pkg/charts/linkerd2/<add-on>.go, and should also implement the AddOn interface, as that logic is used in the install and upgrade paths to render the add-on templates.
Finally, The mergeAddOnValues and checkAddOns functions in install.go should be updated to check for the relevant newly added add-ons.

Pothulapati · 2020-01-20T14:25:38Z

This same add-on model can be used for making both grafana and prometheus as add-ons as per the test PR #3946
Separate PR's will be made for them, once this is accepted as there are some changes required to make grafana and prometheus as add-ons

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

Also refactors the linkerd-values cm to work the same with helm Signed-off-by: Tarun Pothulapati <[email protected]>

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

Pothulapati · 2020-01-23T10:17:00Z

Sorry, Did not realize there was a small error with linkerd-config causing identity pod to fail,
Fixed it.

Ready to review, now :)

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

alpeb

Hi @Pothulapati, I'm starting to review this. Looks promising!

I guess this supersedes #3946? I like that you're just doing the tracing part here, as an example, and then Grafana and Prometheus can be done later as separate PRs 😉

Quick question: why having the addons directly under charts/linkerd2 and not directly under charts?

Nit: An entry in .gitignore has to be added to avoid pushing anything under charts/**/charts (like partials-0.1.0.tgz)

I'll be giving this a full test next week 🎉

charts/linkerd2/add-ons/tracing/Chart.yaml

cli/cmd/install.go

pkg/charts/linkerd2/tracing.go

alpeb · 2020-01-24T21:17:56Z

cli/cmd/install.go

+func mergeAddonValues(values, addonValues *l5dcharts.Values) error {
+
+	if addonValues.Tracing != nil {
+		if err := mergo.Merge(addonValues.Tracing, values.Tracing); err != nil {
+			return err
+		}
+		values.Tracing = addonValues.Tracing
+	}
+	return nil
+}


Why not having the new --config flag serve as overrides for all the values, not just for add-ons? And if so, it might be clearer to implement that as a preliminary PR...

The main reason is that, some fields in values like config etc are generated way before in the install/upgrade path but as we override them with config last in the pipeline, this would require us to again redo those values.

Sure, we can keep this overwrite logic in the start but that would require a lot of code-changes. So, the plan is to do that next but keep it out of this PR. WDYT?

I'm not sure I follow. Doesn't this overriding logic imply that only the values present in the file provided through --config will get overridden, leaving all the others untouched? Why would it need to regenerate those other values? Maybe an example might help me understand 🙂

Ah, Sorry for not being clear.
My main concern is with this, as it is built by combining various values and is added in the linkerd-config cm, we will have to regenerate that agian after the override this. We could regenrate that again if it was based on Values struct but currently it is based on identityInstallOptions.

linkerd2/pkg/charts/linkerd2/values.go

Line 41 in 5fc83bc

Configs ConfigJSONs `json:"configs"`

Gotcha. Another reason to refactor this at some point in the future so we have to rely less on the structs in install.go and leverage those in values.go more directly 😉

I recommend excluding this change from this PR for now. It feels like a more complicated problem than we first thought. The addon will still work without the --config flag, right? It will be restricted to just defaults in the values.yaml.

alpeb · 2020-01-24T21:31:10Z

charts/linkerd2/templates/linkerd-values.yaml

@@ -0,0 +1,33 @@
+{{ if or (.Values.tracing.enabled) -}}


Seems like the or here is not needed?
Also if tracing is not enabled then $dupValues won't be defined. Will that cause a problem down at the end of this file?

This if condition is to not have linkerd-values cm to be deployed, when there are zero add-ons enabled. Hence, the or condition but once we make the linkerd-values common for all config, then we should be able to remove this if condition.

Also introduced a defaultGetFiles method for add-ons Signed-off-by: Tarun Pothulapati <[email protected]>

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

Pothulapati · 2020-01-27T12:38:41Z

@alpeb Updated gitignore for the same :) Thanks
reg sub-charts in linkerd2
There is no particular problem regarding that, but my opinion was that, as the sub-charts can't be re-used individually i.e without linkerd2 parent chart, It made sense to be inside. :) but I can move it if needed.

.gitignore

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

alpeb

Thanks @Pothulapati, this tested great to me.

It tested well with helm install, helm upgrade, linkerd install and linkerd upgrade.
One bug though is that the --config option is only available to the "control-plane" stage in both linkerd install and linkerd upgrade.

reg sub-charts in linkerd2
There is no particular problem regarding that, but my opinion was that, as the sub-charts can't be re-used individually i.e without linkerd2 parent chart, It made sense to be inside. :) but I can move it if needed.

Neither do we publish the "partials" nor "patch" charts, yet they reside under /charts.

Various things to remember when this functionality gets documented:

Note that the collector and Jaeger are installed in linkerd's namespace, which differs from the current tracing walkthrough doc
Need docs on how to turn off (tracing.enabled: false plus kubectl apply --prune I guess?)

charts/linkerd2/templates/linkerd-values.yaml

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

Pothulapati · 2020-01-29T19:17:11Z

Neither do we publish the "partials" nor "patch" charts, yet they reside under /charts.

Yep, :) Moved them to charts now.

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

cli/cmd/install.go

alpeb

Thanks @Pothulapati , I've added a few more comments below.
I tried testing but I couldn't see the tracing components in the output. I'm trying with

bin/go-run cli install --addon-config config.yaml

where config.yaml looks like

# Configuration for Add-ons
tracing:
  enabled: true
  collector:
    name: linkerd-collector
    image: omnition/opencensus-collector:0.1.10
    resources:
      cpu:
        limit: 1
        request: 200m
      memory:
        limit: 1Gi
        request: 400Mi
  jaeger:
    name: linkerd-jaeger
    image: jaegertracing/all-in-one:1.8
    resources:

alpeb · 2020-02-24T21:22:52Z

pkg/charts/charts.go

+// readVirtualFiles read the content of a file from the VFS. If the file is
+// directory, it also loads the children files content, recursively.


typos: reads and If the file is a directory

alpeb · 2020-02-24T21:36:23Z

pkg/charts/charts.go

+		return nil
+	}
+
+	if err := walkVFS(); err != nil {


It seems walkVFS no longer needs to be declared as a lambda so you can flatten out the contents of this function

alpeb · 2020-02-24T21:44:50Z

pkg/charts/charts.go

+// LoadDependencies loads all the dependent subcharts of the specified chart.
+// It relies on LoadChart to load the files and metadata of the chart from the
+// VFS.
+func LoadDependencies(chartName string) ([]*chart.Chart, error) {
+	chart, err := LoadChart(chartName)
+	if err != nil {
+		return nil, err
+	}
+
+	return chart.Dependencies, nil
+}


I think this can be further simplified by removing this function and just calling LoadChart directly and using .Dependencies on the returned value.

In the patch I sent to @Pothulapati, I didn't want to call LoadChart() in install.go directly, to avoid confusion with the existing l5dchart.Chart struct.

pkg/charts/charts_test.go

alpeb · 2020-02-24T22:02:04Z

cli/cmd/install.go

+		addOnValues, enabled := checkAddon(values, chartName)
+		if enabled {


Can be simplified with

if addOnValues, enabled := checkAddon(values, chartName); enabled { ... }

alpeb · 2020-02-24T22:19:12Z

cli/cmd/install.go

+	if !reflect.Indirect(r).FieldByName(strings.Title(name)).IsNil() {
+		if reflect.Indirect(r).FieldByName(strings.Title(name)).MapIndex(reflect.ValueOf("enabled")).Interface().(bool) {
+			values, err := yaml.Marshal(reflect.Indirect(r).FieldByName(strings.Title(name)).Interface())
+			if err != nil {
+				return nil, false
+			}
+			return values, true
+		}


if the addon config is missing then the value of reflect.Indirect(r).FieldByName(strings.Title(name)) would be an empty map (zero value) to which you can safely apply MapIndex, so no need of the nested if nor the IsNil() check.

You can also extract the common part of the remaining condition:

config := reflect.Indirect(r).FieldByName(strings.Title(name)); if config.MapIndex(reflect.ValueOf("enabled")).Interface().(bool) { values, err := yaml.Marshal(config.Interface()) if err != nil { return nil, false } return values, true }

alpeb · 2020-02-24T22:51:24Z

@Pothulapati Also, I wasn't able to test, but it appears this new approach isn't complatible with multi-stage? One thing that occurs to me for that would be to enforce putting config-level templates under a separate templates/config dir...

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

ihcsim · 2020-02-25T17:02:13Z

@alpeb We did some more testing yesterday, and decided that the only way to make sure the addons code works for the different install/upgrade commands and subcommands is to follow the pattern that is used for the partials, where we provide the full path inside the VFS. Let us know what you think. Thanks!

ihcsim

This works great for me 👍! I tested with install, upgrade, install config, install control-plane and inject (with the --addon-config flag), using the CLI from the target/ folder to make sure it's truly reading the templates from the VFS.

alpeb · 2020-02-25T18:13:51Z

@alpeb We did some more testing yesterday, and decided that the only way to make sure the addons code works for the different install/upgrade commands and subcommands is to follow the pattern that is used for the partials, where we provide the full path inside the VFS. Let us know what you think.

I liked how the previous changes allowed for so little customization to be required when adding a new add-on. So I was hoping the VFS loading logic, having being informed of the current stage, would be able to detect the stage some template belonged to given its subdir location or some other convention. Nice to have, but I would understand if that'd be too much at this point 🙂

alpeb · 2020-02-25T18:36:58Z

Oh I see all the VFS magic from charts.go is gone now. Alright, that looks good to me; we'll see later if being able to add addons with little-to-none changes is really required 👍

alpeb

Thanks @Pothulapati , it seems we're almost there 👍
I've added a couple of minor comments 🙏

alpeb · 2020-02-25T18:50:04Z

cli/cmd/install.go

+	addonValues := map[string][]byte{}
+
+	if values.Tracing != nil {
+		if values.Tracing["enabled"].(bool) {


Cool that you found a way around reflection!

OTOH, If the enable property is not a boolean, this will cause a panic. Can you do instead something like

if enabled, ok := values.Tracing["enabled].(bool); !ok { return nil, fmt.Errorf("invalid value for 'Tracing.enabled' (should be boolean): ", values.Tracing["enabled"]); } else if enabled { ... }

cli/cmd/install.go

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

ihcsim · 2020-02-26T06:32:26Z

I liked how the previous changes allowed for so little customization to be required when adding a new add-on

@alpeb Yeah, I back out of it because previously, Tarun and I thought of introducing the usage of the Helm lib to load charts in this PR. Then in a separate PR, see if we can refactor the existing charts code to use the same mechanism. But that would make it impossible to run the CLI unit tests (afaict) without requiring everyone to run bin/helm-build. (I didn't want to add a command.Exec() to the Go unit test code.) The reason is that when using the Helm lib, the render() function of install will always try to look for the addons subcharts in charts/linkerd2/charts, not charts/add-ons. There might be ways to try to bend it, but I feel it will further complicate the code.

We feel what we did here is easier to reason about, because it offers the consistency of using the same pattern as that used by the existing code to render the Linkerd, partials and CNI charts. Thanks for your help on this one!

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

Pothulapati · 2020-02-26T17:11:19Z

@alpeb Thanks for the review! Updated the PR now.

As @ihcsim mentioned, These changes feel more concrete and stable as we are re-using the already present stuff. Also, Not using reflect, and writing concrete type checks make more sense!

Pothulapati · 2020-02-26T17:14:15Z

One more thing, I think we should address is updating helm-build to also do dep up and get relevant sub-charts.

alpeb

Thanks @ihcsim for the detailed explanation, and @Pothulapati for implementing this. I agree about the pending helm-build update, which can be tackled in a separate PR along with the extra CI step needed to publish the chart.

This looks good and tests fine for me! 🥇 🏆

@christyjacob4

## edge-20.2.3 This release introduces the first optional add-on `tracing`, added through the new add-on model! The existing optional `tracing` components Jaeger and OpenCensus can now be installed as add-on components. There will be more information to come about the new add-on model, but please refer to the details of [#3955](#3955) for how to get started. * CLI * Added the `linkerd diagnostics` command to get metrics only from the control plane, excluding metrics from the data plane proxies (thanks @srv-twry!) * Added the `linkerd install --prometheus-image` option for installing a custom Prometheus image (thanks @christyjacob4!) * Fixed an issue with `linkerd upgrade` where changes to the `Namespace` object were ignored (thanks @supra08!) * Controller * Added the `tracing` add-on which installs Jaeger and OpenCensus as add-on components (thanks @Pothulapati!!) * Proxy * Increased the inbound router's default capacity from 100 to 10k to accommodate environments that have a high cardinality of virtual hosts served by a single pod * Web UI * Fixed styling in the CallToAction banner (thanks @aliariff!)

Pothulapati added 6 commits January 19, 2020 23:37

Moves Common templates needed to partials

442c787

As add-ons re-use the partials helm chart, all the templates needed by multiple charts should be present in partials This commit also updates the helm tests Signed-off-by: Tarun Pothulapati <[email protected]>

add tracing add-on helm chart

6eba3da

Tracing sub-chart includes open-census and jaeger components as a sub-chart which can be enabled as needed Signed-off-by: Tarun Pothulapati <[email protected]>

Updated Install path to also install add-ons

a428e96

This includes new interface for add-ons to implement, with example tracing implementation Signed-off-by: Tarun Pothulapati <[email protected]>

Updates the upgrade path about add-ons.

9827600

Upgrade path now checks for the linkerd-values cm, and overwrites the default values with it, if present. It then checks the config option, for any further overwrites Signed-off-by: Tarun Pothulapati <[email protected]>

Refactor linkerd-values and re-update tests

d6eb6db

also adds relevant nil checks Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati requested review from ihcsim, zaharidichev and alpeb January 20, 2020 14:04

Pothulapati added 3 commits January 20, 2020 20:09

Refactor code to fix linting issues

c303d02

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

Fixes an error with linkerd-config global values

1bb5fa4

Also refactors the linkerd-values cm to work the same with helm Signed-off-by: Tarun Pothulapati <[email protected]>

Fix a nil pointer issue for tests

b8f7182

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

Merge branch 'master' into addon-tracing

b72b7f8

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

alpeb reviewed Jan 24, 2020

View reviewed changes

Pothulapati added 2 commits January 26, 2020 14:05

Updated Tracing add-on chart meta-data

df90cb3

Also introduced a defaultGetFiles method for add-ons Signed-off-by: Tarun Pothulapati <[email protected]>

Add add-on/charts to gitignore

6f3fa94

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

Pothulapati requested a review from alpeb January 28, 2020 09:47

alpeb reviewed Jan 28, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

refactor gitignore for chart deps

89fc1df

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

alpeb reviewed Jan 28, 2020

View reviewed changes

charts/linkerd2/templates/linkerd-values.yaml Outdated Show resolved Hide resolved

Pothulapati added 3 commits January 30, 2020 00:16

Moves sub-charts to /charts directly

94f2340

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

Refactor linkerd values cm

1347e12

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

Add comment in linkerd-values

b0e013d

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

Pothulapati added 2 commits February 8, 2020 15:04

Merge branch 'master' into addon-tracing

27347ab

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

remove extra controlplanetracing flag

048e696

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

Pothulapati added 2 commits February 24, 2020 15:30

Fix golden files

2f50ee7

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

remove addon install unit test

8461251

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

alpeb reviewed Feb 24, 2020

View reviewed changes

cli/cmd/install.go Outdated Show resolved Hide resolved

alpeb reviewed Feb 24, 2020

View reviewed changes

cli/cmd/install.go Outdated Show resolved Hide resolved

alpeb reviewed Feb 24, 2020

View reviewed changes

Pothulapati added 3 commits February 25, 2020 15:02

Refactor sub-chart load logic

250e0e2

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

Add install tracing unit test

bc32082

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

golden file update for tracing install

12f7b1a

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

Pothulapati force-pushed the addon-tracing branch from 349a9ea to 12f7b1a Compare February 25, 2020 11:41

Pothulapati added 2 commits February 25, 2020 20:21

Merge branch 'master' into addon-tracing

a669666

Update golden files to reflect another pr changes

cb8b486

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

ihcsim approved these changes Feb 25, 2020

View reviewed changes

alpeb reviewed Feb 25, 2020

View reviewed changes

Move addon-config flag to recordFlagSet

829ba04

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

Pothulapati added 2 commits February 26, 2020 18:30

add relevant tracing enabled checks

8ef593c

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

linting fix

26b28ad

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

Pothulapati force-pushed the addon-tracing branch from f4c6dbb to 26b28ad Compare February 26, 2020 15:03

alpeb approved these changes Feb 26, 2020

View reviewed changes

ihcsim merged commit 948dc22 into linkerd:master Feb 26, 2020

Pothulapati mentioned this pull request Feb 27, 2020

Include SMI metrics as part of Linkerd install #4109

Merged

ihcsim mentioned this pull request Feb 27, 2020

Add changes for edge-20.2.3 #4113

Merged

Pothulapati mentioned this pull request Apr 15, 2020

Bring your own prometheus linkerd/rfc#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing Add-on For Linkerd #3955

Tracing Add-on For Linkerd #3955

Pothulapati commented Jan 20, 2020 •

edited

Loading

Pothulapati commented Jan 20, 2020

Pothulapati commented Jan 20, 2020

Pothulapati commented Jan 23, 2020

alpeb left a comment

alpeb Jan 24, 2020

Pothulapati Jan 26, 2020

alpeb Jan 28, 2020

Pothulapati Jan 28, 2020

alpeb Jan 28, 2020

ihcsim Feb 11, 2020

alpeb Jan 24, 2020

Pothulapati Jan 25, 2020

Pothulapati commented Jan 27, 2020

alpeb left a comment

Pothulapati commented Jan 29, 2020

alpeb left a comment

alpeb Feb 24, 2020

alpeb Feb 24, 2020

alpeb Feb 24, 2020

ihcsim Feb 24, 2020

alpeb Feb 24, 2020

alpeb Feb 24, 2020

alpeb commented Feb 24, 2020

ihcsim commented Feb 25, 2020

ihcsim left a comment

alpeb commented Feb 25, 2020

alpeb commented Feb 25, 2020

alpeb left a comment

alpeb Feb 25, 2020

ihcsim commented Feb 26, 2020

Pothulapati commented Feb 26, 2020

Pothulapati commented Feb 26, 2020

alpeb left a comment

		// readVirtualFiles read the content of a file from the VFS. If the file is
		// directory, it also loads the children files content, recursively.

		addOnValues, enabled := checkAddon(values, chartName)
		if enabled {

Tracing Add-on For Linkerd #3955

Tracing Add-on For Linkerd #3955

Conversation

Pothulapati commented Jan 20, 2020 • edited Loading

Pothulapati commented Jan 20, 2020

Pothulapati commented Jan 20, 2020

Pothulapati commented Jan 23, 2020

alpeb left a 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

Pothulapati commented Jan 27, 2020

alpeb left a comment

Choose a reason for hiding this comment

Pothulapati commented Jan 29, 2020

alpeb left a 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

alpeb commented Feb 24, 2020

ihcsim commented Feb 25, 2020

ihcsim left a comment

Choose a reason for hiding this comment

alpeb commented Feb 25, 2020

alpeb commented Feb 25, 2020

alpeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihcsim commented Feb 26, 2020

Pothulapati commented Feb 26, 2020

Pothulapati commented Feb 26, 2020

alpeb left a comment

Choose a reason for hiding this comment

Pothulapati commented Jan 20, 2020 •

edited

Loading