-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support bundle upload #3356
Support bundle upload #3356
Changes from 45 commits
896df10
0aa8462
53b737c
8365880
2892b5b
816125c
c10e625
1ee21b6
bd1fcc5
cbe49dc
7399a69
11207e4
f95dbfa
b1d9e1c
e3c1492
0f1ca43
c3bb918
130bfec
62d15b6
1000cf1
796b513
4f84daf
f8d254c
fe5f5e9
d9c2281
8a34d7f
7fcc125
24fdfaf
fbac54a
7980e66
9b25a70
73204b6
ceb81dc
ef7f3f8
13454c7
b56104e
5038341
686cc1d
e9f1962
bd38674
a31f544
71f9b91
e0a6f7a
84bd7d0
c1df8a8
fd7cdd9
13260b9
4097263
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// Copyright (c) Microsoft. All rights reserved. | ||
namespace Microsoft.Azure.Devices.Edge.Agent.Core.Logs | ||
{ | ||
using System; | ||
using System.IO; | ||
using System.Threading.Tasks; | ||
|
||
public interface IRequestsUploader | ||
{ | ||
Task UploadLogs(string uri, string module, byte[] payload, LogsContentEncoding logsContentEncoding, LogsContentType logsContentType); | ||
|
||
Task<Func<ArraySegment<byte>, Task>> GetLogsUploaderCallback(string uri, string module, LogsContentEncoding logsContentEncoding, LogsContentType logsContentType); | ||
|
||
Task UploadSupportBundle(string uri, Stream source); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// Copyright (c) Microsoft. All rights reserved. | ||
|
||
namespace Microsoft.Azure.Devices.Edge.Agent.Core.Requests | ||
{ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Text; | ||
using Microsoft.Azure.Devices.Edge.Util; | ||
using Newtonsoft.Json; | ||
|
||
public class SupportBundleRequest | ||
{ | ||
public SupportBundleRequest(string schemaVersion, string sasUrl, string since, string iothubHostname, bool? edgeRuntimeOnly) | ||
{ | ||
this.SchemaVersion = Preconditions.CheckNonWhiteSpace(schemaVersion, nameof(schemaVersion)); | ||
this.SasUrl = Preconditions.CheckNotNull(sasUrl, nameof(sasUrl)); | ||
this.Since = Option.Maybe(since); | ||
this.IothubHostname = Option.Maybe(iothubHostname); | ||
this.EdgeRuntimeOnly = Option.Maybe(edgeRuntimeOnly); | ||
} | ||
|
||
[JsonProperty("schemaVersion")] | ||
public string SchemaVersion { get; } | ||
|
||
[JsonIgnore] | ||
public string SasUrl { get; } | ||
|
||
[JsonProperty("since")] | ||
public Option<string> Since { get; } | ||
|
||
[JsonProperty("iothubHostname")] | ||
public Option<string> IothubHostname { get; } | ||
|
||
[JsonProperty("edgeRuntimeOnly")] | ||
public Option<bool> EdgeRuntimeOnly { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// Copyright (c) Microsoft. All rights reserved. | ||
|
||
namespace Microsoft.Azure.Devices.Edge.Agent.Core.Requests | ||
{ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Text; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.Azure.Devices.Edge.Agent.Core.Logs; | ||
using Microsoft.Azure.Devices.Edge.Util; | ||
|
||
public class SupportBundleRequestHandler : RequestHandlerBase<SupportBundleRequest, TaskStatusResponse> | ||
{ | ||
public delegate Task<Stream> GetSupportBundle(Option<string> since, Option<string> iothubHostname, Option<bool> edgeRuntimeOnly, CancellationToken token); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a delegate seems like an Interface might be cleaner? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
readonly GetSupportBundle getSupportBundle; | ||
readonly IRequestsUploader requestsUploader; | ||
|
||
public SupportBundleRequestHandler(GetSupportBundle getSupportBundle, IRequestsUploader requestsUploader) | ||
{ | ||
this.getSupportBundle = getSupportBundle; | ||
this.requestsUploader = requestsUploader; | ||
} | ||
|
||
public override string RequestName => "UploadSupportBundle"; | ||
|
||
protected override Task<Option<TaskStatusResponse>> HandleRequestInternal(Option<SupportBundleRequest> payloadOption, CancellationToken cancellationToken) | ||
{ | ||
SupportBundleRequest payload = payloadOption.Expect(() => new ArgumentException("Request payload not found")); | ||
|
||
(string correlationId, BackgroundTaskStatus status) = BackgroundTask.Run( | ||
async () => | ||
{ | ||
Stream source = await this.getSupportBundle(payload.Since, payload.IothubHostname, payload.EdgeRuntimeOnly, cancellationToken); | ||
await this.requestsUploader.UploadSupportBundle(payload.SasUrl, source); | ||
}, | ||
"upload support bundle", | ||
cancellationToken); | ||
|
||
return Task.FromResult(Option.Some(TaskStatusResponse.Create(correlationId, status))); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iotedge check has it as an option, so the support bundle does too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provisioning_backup.json
has the hub name, soiotedge check
/support-bundle
could read it from there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If you do make that change, you'll probably want to keep the
--iothub-hostname
parameter tocheck
/support-bundle
so that passing it is not an error, such as when running old EA with new host package.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for this pr, I'll remove the iothubHostname param, and just fill it in from ea's known hostname