Local Machine exporter #57

sophsoph321 · 2021-05-19T17:46:00Z

This is another arc customization that we plan to include, which is to export logs to the local machine in the scenario where there is no network connectivity to Azure.

jonathan-innis

Small comments, overall looks good 😄

jonathan-innis · 2021-05-20T18:36:37Z

cmd/aks-periscope/aks-periscope.go

+	exporters := []interfaces.Exporter{}
+	clusterType := os.Getenv("CLUSTER_TYPE")
+
+	exporters = append(exporters, &exporter.AzureBlobExporter{})


If this is always appended just init with it already there, i.e. []interfaces.Exporter{&exporter.AzureBlobExporter{}}

cmd/aks-periscope/aks-periscope.go

jonathan-innis · 2021-05-20T18:40:43Z

pkg/exporter/localmachine_exporter.go

+		if err != nil {
+			return err
+		}
+		defer fileToZip.Close()


Just a heads up, this defer will not be called until the end of the func so you will be opening a lot of file descriptors in the range function until you close them at the end. This could potentially be an issue with a large number of files

This may be consistent with how others are doing it, in that case not an issue 😄

Yes, I might leave this part as is for now, bc other classes seem to be using defer like this. If it does become an issue, I will fix it.

# This is the 1st commit message: Revert "introduce arcmode" This reverts commit 5f4fed4. # This is the commit message #2: remove secrets # This is the commit message Azure#3: add print statement # This is the commit message Azure#4: update print statement # This is the commit message Azure#5: committed # This is the commit message Azure#6: committed # This is the commit message Azure#7: committed # This is the commit message Azure#8: committed # This is the commit message Azure#9: remove print statements # This is the commit message Azure#10: add helm collector # This is the commit message Azure#11: change helm command # This is the commit message Azure#12: add helm 3 installation # This is the commit message Azure#13: add curl command installation # This is the commit message Azure#14: change helm command # This is the commit message Azure#15: remove helm history # This is the commit message Azure#16: debug helm history # This is the commit message Azure#17: add repo command # This is the commit message Azure#18: change stable repo name # This is the commit message Azure#19: add write to file # This is the commit message Azure#20: add kured # This is the commit message Azure#21: change # This is the commit message Azure#22: changes # This is the commit message Azure#23: add default namespace # This is the commit message Azure#24: change # This is the commit message Azure#25: add integration test # This is the commit message Azure#26: changes # This is the commit message Azure#27: add helm test # This is the commit message Azure#28: change print statement to error # This is the commit message Azure#29: change # This is the commit message Azure#30: more changes # This is the commit message Azure#31: add go installation # This is the commit message Azure#32: fix unit test # This is the commit message Azure#33: iptables to Helm # This is the commit message Azure#34: add custom resource collector # This is the commit message Azure#35: add new exporter, diagnoser, collector # This is the commit message Azure#36: comment unused variables # This is the commit message Azure#37: debug exporter # This is the commit message Azure#38: filenames # This is the commit message Azure#39: test zip function # This is the commit message Azure#40: list files # This is the commit message Azure#41: fmt to log # This is the commit message Azure#42: delete lines # This is the commit message Azure#43: changed # This is the commit message Azure#44: get current directory # This is the commit message Azure#45: remove some print statements # This is the commit message Azure#46: test zip # This is the commit message Azure#47: changes # This is the commit message Azure#48: add windir check # This is the commit message Azure#49: minor fix # This is the commit message Azure#50: get hostname # This is the commit message Azure#51: add expose in dockerfile # This is the commit message Azure#52: add exec collector # This is the commit message Azure#53: mitigate exit code 126 # This is the commit message Azure#54: change curl url from example.com to dp endpoint # This is the commit message Azure#55: changes # This is the commit message Azure#56: uncomment exec # This is the commit message Azure#57: add new diagnoser # This is the commit message Azure#58: debugging # This is the commit message Azure#59: debug # This is the commit message Azure#60: debugging # This is the commit message Azure#61: remove print statements # This is the commit message Azure#62: remove print # This is the commit message Azure#63: add back crd print statement # This is the commit message Azure#64: change # This is the commit message Azure#65: change # This is the commit message Azure#66: update dataPoint name # This is the commit message Azure#67: modify forloop # This is the commit message Azure#68: add filename to datapoint # This is the commit message Azure#69: add back log prints # This is the commit message Azure#70: test # This is the commit message Azure#71: add fields to diagnostic signal # This is the commit message Azure#72: add config content to diagnoser # This is the commit message Azure#73: change format from yaml to json # This is the commit message Azure#74: add parameters for kubeobject config map # This is the commit message Azure#75: Revert "introduce arcmode" This reverts commit 5f4fed4. # This is the commit message Azure#76: fix helm collector style # This is the commit message Azure#77: revert changes that test arc customizations # This is the commit message Azure#78: fix merge conflicts # This is the commit message Azure#79: fix merge conflicts # This is the commit message Azure#80: Revert "Add v0.3 acr image for Private cluster fix. (Azure#22)" This reverts commit 49dd302. # This is the commit message Azure#81: fix merge conflicts # This is the commit message Azure#82: fix merge conflicts # This is the commit message Azure#83: add print statement # This is the commit message Azure#84: update print statement # This is the commit message Azure#85: committed # This is the commit message Azure#86: committed # This is the commit message Azure#87: committed # This is the commit message Azure#88: committed # This is the commit message Azure#89: remove print statements # This is the commit message Azure#90: fix merge conflicts # This is the commit message Azure#91: fix merge conflicts # This is the commit message Azure#92: fix merge conflicts # This is the commit message Azure#93: add repo command # This is the commit message Azure#94: change stable repo name # This is the commit message Azure#95: add write to file # This is the commit message Azure#96: add kured # This is the commit message Azure#97: change # This is the commit message Azure#98: changes # This is the commit message Azure#99: add default namespace # This is the commit message Azure#100: change # This is the commit message Azure#101: add integration test # This is the commit message Azure#102: changes # This is the commit message Azure#103: add helm test # This is the commit message Azure#104: change print statement to error # This is the commit message Azure#105: change # This is the commit message Azure#106: more changes # This is the commit message Azure#107: add go installation # This is the commit message Azure#108: fix unit test # This is the commit message Azure#109: add custom resource collector # This is the commit message Azure#110: fix merge conflicts # This is the commit message Azure#111: comment unused variables # This is the commit message Azure#112: debug exporter # This is the commit message Azure#113: filenames # This is the commit message Azure#114: test zip function # This is the commit message Azure#115: list files # This is the commit message Azure#116: fmt to log # This is the commit message Azure#117: delete lines # This is the commit message Azure#118: changed # This is the commit message Azure#119: get current directory # This is the commit message Azure#120: remove some print statements # This is the commit message Azure#121: test zip # This is the commit message Azure#122: changes # This is the commit message Azure#123: add windir check # This is the commit message Azure#124: minor fix # This is the commit message Azure#125: get hostname # This is the commit message Azure#126: add expose in dockerfile # This is the commit message Azure#127: add exec collector

arnaud-tincelin · 2021-05-24T13:09:48Z

This is another arc customization that we plan to include, which is to export logs to the local machine in the scenario where there is no internet connectivity.

Could you elaborate more on this subject please? Blob Storage can still be accessed by private endpoints even if you block outbound to internet

sophsoph321 · 2021-05-24T16:14:09Z

This is another arc customization that we plan to include, which is to export logs to the local machine in the scenario where there is no internet connectivity.

Could you elaborate more on this subject please? Blob Storage can still be accessed by private endpoints even if you block outbound to internet

@arnaud-tincelin. Good catch, I will update the description. This is an arc customization, a new exporter that we want to have that can download logs to the local machine. Of course, on the periscope end, the logs would be stored into the pods/containers. We're planning on having an arc-specific CLI that's going to use "kubectl cp" to copy the logs from the pods to the local machine.

About the Azure blob storage, I agree that Azure blob storage will still work for arc connected clusters. But there are arc scenarios where we're also dealing with resources that do not have network connectivity with Azure. When network connectivity to Azure is not possible, we wouldn't be able to validate or access the storage accounts, so the logs would need to be exported to the local machine instead, which is why we're adding this new exporter. Does this make sense?

arnaud-tincelin · 2021-05-25T12:10:43Z

This is another arc customization that we plan to include, which is to export logs to the local machine in the scenario where there is no internet connectivity.

Could you elaborate more on this subject please? Blob Storage can still be accessed by private endpoints even if you block outbound to internet

This is another arc customization that we plan to include, which is to export logs to the local machine in the scenario where there is no internet connectivity.

Could you elaborate more on this subject please? Blob Storage can still be accessed by private endpoints even if you block outbound to internet

@arnaud-tincelin. Good catch, I will update the description. This is an arc customization, a new exporter that we want to have that can download logs to the local machine. Of course, on the periscope end, the logs would be stored into the pods/containers. We're planning on having an arc-specific CLI that's going to use "kubectl cp" to copy the logs from the pods to the local machine.

About the Azure blob storage, I agree that Azure blob storage will still work for arc connected clusters. But there are arc scenarios where we're also dealing with resources that do not have network connectivity with Azure. When network connectivity to Azure is not possible, we wouldn't be able to validate or access the storage accounts, so the logs would need to be exported to the local machine instead, which is why we're adding this new exporter. Does this make sense?

Thank you for your reply @sophsoph321 :) When you speak about local machine, is it the node? If so, do not forget they may be deleted at any time, and the logs gone. We might want to store them on a PV instead. Also, if there is no connectivity, how do you plan to export the logs?

deployment/aks-periscope.yaml

safeermohammed · 2021-06-01T16:50:15Z

Hi @arnaud-tincelin, local machine here is the machine on which az CLI runs. Basically the zip file should be copied to the CLI machine. Both storage and local zip are valid scenarios for Arc and default is always to storage account. Local machine is special case. Offline I will send you specs for Arc.

@sophsoph321, The higher order of priority for zip file (both Azureblob export or local export) is zip files should not stored by default on Nodes. Nodes in Arc are owned by customer and we would not have access. Can you point me to the code where we are making sure in Arc mode the zips are not stored on Nodes?

In reply to: 847815210

safeermohammed · 2021-06-01T16:53:38Z

cmd/aks-periscope/aks-periscope.go

 	var waitgroup sync.WaitGroup

 	err := utils.CreateCRD()
 	if err != nil {
 		log.Printf("Failed to create CRD: %+v", err)
 	}
+	for _, exporter := range exporters {


What is this change? Exporter should be either or but not both

Ok, thanks for pointing it out. Will change this so that it's not both.

Actually I think it could be fine as the user could specify multiples exporters.
However, I don't understand why for each exporter we run the collectors and the diagnosers? To me, it should be run collectors --> run diagnosers --> run exporters

I think this is ok if they user wants to use multiple exporters.
However, collectors and diagnosers should run once once for all exporter
the loop should only do something like

for _, exporter := range exporters { exporter.Export() }

safeermohammed

🕐

arnaud-tincelin · 2021-06-02T08:00:01Z

cmd/aks-periscope/aks-periscope.go

 	var waitgroup sync.WaitGroup

 	err := utils.CreateCRD()
 	if err != nil {
 		log.Printf("Failed to create CRD: %+v", err)
 	}
+	for _, exporter := range exporters {


Actually I think it could be fine as the user could specify multiples exporters.
However, I don't understand why for each exporter we run the collectors and the diagnosers? To me, it should be run collectors --> run diagnosers --> run exporters

pkg/exporter/localmachine_exporter.go

arnaud-tincelin · 2021-06-02T08:04:24Z

cmd/aks-periscope/aks-periscope.go

 	var waitgroup sync.WaitGroup

 	err := utils.CreateCRD()
 	if err != nil {
 		log.Printf("Failed to create CRD: %+v", err)
 	}
+	for _, exporter := range exporters {


I think this is ok if they user wants to use multiple exporters.
However, collectors and diagnosers should run once once for all exporter
the loop should only do something like

for _, exporter := range exporters { exporter.Export() }

pkg/exporter/localmachine_exporter.go

arnaud-tincelin · 2021-06-08T13:02:05Z

Hi @arnaud-tincelin, local machine here is the machine on which az CLI runs. Basically the zip file should be copied to the CLI machine. Both storage and local zip are valid scenarios for Arc and default is always to storage account. Local machine is special case. Offline I will send you specs for Arc.

@sophsoph321, The higher order of priority for zip file (both Azureblob export or local export) is zip files should not stored by default on Nodes. Nodes in Arc are owned by customer and we would not have access. Can you point me to the code where we are making sure in Arc mode the zips are not stored on Nodes?

In reply to: 847815210

Keep in mind aks-periscope doesn't not aim to be used as a cli. Exporters shall not be bound as to how we deploy aks-periscope.
I'm available if you want to discuss the use case and solutions we have.

To address your question about zip files, this PR should resolve the issue : #65

Tatsinnit · 2021-06-09T09:24:10Z

pkg/exporter/localmachine_exporter.go

@@ -0,0 +1,71 @@
+package exporter


💡Thank you for this, here are my thoughts, I second Arnaud's thoughts here: #57 (comment) ; what is this local exporter achieving? At given state, tool is not designed to do local copy to user saves, aka, daemon-set namespace aks-persicope get deployed as pods at every node. Or might be I am misunderstood something. (In that case please correct me)

Issues you guys need to think with storing at users machine:

Under what local file-system permission structure and where in a specific local storage location you will save these files? How would we document this and what ARC mechanism will handle these files?

How would the clean up of these file happen from user's machine?

Security and compliance to think: If it is saving it into the machine the tool is run? (The consuming tool can do that like az cli can have some implementation at their end, but again I am not sure of any feature at their end which has this local file save mechanism) but regarding this tool If that is the case I don't think it will be that easy, the moment we are saving things into end-user's machine we have security and compliance issues, along with clean-up?

Thanks,

Fair points, From periscope perspective, like Sophie mentioned the zip files are storage in container so that they can be pulled by clients like CLI. The folder where the Clients will Pull the zip files is client local problem. BTW, default is Azure storage, User has to explicitly specify the option if they wish to export the files locally, so the user is aware where and what files are exported on their machine. If there is a security issue, let us know or point to compliance team so we can close on this aspect.

Regarding scenario, think of case where Pods has misconfigured enterprise proxy settings which is pretty common in Arc case. Azure blobs etc., will not be reachable in this case. Looping @shashankbarsin (Arc PM) on scenarios and questions.

Tatsinnit · 2021-07-12T23:08:40Z

💡 Thanks for this PR, given that #75 is closed now, can you please rebase and merge it with master so that this feature change is ready for review with existing updates.

Also, please make sure most of the comments are taken care off and let us know and we can add our input.

Currently, please make sure you add a description as to what flag will enable this feature? (Might user case scenario is best way$
What file system level decision this feature will make for the user? I.e. currently with new updates tool writes into Inmemory and transition that to storage. I believe you will write are enabling the feature to be saved in “host machine” where user run this? (Please do correct if this is in-correct?)

Thank you so much for this and please let us know once this PR is ready for review. 🙏

Tatsinnit · 2021-07-27T22:21:19Z

💡 Gentle ping: @sophsoph321 - is this PR still valid? if it is stale can we please close this.

sophsoph321 · 2021-07-27T22:40:29Z

💡 Gentle ping: @sophsoph321 - is this PR still valid? if it is stale can we please close this.

Hi @Tatsinnit really sorry for the delayed response. I will eventually refactor this code, but it probably won't be anytime soon. I can close this PR temporarily and will reopen once I've gotten around to making the requested changes. Thanks for your attention on this.

sophsoph321 force-pushed the localexporter branch 4 times, most recently from a1569b8 to 96dda2b Compare May 19, 2021 18:37

jonathan-innis reviewed May 20, 2021

View reviewed changes

Tatsinnit assigned sophsoph321 May 26, 2021

safeermohammed reviewed Jun 1, 2021

View reviewed changes

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

safeermohammed reviewed Jun 1, 2021

View reviewed changes

safeermohammed suggested changes Jun 1, 2021

View reviewed changes

Tatsinnit requested a review from arnaud-tincelin June 2, 2021 01:41

arnaud-tincelin suggested changes Jun 2, 2021

View reviewed changes

sophsoph321 force-pushed the localexporter branch from 8bf7747 to f00d2d2 Compare June 2, 2021 17:01

sophsoph321 marked this pull request as ready for review June 2, 2021 17:03

add local machine exporter

2b32163

sophsoph321 force-pushed the localexporter branch from f00d2d2 to 2b32163 Compare June 2, 2021 19:51

Tatsinnit reviewed Jun 9, 2021

View reviewed changes

Tatsinnit requested a review from davidkydd July 12, 2021 23:09

sophsoph321 closed this Jul 27, 2021

Tatsinnit mentioned this pull request Aug 1, 2021

Selectable C, D, E's #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local Machine exporter #57

Local Machine exporter #57

sophsoph321 commented May 19, 2021 •

edited

Loading

jonathan-innis left a comment

jonathan-innis May 20, 2021 •

edited

Loading

jonathan-innis May 20, 2021

jonathan-innis May 20, 2021

sophsoph321 May 24, 2021

arnaud-tincelin commented May 24, 2021

sophsoph321 commented May 24, 2021

arnaud-tincelin commented May 25, 2021

safeermohammed commented Jun 1, 2021

safeermohammed Jun 1, 2021 •

edited

Loading

sophsoph321 Jun 1, 2021 •

edited

Loading

arnaud-tincelin Jun 2, 2021

arnaud-tincelin Jun 2, 2021

safeermohammed left a comment

arnaud-tincelin Jun 2, 2021

arnaud-tincelin Jun 2, 2021

arnaud-tincelin commented Jun 8, 2021

Tatsinnit Jun 9, 2021 •

edited

Loading

safeermohammed Jun 9, 2021 •

edited

Loading

Tatsinnit commented Jul 12, 2021 •

edited

Loading

Tatsinnit commented Jul 27, 2021

sophsoph321 commented Jul 27, 2021

Local Machine exporter #57

Local Machine exporter #57

Conversation

sophsoph321 commented May 19, 2021 • edited Loading

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis May 20, 2021 • edited Loading

Choose a reason for hiding this comment

jonathan-innis May 20, 2021

Choose a reason for hiding this comment

jonathan-innis May 20, 2021

Choose a reason for hiding this comment

sophsoph321 May 24, 2021

Choose a reason for hiding this comment

arnaud-tincelin commented May 24, 2021

sophsoph321 commented May 24, 2021

arnaud-tincelin commented May 25, 2021

safeermohammed commented Jun 1, 2021

safeermohammed Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

sophsoph321 Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

arnaud-tincelin Jun 2, 2021

Choose a reason for hiding this comment

arnaud-tincelin Jun 2, 2021

Choose a reason for hiding this comment

safeermohammed left a comment

Choose a reason for hiding this comment

arnaud-tincelin Jun 2, 2021

Choose a reason for hiding this comment

arnaud-tincelin Jun 2, 2021

Choose a reason for hiding this comment

arnaud-tincelin commented Jun 8, 2021

Tatsinnit Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

safeermohammed Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Tatsinnit commented Jul 12, 2021 • edited Loading

Tatsinnit commented Jul 27, 2021

sophsoph321 commented Jul 27, 2021

sophsoph321 commented May 19, 2021 •

edited

Loading

jonathan-innis May 20, 2021 •

edited

Loading

safeermohammed Jun 1, 2021 •

edited

Loading

sophsoph321 Jun 1, 2021 •

edited

Loading

Tatsinnit Jun 9, 2021 •

edited

Loading

safeermohammed Jun 9, 2021 •

edited

Loading

Tatsinnit commented Jul 12, 2021 •

edited

Loading