Skip to content

Commit

Permalink
Fix Edge Agent Docker-mode's handling of create-container request bod…
Browse files Browse the repository at this point in the history
…y. (Azure#1695)

With 5a6d506, the Docker-mode `CreateCommand`
has to do a round-trip serialization of the create-container request body to
convert it from `Edge.Agent.Docker.Models.CreateContainerParameters` to
`Docker.DotNet.Models.CreateContainerParameters`.

However, the original definition had a `Name` property that would be
serialized into the JSON body as a "Name" field. This is not actually how
the property is supposed to be serialized. It's supposed to be transmitted
separately as a query-string parameter named "name".

The Docker.DotNet type does have a `Name` property, but it's annotated with
their custom `QueryStringParameterAttribute` so that it's serialized into
the querystring.

Because of this mismatch, the round-trip would ignore the field and
the resulting object would have `Name = null`, causing Docker to choose
a random name. This broke the Docker-mode tests that run in our CI.

Technically the `Name` property does not belong in this type, since
it ought to represent the request body. However Docker.DotNet has it, and
Edge Agent code has been written to use it rather than storing the module name
separately.

So this commit fixes the state of affairs by leaving the `Name` property in
the `Edge.Agent.Docker.Models.CreateContainerParameters` type, but fixing its
annotation so that it's ignored rather than serialized. Then, after
`Microsoft.Azure.Devices.Edge.Agent.Docker.CreateCommand` does
the serialization round-trip, it manually copies the `Name` property over.

This also fixes one more test issue I found while running the tests locally.
The `TestUdpModuleConfig` test set a single log config and expected to get
a single log config back when inspecting the container. But if other
log configs were configured in the Docker `daemon.json`, there would be more
than one. This change fixes the test to expect at least one log config.
  • Loading branch information
arsing authored Sep 11, 2019
1 parent 86372f0 commit a747950
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ public Task ExecuteAsync(CancellationToken token)
// This will lose properties in the former that are not defined in the latter, but this code path is only for the old Docker mode anyway.
var createContainerParameters =
JsonConvert.DeserializeObject<DockerModels.CreateContainerParameters>(JsonConvert.SerializeObject(this.createContainerParameters));

// Copy the Name property manually. See the docs of Edge.Agent.Docker.Models.CreateContainerParameters' Name property for an explanation.
createContainerParameters.Name = this.createContainerParameters.Name;

return this.client.Containers.CreateContainerAsync(createContainerParameters, token);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,16 @@ public class CreateContainerParameters
[JsonProperty("Labels", DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
public IDictionary<string, string> Labels { get; set; }

[JsonProperty("Name", DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
// This field is not actually part of the serialized JSON value:
//
// - In the Docker create-container API, it's a query string parameter named "name".
// - In the Edgelet create-module API, it's the top-level "name" field of the request body.
//
// However Docker.DotNet's type CreateContainerParameters stores it as a property, annotated with its custom QueryStringParameterAttribute
// to ensure it ends up in the querystring. So Edge Agent's code has been written to use it as a property of this type.
//
// As a result this model type continues to provide it.
[JsonIgnore]
public string Name { get; set; }

[JsonProperty("NetworkingConfig", DefaultValueHandling = DefaultValueHandling.IgnoreAndPopulate)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,11 @@ public async Task TestUdpModuleConfig()
Assert.False(container.HostConfig.PortBindings.ContainsKey("443/tcp"));
// logging
Assert.Equal("json-file", container.HostConfig.LogConfig.Type);
Assert.True(container.HostConfig.LogConfig.Config.Count == 1);

// While we only set one log config for max-size, there may be other log-configs in docker's daemon.json that the container will inherit.
// So there can be more than one.
Assert.True(container.HostConfig.LogConfig.Config.Count >= 1);

Assert.Equal("100M", container.HostConfig.LogConfig.Config["max-size"]);
}
}
Expand Down

0 comments on commit a747950

Please sign in to comment.