Implement proper handling of Docker Streams #147

galvesribeiro · 2017-01-18T00:34:29Z

This PR implements #108

Read and notify properly Docker event Streams in:

Image Operations
Container Logs
Container Stats
MISC endpoint Monitor Event

This PR will be update with one commit for each part to make easier to review.

msftclas · 2017-01-18T00:34:44Z

Hi @galvesribeiro, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

jterry75 · 2017-01-18T16:57:48Z

src/Docker.DotNet/Endpoints/ContainerOperations.cs

@@ -238,7 +238,7 @@ public Task RemoveContainerAsync(string id, ContainerRemoveParameters parameters
            return this._client.MakeRequestAsync(new[] { NoSuchContainerHandler }, HttpMethod.Delete, $"containers/{id}", queryParameters);
        }

-        public Task<Stream> GetContainerLogsAsync(string id, ContainerLogsParameters parameters, CancellationToken cancellationToken)
+        public async Task GetContainerLogsAsync(string id, ContainerLogsParameters parameters, CancellationToken cancellationToken, IProgress<string> logReport = null)


How about this. Leave the old method and add an [Obsolete] tag with reference to the new sig. Have this new method call the old one to get the stream and just do the while (can read) { report } part. Then we have no breaking changes but we do alert people to use the new method. And then in the next version of docker I will pull the [Obsolete] method and merge the logic to just the IProgress{T} variant.

Also how do you feel about just calling it IProgress<string> progress = null

Sure! Let me change that.

jterry75 · 2017-01-18T16:59:03Z

src/Docker.DotNet/Endpoints/ContainerOperations.cs

+                var line = await reader.ReadLineAsync();
+                if (logReport == null) continue;
+
+                logReport.Report(line);


Why did you kill the part about making this a typed message? Dont we know what the string type is and you could just use:

logReport.Report(this._client.JsonSerializer.DeserialzeObject<Progress>(line);

jterry75 · 2017-01-18T17:03:18Z

src/Docker.DotNet/Endpoints/ImageOperations.cs

+                    var line = await reader.ReadLineAsync();
+                    if (progress == null) continue;
+
+                    var @event = JObject.Parse(line);


Why the complexity here? Cant JSON.NET just handle the deserialization in 1 object definition?

The reason is that I can't find the object structure well defined for those events in go files... So I got the fields that matter and manually read them. If you can point me to the right structure I can make it deserialize direct to the object just like in the others.

Go ahead and leave this for now. I will remove it in the 1.13 branch that my PR eludes to because the type wasnt yet defined.

jterry75 · 2017-01-18T17:09:41Z

src/Docker.DotNet/Models/Events.cs

+
+namespace Docker.DotNet.Models
+{
+    public class EventMessage


Ah. Add a line to specgen.go by /events
{reflect.TypeOf(events.Message{}), "EventsMessage"}, so that this stays in sync with the docker types. This will also generate for you the Actor struct etc.

Ok did some digging. I honestly cant tell from the docker code. I think we actually want jsonmessage.JsonMessage type here instead. @swernli does this seem right?

This will reflect the JSONError and JSONProgress structs too which is the full message as far as I can tell.

I got it from here https://github.com/docker/docker/blob/master/api/types/events/events.go which are the only objects used by this method. Do you want submit a PR with the Go stuff changed? I don't have it here and would take a while to get it working...

Added this in the other PR. Rebase on it and you can remove.

galvesribeiro · 2017-01-18T18:30:25Z

@jterry75 done. Now only thing missing is that Go thing...

galvesribeiro · 2017-01-19T14:44:21Z

@jterry75 ?

jterry75

Almost! Thanks for doing this!

jterry75 · 2017-01-19T18:40:11Z

src/Docker.DotNet/Endpoints/ContainerOperations.cs

@@ -254,6 +256,21 @@ public Task<Stream> GetContainerLogsAsync(string id, ContainerLogsParameters par
            return this._client.MakeRequestForStreamAsync(new[] { NoSuchContainerHandler }, HttpMethod.Get, $"containers/{id}/logs", queryParameters, null, cancellationToken);
        }

+        public async Task GetContainerLogsAsync(string id, ContainerLogsParameters parameters, CancellationToken cancellationToken, IProgress<string> progress = null)


Does this compile? I would assume that for this transfer period what we will need to do is to make the progress a non default parameter. Then we will have 2 physical overloads and then when I merge/remove the [Obsolete] version I will make it optional again to keep the same set of overloads.

jterry75 · 2017-01-19T18:42:53Z

src/Docker.DotNet/Endpoints/ContainerOperations.cs

+                    var line = await reader.ReadLineAsync();
+                    if (progress == null) continue;
+
+                    var report = JsonConvert.DeserializeObject<ContainerStatsResponse>(line);


Can you use the this._client.JsonSerializer.DeserializeObject it makes it a whole lot easier for writing tests to not hard reference JsonConvert

And then you dont need the using at the top

jterry75 · 2017-01-19T18:44:04Z

src/Docker.DotNet/Endpoints/IContainerOperations.cs

@@ -40,6 +41,8 @@ public interface IContainerOperations

        Task<Stream> GetContainerLogsAsync(string id, ContainerLogsParameters parameters, CancellationToken cancellationToken);


Ya now I think I mislead you. We only expose the Interfaces externally. I think you need to make the [Obsolete] tags here instead. My bad

jterry75 · 2017-01-19T18:45:35Z

src/Docker.DotNet/Endpoints/ImageOperations.cs

+                    var line = await reader.ReadLineAsync();
+                    if (progress == null) continue;
+
+                    var @event = JObject.Parse(line);


Go ahead and leave this for now. I will remove it in the 1.13 branch that my PR eludes to because the type wasnt yet defined.

jterry75 · 2017-01-19T18:46:23Z

src/Docker.DotNet/Endpoints/MiscellaneousOperations.cs

@@ -4,6 +4,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Docker.DotNet.Models;
+using Newtonsoft.Json;


Same. If you remove the JsonConvert reference and use this._client.JsonSerializer we can remove this too

jterry75 · 2017-01-19T18:46:44Z

src/Docker.DotNet/Models/Events.cs

+
+namespace Docker.DotNet.Models
+{
+    public class EventMessage


Added this in the other PR. Rebase on it and you can remove.

jterry75 · 2017-01-19T18:47:01Z

src/Docker.DotNet/Models/ImageOperationProgress.cs

@@ -0,0 +1,9 @@
+namespace Docker.DotNet.Models
+{
+    public struct ImageOperationProgress


Keep for now. I will remove later in the 1.13 branch

galvesribeiro · 2017-01-19T19:45:39Z

@jterry75 done :)

Fixes issue dotnet#147.

msftclas added the cla-already-signed label Jan 18, 2017

galvesribeiro mentioned this pull request Jan 18, 2017

Tasks that return status streams should be replaced with calls to IProgress<StreamStatus> #108

Closed

galvesribeiro changed the title ~~[WIP] Implement proper handling of Docker Streams~~ Implement proper handling of Docker Streams Jan 18, 2017

jterry75 suggested changes Jan 18, 2017

View reviewed changes

jterry75 suggested changes Jan 19, 2017

View reviewed changes

galvesribeiro added 6 commits January 19, 2017 17:31

Image operations with Streams

955a450

Container stream report.

113626f

Container stats streams

c647007

MISC events

0b6d4e6

Feedback

d380bf9

Feedback

30703f0

jterry75 approved these changes Jan 19, 2017

View reviewed changes

jterry75 merged commit e8d7f8b into dotnet:master Jan 19, 2017

mdarocha pushed a commit to TrapTech/Docker.DotNet that referenced this pull request Jul 11, 2022

Revert changes in BigInteger.Random(int bitLength) from commit 1b881f5.

3f28d8c

Fixes issue dotnet#147.

mdarocha pushed a commit to TrapTech/Docker.DotNet that referenced this pull request Jul 11, 2022

Revert changes in BigInteger.Random(int bitLength) from commit 1b881f5.

e37f0bf

Fixes issue dotnet#147.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement proper handling of Docker Streams #147

Implement proper handling of Docker Streams #147

galvesribeiro commented Jan 18, 2017 •

edited

msftclas commented Jan 18, 2017

jterry75 Jan 18, 2017

jterry75 Jan 18, 2017

galvesribeiro Jan 18, 2017

jterry75 Jan 18, 2017

jterry75 Jan 18, 2017

galvesribeiro Jan 18, 2017

jterry75 Jan 19, 2017

jterry75 Jan 18, 2017

jterry75 Jan 18, 2017

jterry75 Jan 18, 2017

galvesribeiro Jan 18, 2017

jterry75 Jan 19, 2017

galvesribeiro commented Jan 18, 2017

galvesribeiro commented Jan 19, 2017

jterry75 left a comment •

edited

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

jterry75 Jan 19, 2017

galvesribeiro commented Jan 19, 2017

		@@ -40,6 +41,8 @@ public interface IContainerOperations

		Task<Stream> GetContainerLogsAsync(string id, ContainerLogsParameters parameters, CancellationToken cancellationToken);

Implement proper handling of Docker Streams #147

Implement proper handling of Docker Streams #147

Conversation

galvesribeiro commented Jan 18, 2017 • edited

msftclas commented Jan 18, 2017

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

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

galvesribeiro commented Jan 18, 2017

galvesribeiro commented Jan 19, 2017

jterry75 left a comment • edited

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

Choose a reason for hiding this comment

galvesribeiro commented Jan 19, 2017

galvesribeiro commented Jan 18, 2017 •

edited

jterry75 left a comment •

edited