Support bundle upload #3356

lfitchett · 2020-08-04T17:16:48Z

This allows the support bundle to be uploaded in a similar matter to the built in logs upload. Currently the support bundle is held in memory while it is streamed to the cloud, but is architectured such that it can be stored and streamed from a file on disk in the future.

C# Changes

Added Support Bundle Direct Method
Renamed LogsUploader to RequestUploader

Rust Changes

Split Support Bundle and Logs Collection to new library that is shared by iotedge tool and the management endpoint.
Refactored Support Bundle to use generic Read instead of File
added endpoint to get support bundle

Note: The 2020-07-07 mgmt api version is brand new, and these additions are planned.

…load

…t-bundle_upload

edgelet/support-bundle/src/support_bundle.rs

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Core/logs/ILogsUploader.cs

varunpuranik · 2020-08-05T17:58:05Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Core/requests/SupportBundleRequest.cs

+        public Option<string> Since { get; }
+
+        [JsonProperty("iothubHostname")]
+        public Option<string> IothubHostname { get; }


How is this used?

iotedge check has it as an option, so the support bundle does too

What happens if a user specifies a different IoTHub?
I think this parameter definitely seems odd - I would say remove it and use the IOTHub that EdgeAgent is using.
@veyalla ?

Using IoT Hub that edge agent is using would actually be an improvement over running support-bundle locally because currently we cannot run any connectivity tests when using DPS because we can't get the iot hub name from config yaml. The recommendation for DPS-provisioned devices is to use the command line parameter to explicitly specify iot hub name which is behavior the direct method is implementing.

However, it would cause inconsistent behavior for the same command when run locally vs remotely which is weird. There was some thoughts from @arsing around getting the iot hub name from a cached provisioning result when run locally on a DPS provisioned device. If that can be implemented we could have a consistent experience.

provisioning_backup.json has the hub name, so iotedge check / support-bundle could read it from there.

(If you do make that change, you'll probably want to keep the --iothub-hostname parameter to check / support-bundle so that passing it is not an error, such as when running old EA with new host package.)

IMO, we should make the change Arnav is suggesting, it will improve troubleshooting experience for customers running check / support-bundle locally and also provide consistency when running it locally or remotely. Thoughts @CindyXing ?

Yes, wherever we can infer the IoThub, we should I can't think of a scenario where support bundle or iotedge check would need to talk to a different IoTHub.

So for this pr, I'll remove the iothubHostname param, and just fill it in from ea's known hostname

varunpuranik · 2020-08-05T18:01:05Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Core/requests/SupportBundleRequestHandler.cs

+
+    public class SupportBundleRequestHandler : RequestHandlerBase<SupportBundleRequest, TaskStatusResponse>
+    {
+        public delegate Task<Stream> GetSupportBundle(Option<string> since, Option<string> iothubHostname, Option<bool> edgeRuntimeOnly, CancellationToken token);


Instead of a delegate seems like an Interface might be cleaner?

I'm not sure which interface I would use, but a delegate seemed like an clean way to pass a single function in. I've used this pattern before in the code base.

damonbarry

One question, otherwise the .NET code LGTM

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Edgelet/ModuleManagementHttpClient.cs

CindyXing · 2020-08-06T00:32:22Z

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Service/ExperimentalFeatures.cs

        {
            this.Enabled = enabled;
            this.DisableCloudSubscriptions = disableCloudSubscriptions;
            this.EnableUploadLogs = enableUploadLogs;
            this.EnableGetLogs = enableGetLogs;
+            this.EnableUploadSupportBundle = enableUploadSupportBundle;


So, in 1.0.10, we'll keep this and uploadlogs, getlogs as experimental features?

the plan is to move at least get and upload logs out of experimental for 1.10, this will probably stay in experimental

* Rename logs => module logs * make handler classes * include new dm * chkpnt * start able to write to in memory * fix types * output to console * fix flags * start able to write to in memory * fix types * output to console * fix flags * re-enable tests * import zip module and use display * use --output - for stdout * call support bundle * parse params * add new api version * reuse http client * flesh out request * make logs uploader more generic * fix merge issue * bring in api changes * fix version * add get_result * move support bundle logic to own file * remove s * collect bundle * wrap read in body * return support bundle size * send full sb * fix fileResponse * read params * parse_since * pass hostname * clippy * re-enable first test * add logs test back * rename ILogsUPloader file to IRequestsUploader * remove unused IDisposable * get iothub hostname from edgeAgent Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

lfitchett added 30 commits February 14, 2020 10:45

Rename logs => module logs

896df10

make handler classes

0aa8462

include new dm

53b737c

chkpnt

8365880

start able to write to in memory

2892b5b

fix types

816125c

output to console

c10e625

fix flags

1ee21b6

start able to write to in memory

bd1fcc5

fix types

cbe49dc

output to console

7399a69

fix flags

11207e4

re-enable tests

f95dbfa

import zip module and use display

b1d9e1c

Merge branch 'support_bundle_console' into support-bundle_upload

e3c1492

use --output - for stdout

0f1ca43

call support bundle

c3bb918

parse params

130bfec

add new api version

62d15b6

reuse http client

1000cf1

flesh out request

796b513

make logs uploader more generic

4f84daf

Merge remote-tracking branch 'upstream/master' into support-bundle_up…

f8d254c

…load

fix merge issue

fe5f5e9

Merge branch 'master' of https://github.com/Azure/iotedge into suppor…

d9c2281

…t-bundle_upload

Merge branch 'master' of https://github.com/Azure/iotedge into suppor…

8a34d7f

…t-bundle_upload

bring in api changes

7fcc125

fix version

24fdfaf

add get_result

fbac54a

move support bundle logic to own file

7980e66

lfitchett added 4 commits August 3, 2020 11:11

parse_since

686cc1d

pass hostname

e9f1962

Merge branch 'master' of https://github.com/Azure/iotedge into suppor…

bd38674

…t-bundle_upload

Merge branch 'master' of https://github.com/Azure/iotedge into suppor…

a31f544

…t-bundle_upload

lfitchett requested review from varunpuranik, damonbarry and arsing August 4, 2020 17:17

arsing requested changes Aug 4, 2020

View reviewed changes

edgelet/support-bundle/src/support_bundle.rs Show resolved Hide resolved

edgelet/support-bundle/src/support_bundle.rs Outdated Show resolved Hide resolved

lfitchett added 3 commits August 5, 2020 08:36

clippy

71f9b91

re-enable first test

e0a6f7a

add logs test back

84bd7d0

lfitchett requested a review from arsing August 5, 2020 17:37

arsing previously approved these changes Aug 5, 2020

View reviewed changes

varunpuranik reviewed Aug 5, 2020

View reviewed changes

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Core/logs/ILogsUploader.cs Outdated Show resolved Hide resolved

varunpuranik reviewed Aug 5, 2020

View reviewed changes

lfitchett requested a review from varunpuranik August 5, 2020 19:20

rename ILogsUPloader file to IRequestsUploader

c1df8a8

lfitchett dismissed arsing’s stale review via c1df8a8 August 5, 2020 19:22

damonbarry reviewed Aug 5, 2020

View reviewed changes

edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Edgelet/ModuleManagementHttpClient.cs Outdated Show resolved Hide resolved

lfitchett added 2 commits August 5, 2020 15:44

remove unused IDisposable

fd7cdd9

get iothub hostname from edgeAgent

13260b9

CindyXing reviewed Aug 6, 2020

View reviewed changes

varunpuranik approved these changes Aug 6, 2020

View reviewed changes

damonbarry approved these changes Aug 6, 2020

View reviewed changes

lfitchett added the ready-to-merge label Aug 6, 2020

Merge branch 'master' into support-bundle_upload

4097263

lfitchett merged commit a8b20ea into Azure:master Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bundle upload #3356

Support bundle upload #3356

lfitchett commented Aug 4, 2020 •

edited

Loading

varunpuranik Aug 5, 2020

lfitchett Aug 5, 2020

varunpuranik Aug 5, 2020

veyalla Aug 5, 2020 •

edited

Loading

arsing Aug 5, 2020

arsing Aug 5, 2020

veyalla Aug 5, 2020

varunpuranik Aug 5, 2020

lfitchett Aug 5, 2020

varunpuranik Aug 5, 2020

lfitchett Aug 5, 2020

damonbarry left a comment

CindyXing Aug 6, 2020

lfitchett Aug 6, 2020

Support bundle upload #3356

Support bundle upload #3356

Conversation

lfitchett commented Aug 4, 2020 • edited Loading

C# Changes

Rust Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veyalla Aug 5, 2020 • edited Loading

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

damonbarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfitchett commented Aug 4, 2020 •

edited

Loading

veyalla Aug 5, 2020 •

edited

Loading