osm/smi: update dir structure and add test #118

SanyaKochhar · 2021-09-17T22:54:58Z

Updates to collect osm custom resource meshconfig per latest osm release
Fixes collection of smi crds (rbac in cluster-role)
Adds naming structure to filename: meshName/resourceName_resourceType and smi/resourceName_resourceType>
Tested with kustomize kubectl apply -k deployment

codecov-commenter · 2021-09-17T23:02:11Z

Codecov Report

Merging #118 (3db237a) into master (118d62a) will increase coverage by 7.38%.
The diff coverage is 0.00%.

❗ Current head 3db237a differs from pull request most recent head 84eb0de. Consider uploading reports for the commit 84eb0de to get more accurate results

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   31.46%   38.85%   +7.38%     
==========================================
  Files          12       12              
  Lines         518      525       +7     
==========================================
+ Hits          163      204      +41     
+ Misses        340      304      -36     
- Partials       15       17       +2

Impacted Files	Coverage Δ
pkg/collector/osm_collector.go	`6.54% <0.00%> (+6.54%)`	⬆️
pkg/collector/smi_collector.go	`0.00% <0.00%> (ø)`
pkg/collector/pods_containerlogs_collector.go	`81.81% <0.00%> (+4.04%)`	⬆️
pkg/collector/networkoutbound_collector.go	`95.83% <0.00%> (+4.16%)`	⬆️
pkg/collector/helm_collector.go	`80.43% <0.00%> (+4.34%)`	⬆️
pkg/collector/dns_collector.go	`88.23% <0.00%> (+11.76%)`	⬆️
pkg/collector/nodelogs_collector.go	`86.66% <0.00%> (+13.33%)`	⬆️
pkg/collector/kubeobjects_collector.go	`80.00% <0.00%> (+80.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 118d62a...84eb0de. Read the comment docs.

deployment/aks-periscope.yaml

Tatsinnit

💡 Thank you for this PR and pinging us for review.

I have added few key comments, I assume you have tested all this for SMI\OSM scenario and all works as expected.

If the yaml changes are specific to SMI/OSM needs, you might only want to keep them in your consuming tools.
Please add some unit-test for this collector if possible, currently it decreases the over-all code-coverage. ( )
Regarding the over all, after you are GA, we should align a work for converting it using client-go packages into this collector, that will add better test-ability as well. https://github.com/kubernetes/client-go
- There are some recent refactors we have done for those reason, one of the sample implementation is in Pod Container work or systemperf collector.

Thank you

deployment/cluster-role.yaml

Tatsinnit · 2021-09-22T03:55:37Z

pkg/collector/osm_collector.go

@@ -197,8 +198,8 @@ func (collector *OsmCollector) collectDataFromEnvoys(namespace string) error {
 			// Remove certificate secrets from Envoy config i.e., "inline_bytes" field from response
 			re := regexp.MustCompile("(?m)[\r\n]+^.*inline_bytes.*$")
 			secretRemovedResponse := re.ReplaceAllString(string(responseBody), "---redacted---")
-
-			collector.data[query+"_"+podName] = secretRemovedResponse
+			filePath := meshName + "/envoy/"+ podName + query


💡 question: I don't know much about this scenario hence I am not not sure what these changes are, but I am guessing this mesh which is passed throughout is for a specific reasons?

There can be multiple meshes in a cluster and the collector collects all resources for each of them, so the number of files was getting overwhelming and had to discern between. Passing in mesh allows the file system to be organized so that resources collected for each mesh are grouped together, at least. Initially I was trying to just use a smarter naming system without creating subdirectories but the names were getting very long and unwieldy, ex: <meshName>_namespace_<namespaceName>_pod_<podName>_logs. If your team is open to it, I would prefer to include further subdirectories for each mesh, namespace, etc. as we had before the code was refactored so we can avoid such complex naming

cc: @arnaud-tincelin

pkg/collector/osm_collector.go

Tatsinnit · 2021-09-22T03:58:22Z

pkg/collector/osm_collector.go

@@ -220,8 +221,8 @@ func (collector *OsmCollector) collectPodLogs(namespace string) error {
 			output = fmt.Sprintf("Failed to collect logs for pod %s: %+v", podName, err)
 			log.Print(output)
 		}
-
-		collector.data[podName+"_logs"] = output
+		filePath := meshName + "/" + podName + "_podLogs"


💡 what is the outcome of this change? does this automatically generates the folder structure?

yes, more context here: #118 (comment)

pkg/collector/osm_collector.go

Tatsinnit

💡 🙏 Thanks for this, there are few comments, especially the yaml changes are something which need re-look. I see mainly PR is just the name and folder structure change and if you have tested this and only thing left is the yaml changes which is not needed in the core. Thank you for adding very initial test class for this as well.

Can you please open a tracking item for the conversion of these collector to client-go we have already implemented logic clients and podDecribe For example a sample reside here: https://github.com/Azure/aks-periscope/blob/master/pkg/collector/kubeobjects_collector.go#L54

for future purpose regarding the client-go > api based implementation, I can help and implement this the conversion with you but I will need someone who understand the SMI/OSM scenario to test them alongside. 🙏☕️

Thank you so much,

deployment/config-map.yaml

deployment/cluster-role.yaml

pkg/collector/osm_collector_test.go

SanyaKochhar · 2021-10-04T18:19:42Z

💡 🙏 Thanks for this, there are few comments, especially the yaml changes are something which need re-look. I see mainly PR is just the name and folder structure change and if you have tested this and only thing left is the yaml changes which is not needed in the core. Thank you for adding very initial test class for this as well.

Can you please open a tracking item for the conversion of these collector to client-go we have already implemented logic clients and podDecribe For example a sample reside here: https://github.com/Azure/aks-periscope/blob/master/pkg/collector/kubeobjects_collector.go#L54

for future purpose regarding the client-go > api based implementation, I can help and implement this the conversion with you but I will need someone who understand the SMI/OSM scenario to test them alongside. 🙏☕️

Thank you so much,

That is correct, I've now updated the title of the PR to reflect the same and opened an issue to track the conversion to client go here #124. Thanks for your help with this.

Tatsinnit

👍

johnsonshi reviewed Sep 20, 2021

View reviewed changes

deployment/aks-periscope.yaml Outdated Show resolved Hide resolved

SanyaKochhar force-pushed the osm-collector-updates branch from ce68617 to f0c6d2d Compare September 21, 2021 16:28

SanyaKochhar marked this pull request as ready for review September 21, 2021 16:29

SanyaKochhar requested a review from johnsonshi September 21, 2021 18:41

Tatsinnit requested review from arnaud-tincelin and Tatsinnit September 21, 2021 19:41

Tatsinnit assigned SanyaKochhar Sep 21, 2021

Tatsinnit requested changes Sep 22, 2021

View reviewed changes

SanyaKochhar force-pushed the osm-collector-updates branch 2 times, most recently from 2a404a7 to 3998c55 Compare September 27, 2021 22:50

Tatsinnit requested changes Oct 2, 2021

View reviewed changes

deployment/config-map.yaml Outdated Show resolved Hide resolved

deployment/cluster-role.yaml Show resolved Hide resolved

Tatsinnit reviewed Oct 4, 2021

View reviewed changes

pkg/collector/osm_collector_test.go Show resolved Hide resolved

SanyaKochhar changed the title ~~osm/smi: update collectors to work for crd collection~~ osm/smi: update file structure and add test Oct 4, 2021

osm/smi: update dir structure and add test

84eb0de

SanyaKochhar force-pushed the osm-collector-updates branch from 3db237a to 84eb0de Compare October 4, 2021 18:14

SanyaKochhar changed the title ~~osm/smi: update file structure and add test~~ osm/smi: update dir structure and add test Oct 4, 2021

Tatsinnit approved these changes Oct 4, 2021

View reviewed changes

Tatsinnit merged commit f4237c0 into Azure:master Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osm/smi: update dir structure and add test #118

osm/smi: update dir structure and add test #118

SanyaKochhar commented Sep 17, 2021 •

edited

Loading

codecov-commenter commented Sep 17, 2021 •

edited

Loading

Tatsinnit left a comment •

edited

Loading

Tatsinnit Sep 22, 2021

SanyaKochhar Sep 22, 2021

Tatsinnit Sep 22, 2021

SanyaKochhar Sep 22, 2021

Tatsinnit left a comment •

edited

Loading

SanyaKochhar commented Oct 4, 2021

Tatsinnit left a comment

osm/smi: update dir structure and add test #118

osm/smi: update dir structure and add test #118

Conversation

SanyaKochhar commented Sep 17, 2021 • edited Loading

codecov-commenter commented Sep 17, 2021 • edited Loading

Codecov Report

Tatsinnit left a comment • edited Loading

Choose a reason for hiding this comment

Tatsinnit Sep 22, 2021

Choose a reason for hiding this comment

SanyaKochhar Sep 22, 2021

Choose a reason for hiding this comment

Tatsinnit Sep 22, 2021

Choose a reason for hiding this comment

SanyaKochhar Sep 22, 2021

Choose a reason for hiding this comment

Tatsinnit left a comment • edited Loading

Choose a reason for hiding this comment

SanyaKochhar commented Oct 4, 2021

Tatsinnit left a comment

Choose a reason for hiding this comment

SanyaKochhar commented Sep 17, 2021 •

edited

Loading

codecov-commenter commented Sep 17, 2021 •

edited

Loading

Tatsinnit left a comment •

edited

Loading

Tatsinnit left a comment •

edited

Loading