Remove usage of files

[arnaud-tincelin]

This PR removes all uses of files so there is no need to mount a volume on the container. The starting point was to remove functions in utils.go: WriteToFile, CreateCollectorDir, CreateDiagnosticDir

Notes:

• To stay as close as possible to the actual behaviour, the collectors and diagnosers output a map[string]string, the key being the file name & the value the content of the file.
• Interfaces have had small changes to be focused on their original goal (ex: a collector can no longer Export() and an exporter requires a dataProducer to Export()).
• I removed the zip & export since it re-does the export. If requested we can bring it back, but I would need an explanation :)
• the path in the blob storage has changed from /creationDate/hostname/<collector/diagnoser>/filename to /creationDate/hostname/filename

[Tatsinnit]

👍 This PR looks much valuable, it removes the unwanted function into much organic in-memory handling and storing information and writing to the storage.

• The way I understood, now everything is handled in memory rather than collecting logs locally and then exporting it to storage.

• Also a key thing is the removal of double up effort aka, zip & export now just export to storage, I am not sure why that *.zip was separately getting exported.

📓 Update 14th June: I noticed that the zip is removed which will break the vscode.

I will take another look tonight, and add more folks who will get affected by this in the current PRs.

I feel under the hood it makes things much more non-convoluted.

Hiya, @JunSun17 can you please eye-ball - seems a simpler approach, but in case you can think of anything which will be an issue please? 🙏 Thank you so much.

FYI: cc: @davidkydd , @sophsoph321 , @SanyaKochhar

[davidkydd]

This PR removes all uses of files so there is no need to mount a volume on the container. The starting point was to remove functions in utils.go: WriteToFile, CreateCollectorDir, CreateDiagnosticDir

Notes:

• To stay as close as possible to the actual behavior, the collectors and diagnosers output a map[string]string, the key being the file name & the value the content of the file.
• Interfaces have had small changes to be focused on their original goal (ex: a collector can no longer Export() and an exporter requires a dataProducer to Export()).
• I removed the zip & export since it re-does the export. If requested we can bring it back, but I would need an explanation :)
• the path in the blob storage has changed from /creationDate/hostname/<collector/diagnoser>/filename to `/creationDate/hostname/filename

What's the motivation behind getting rid of the volume mount? I'm not sure about keeping everything in memory as some clusters already generate significant amounts of data, and additional collectors added in future may exacerbate this. This seems like a recipe for getting OOMKilled?

[arnaud-tincelin]

This PR removes all uses of files so there is no need to mount a volume on the container. The starting point was to remove functions in utils.go: WriteToFile, CreateCollectorDir, CreateDiagnosticDir
Notes:

• To stay as close as possible to the actual behavior, the collectors and diagnosers output a map[string]string, the key being the file name & the value the content of the file.
• Interfaces have had small changes to be focused on their original goal (ex: a collector can no longer Export() and an exporter requires a dataProducer to Export()).
• I removed the zip & export since it re-does the export. If requested we can bring it back, but I would need an explanation :)
• the path in the blob storage has changed from /creationDate/hostname/<collector/diagnoser>/filename to `/creationDate/hostname/filename

What's the motivation behind getting rid of the volume mount? I'm not sure about keeping everything in memory as some clusters already generate significant amounts of data, and additional collectors added in future may exacerbate this. This seems like a recipe for getting OOMKilled?

@davidkydd I believe it's pretty much the same as before regarding memory as we were storing the command's outputs in a variable and reading content of files in one shot. I believe any command today could cause an OOM in a significant cluster and at least this PR reduces i/o on the disk.
This is definitely not the target, I would like to stream everything end-to-end so that we can absorb any amount of data. The motivation to this PR is to simplify the code and deployment before the next iteration.
Does it make sense?

[davidkydd]

This PR removes all uses of files so there is no need to mount a volume on the container. The starting point was to remove functions in utils.go: WriteToFile, CreateCollectorDir, CreateDiagnosticDir
Notes:

• To stay as close as possible to the actual behavior, the collectors and diagnosers output a map[string]string, the key being the file name & the value the content of the file.
• Interfaces have had small changes to be focused on their original goal (ex: a collector can no longer Export() and an exporter requires a dataProducer to Export()).
• I removed the zip & export since it re-does the export. If requested we can bring it back, but I would need an explanation :)
• the path in the blob storage has changed from /creationDate/hostname/<collector/diagnoser>/filename to `/creationDate/hostname/filename

What's the motivation behind getting rid of the volume mount? I'm not sure about keeping everything in memory as some clusters already generate significant amounts of data, and additional collectors added in future may exacerbate this. This seems like a recipe for getting OOMKilled?

@davidkydd I believe it's pretty much the same as before regarding memory as we were storing the command's outputs in a variable and reading content of files in one shot. I believe any command today could cause an OOM in a significant cluster and at least this PR reduces i/o on the disk.
This is definitely not the target, I would like to stream everything end-to-end so that we can absorb any amount of data. The motivation to this PR is to simplify the code and deployment before the next iteration.
Does it make sense?

Ah yes thanks Arnaud! With a streaming solution planned for later this sounds like a great change, reducing disk io is a good immediate benefit too

[arnaud-tincelin]

This PR removes all uses of files so there is no need to mount a volume on the container. The starting point was to remove functions in utils.go: WriteToFile, CreateCollectorDir, CreateDiagnosticDir
Notes:

• To stay as close as possible to the actual behavior, the collectors and diagnosers output a map[string]string, the key being the file name & the value the content of the file.
• Interfaces have had small changes to be focused on their original goal (ex: a collector can no longer Export() and an exporter requires a dataProducer to Export()).
• I removed the zip & export since it re-does the export. If requested we can bring it back, but I would need an explanation :)
• the path in the blob storage has changed from /creationDate/hostname/<collector/diagnoser>/filename to `/creationDate/hostname/filename

What's the motivation behind getting rid of the volume mount? I'm not sure about keeping everything in memory as some clusters already generate significant amounts of data, and additional collectors added in future may exacerbate this. This seems like a recipe for getting OOMKilled?

@davidkydd I believe it's pretty much the same as before regarding memory as we were storing the command's outputs in a variable and reading content of files in one shot. I believe any command today could cause an OOM in a significant cluster and at least this PR reduces i/o on the disk.
This is definitely not the target, I would like to stream everything end-to-end so that we can absorb any amount of data. The motivation to this PR is to simplify the code and deployment before the next iteration.
Does it make sense?

Ah yes thanks Arnaud! With a streaming solution planned for later this sounds like a great change, reducing disk io is a good immediate benefit too

cool :) any help on the subject is more than welcome as I don't know for sure if it's doable. It will also depend on what the libraries can do. But I do hope we can find an elegant solution :)
As this is an important change, we need everyone to support this PR if we believe it goes the right direction. If not, please step in.
@JunSun17 @Tatsinnit @davidkydd

[Tatsinnit]

💡 ⚠️ 📓 Hiya guys, I like this PR but noticed a removal of .zip file functionality which will cause some breaking change for vscode, thus I implemented a POC for Vscode for the zip breaking changes which will happen due to probably proposition of Zip but then it introduce some major implications detail below.

For Honest appraisal of the situation between both tools, here are some quick thoughts:

• In case we move away from the *.zip upload from this tool then (due to breaking nature) the current VSCode implications of handling zip are documented here: Scope work for zipping blob within vscode and decouple dependency on zip file from periscope tool. vscode-aks-tools#72 (Implementation PR is attached with the description of this Workitem)

• Essentially, beside removing what we can do is to keep *.zip as it is for the upload and think if we need other non-zip folders? The use case seems like the easy way user can share the logs with CSS folks.

• Also, I am sorry but I completely forgot the reason of removing the *.zip file?

• If we can come to any consensus and move forward with rest of changes that is in this PR but also keep zip file that will become non-breaking and I could do the zip command to zip archive package use or fix separately.

Please let me know what you guys things and ideas? Happy to chat up as well.

I could try and add zip back with rest of refactor from the fork? (If this sounds cool) that will make thi PR non-breaking and a win-win.

Thanks heaps,

cc: @arnaud-tincelin, @davidkydd

[Tatsinnit]

💡 Thank you so much for this change and contribution, especially adding back the *.zip functionality with latest upgrade, I have tested locally with my local image tatsat/test-remove-useoffiles looks good. +1 and approve.

Thank you for incorporating OSM changes as well. Hierarchy looks much cleaner.

I will leave one final look with other folks and maintainers, if no replies we can go ahead in next few hours to merge it. Looks good to me. 🙏

cc: @JunSun17 an @davidkydd