Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Consul.Test/Consul.Test.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.10.0" />
<PackageReference Include="Microsoft.NETCore.Platforms" Version="3.1.0" />
<PackageReference Include="System.Text.Json" Version="7.0.3" />
<PackageReference Include="System.Text.Json" Version="8.0.4" />
<PackageReference Include="xunit" Version="2.4.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" />
<PackageReference Include="Xunit.SkippableFact" Version="1.3.12" />
Expand Down
41 changes: 41 additions & 0 deletions Consul.Test/CoordinateTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,46 @@ public async Task Coordinate_GetNode()
Assert.IsType<CoordinateEntry[]>(nodeDetails);
Assert.NotEmpty(nodeDetails);
}

[Fact]
public async Task Coordinate_Update()
{
var nodeName = "foo-update-lan";
var registration = new CatalogRegistration
{
Node = nodeName,
Address = "1.1.1.1"
};
var register = await _client.Catalog.Register(registration);

var coord = new CoordinateEntry
{
Node = nodeName,
Coord = new SerfCoordinate()
};
coord.Coord.Error = 1.5;
coord.Coord.Height = 0.5;
coord.Coord.Adjustment = 0.0;
for (int i = 0; i < 8; i++) coord.Coord.Vec.Add(0.0);

var response = await _client.Coordinate.Update(coord);
Assert.Equal("OK", response.StatusCode.ToString());

var newCoordResult = await _client.Coordinate.Node(nodeName);
for (int i = 0; i < 5; i++)
Copy link
Contributor

@marcin-krystianc marcin-krystianc Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of that loop.
I think there is a reliable way to query the node without using the loop.
You can use so called "blocking queries", i.e.: use query options with a non-zero wait-index:

   await _client.Catalog.Register(registration);
   var nodeResult = await _client.Catalog.Node(nodeName);
   Assert.Equal(HttpStatusCode.OK, nodeResult.StatusCode);
...
   await _client.Coordinate.Update(coord);
...
   var q = new QueryOptions{ WaitIndex = nodeResult.LastIndex, };
   var newCoordResult = await _client.Coordinate.Node(nodeName, q);
   Assert.Equal(HttpStatusCode.OK, response.StatusCode);

I tried this locally and it seemed working reliably.

Copy link
Contributor Author

@IvanKolchanov IvanKolchanov Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, there wasn't really a similar situation in the previous code, so I went for something similar to Golang code's retry loop. Should we change waitTime to around 10 seconds, to avoid hanging for the default 5 minutes? And I have a little question, I am a bit confused on how is the index of node registered in catalog connected to the coordinate node? I am still working on my familiarity with all consul features, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, it is sometimes a bit of unknown for me as well 😄 I'm concerned about the loop because it introduces a risk of flakiness. We run each test many times (we have a matrix with os, consul version and runtime version), thus we need to have really robust tests.
I think the "LastIndex"/"WaitIndex" are global, so it doesn't matter that they are from different endpoints.

Should we change waitTime to around 10 seconds, to avoid hanging for the default 5 minutes
I think it is better to wait longer, but have more robust tests. Therefore I wouldn't change the default waitTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!) Makes sense, I will commit all the changes and we see how the tests go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I've just updated my code snipped from above, to make it clear that the Catalog.Node() call should be before the Coordinate.Update() call.

{
if (newCoordResult.Response != null) break;
await Task.Delay(1000 * 2);
newCoordResult = await _client.Coordinate.Node(nodeName);
}

Assert.NotNull(newCoordResult.Response);
var newCoord = newCoordResult.Response[0];
Assert.Equal(coord.Coord.Vec.Count, newCoord.Coord.Vec.Count);
Assert.Equal(coord.Node, newCoord.Node);
Assert.True(Math.Abs(coord.Coord.Height - newCoord.Coord.Height) <= 0.00001);
Assert.True(Math.Abs(coord.Coord.Adjustment - newCoord.Coord.Adjustment) <= 0.00001);
Assert.True(Math.Abs(coord.Coord.Error - newCoord.Coord.Error) <= 0.00001);
}
}
}
6 changes: 6 additions & 0 deletions Consul/Coordinate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace Consul
public class CoordinateEntry
{
public string Node { get; set; }
public string Segment { get; set; }
public SerfCoordinate Coord { get; set; }
}

Expand Down Expand Up @@ -105,6 +106,11 @@ public Task<QueryResult<CoordinateEntry[]>> Node(string node, CancellationToken
{
return Node(node, QueryOptions.Default, ct);
}

public Task<WriteResult> Update(CoordinateEntry entry, CancellationToken ct = default)
{
return _client.Put("/v1/coordinate/update", entry, null).Execute(ct);
}
}

public partial class ConsulClient : IConsulClient
Expand Down
1 change: 1 addition & 0 deletions Consul/Interfaces/ICoordinateEndpoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ public interface ICoordinateEndpoint
Task<QueryResult<CoordinateEntry[]>> Node(string node, QueryOptions q, CancellationToken ct = default);
Task<QueryResult<CoordinateEntry[]>> Node(string node, CancellationToken ct = default);
Task<QueryResult<CoordinateEntry[]>> Nodes(QueryOptions q, CancellationToken ct = default);
Task<WriteResult> Update(CoordinateEntry entry, CancellationToken ct = default);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need a variant with WriteOptions

}
}