adding health checks #432

areller · 2020-04-30T23:51:46Z

adding health checks according to this #19

Changes

Replicas can be cancelled independent of the service
There are 2 new configurations for a service: liveness, readiness. each can hold these settings

http:
  path: /health
  port: 80 # default: pick the first binding of the service
  protocol: http # default: none
  headers:
    - name: A
       value: B
initialDelay: 5 # default: 0
period: 1 # default: 1
timeout: 1 # default: 1
successThreshold: 1 # default: 1
failureThreshold: 3 # default: 3

There are 2 new replica states: Healthy, Ready (Ready = passed both liveness and readiness probing. Healthy = passed liveness but not readiness).
If neither liveness nor readiness were provided, replica will be upgraded to Ready upon creation. If only liveness if provided, replica will be upgraded to Ready upon passing the liveness probe. if only readiness is provided, replica will be upgraded to Healthy upon creation.
There is a ReplicaMonitor that reads the liveness and readiness probe configuration, and kills/changes the state of the replica according to probe results
Proxy and Ingress only forward to Ready replicas
The generated Kubernetes manifest contains the liveness and readiness configuration.

davidfowl · 2020-05-01T18:15:11Z

src/Microsoft.Tye.Hosting/DockerRunner.cs

@@ -219,210 +219,215 @@ private async Task StartContainerAsync(Application application, Service service,

            async Task RunDockerContainer(IEnumerable<(int ExternalPort, int Port, int? ContainerPort, string? Protocol)> ports)
            {
-                var hasPorts = ports.Any();
+                while (!dockerInfo.StoppingTokenSource.IsCancellationRequested)


Why is there a loop here?

When I implement the monitor, it might decide to kill a container because it's unhealthy. In that case I'd want the DockerRunner to recreate the replica.
Another way to do it, is to kill the container by calling docker restart, and then there is no need for that loop. what do you think?

The argument behind the former, is that it's consistent with how I'll do it in ProcessRunner

The docker runner already restarts the replica albiet with the same name.

@davidfowl I think that the problem with doing docker restart is that the health monitor won't be able to tell that the container was restarted (because there won't be Stopped and Started events). Unless, I explicitly send ReplicaState.Stopped and ReplicaState.Started events whenever I restart the container, but I don't like that approach.

From what I can tell with this loop, you would call docker rm and other commands I don't think you want to run:

tye/src/Microsoft.Tye.Hosting/DockerRunner.cs

Line 429 in 65c7ae0

result = await ProcessUtil.RunAsync("docker", $"rm {containerId}", throwOnError: false, cancellationToken: timeoutCts.Token);

The restart loop for rerunning docker is here:

tye/src/Microsoft.Tye.Hosting/DockerRunner.cs

Line 382 in 65c7ae0

var logsRes = await ProcessUtil.RunAsync("docker", $"logs -f {containerId}",

. Can you modify this loop instead?

@jkotalik yeah, makes sense. I'll try doing it that way.

@jkotalik changed the DockerRunner to do docker restart when StoppingTokenSource is canceled.

…obe settings

areller · 2020-05-07T01:20:29Z

@davidfowl there are two major things left

generating k8s yamls with liveness and readiness
I don't forward traffic to non ready replicas, unless there is a single replica. proxying with a single replica causes some tests that use external docker images (e.g. redis) to fail, and I need to check why.

Maybe it's best to start reviewing this PR and I can submit the rest later on this PR or another PR?

# Conflicts: # src/Microsoft.Tye.Hosting/DockerRunner.cs

davidfowl · 2020-05-08T06:57:05Z

test/E2ETest/testassets/projects/health-checks/tye-all.yaml

+    replicas: 3
+    bindings:
+      - port: 8004
+    liveness:


These 2 probes are bigger than the entire yaml, that makes me 😢

@davidfowl yeah... I thought it's best to go with something flexible like in k8s, but you can get a way with just providing a path, the rest have default values

# Conflicts: # src/Microsoft.Tye.Core/ApplicationFactory.cs

areller · 2020-05-12T03:55:21Z

I'll probably add the k8s manifest generation tomorrow.

jkotalik · 2020-05-13T16:53:10Z

@areller this change is amazing. Got my coffee ☕ and will be reviewing now.

jkotalik

Great start. I think there is some small cleanup tasks for code readability in the replica state management, but overall looks like the right idea.

src/Microsoft.Tye.Core/ContainerServiceBuilder.cs

src/Microsoft.Tye.Core/ExecutableServiceBuilder.cs

jkotalik · 2020-05-13T17:00:38Z

src/Microsoft.Tye.Core/KubernetesManifestGenerator.cs

+
+                if (builderHttp.Protocol != null)
+                {
+                    httpGet.Add("scheme", builderHttp.Protocol!.ToUpper());


I think you don't need ! here, other similar cases in this file as well.

Suggested change

httpGet.Add("scheme", builderHttp.Protocol!.ToUpper());

httpGet.Add("scheme", builderHttp.Protocol.ToUpper());

yep. I removed it and it didn't throw any warnings. probably because it's inside a block that runs if builderHttp.Protocol != null.
It's weird because at times the compiler will throw a warning when not using the ! operator, even though the expression is in a context where it's guaranteed to not be null...

found an example: if you go to ReplicaMonitor.cs:113 and remove the ! after serviceDesc.Readiness, the compiler will throw a warning, even though there is no way for that variable to be null at that point.

jkotalik · 2020-05-13T17:15:35Z

src/Microsoft.Tye.Core/KubernetesManifestGenerator.cs

+                else
+                {
+                    // If port is not given, we pull default port
+                    var binding = service.Bindings.First(b => builderHttp.Protocol is null || b.Protocol == builderHttp.Protocol);


It does feel a bit awkward to have health checks per service rather than per endpoint, but it seems to be what k8s does. Or maybe we require a port?

requiring a port will indeed make it more compatible with k8s, but it will make the tye.yaml more verbose and less maintainable in my opinion. one thing that k8s does to help with maintainability is to allow to specify a variable as a port. e.g.

port: liveness-port

src/Microsoft.Tye.Core/ProjectServiceBuilder.cs

jkotalik · 2020-05-13T17:34:42Z

src/Microsoft.Tye.Hosting/PortAssigner.cs

@@ -50,7 +50,7 @@ static int GetNextPort()
                        binding.Port = GetNextPort();
                    }

-                    if (service.Description.Replicas == 1)
+                    if ((service.Description.Readiness is null || service.Description.RunInfo is IngressRunInfo) && service.Description.Replicas == 1)


Why add the check for service.Description.RunInfo is IngressRunInfo

because I forgot to remove when added service.Description.Readiness is null :)

jkotalik · 2020-05-13T17:37:47Z

src/Microsoft.Tye.Hosting/ReplicaMonitor.cs

+            private void StartLivenessProbe(Probe probe, ReplicaState moveToOnSuccess = ReplicaState.Healthy)
+            {
+                // currently only HTTP is available
+                if (probe.Http is null)


Suggested change

if (probe.Http is null)

if (probe.Http == null)

And for the rest of the file

jkotalik · 2020-05-13T17:38:04Z

src/Microsoft.Tye.Hosting/ReplicaMonitor.cs

+                // currently only HTTP is available
+                if (probe.Http is null)
+                {
+                    _logger.LogWarning("cannot start probing replica {name} because probe configuration is not set", _replica.Name);


Suggested change

_logger.LogWarning("cannot start probing replica {name} because probe configuration is not set", _replica.Name);

_logger.LogWarning("Cannot start probing replica {name} because probe configuration is not set", _replica.Name);

And for the rest of the file.

jkotalik · 2020-05-13T17:40:26Z

src/Microsoft.Tye.Hosting/ReplicaMonitor.cs

+
+            private void MoveToHealthy(ReplicaState from)
+            {
+                _logger.LogInformation("replica {name} is moving to an healthy state", _replica.Name);


These logs should probably be at debug level.

jkotalik · 2020-05-13T17:42:20Z

src/Microsoft.Tye.Hosting/ReplicaMonitor.cs

+
+            public override void Start()
+            {
+                Func<ReplicaBinding, bool> bindingClosure = (_httpProberSettings.Port.HasValue, _httpProberSettings.Protocol != null) switch


This probably deserves a comment as well.

jkotalik

I think this overall looks good. There will be some follow up I'd like for us to do for polish, but can be done in a separate PR, including:

Sample
Docs
Recipe

src/Microsoft.Tye.Core/CoreStrings.resx

src/Microsoft.Tye.Core/KubernetesManifestGenerator.cs

jkotalik · 2020-05-18T16:59:36Z

src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs

@@ -187,6 +201,165 @@ private static void HandleServiceVolumes(YamlSequenceNode yamlSequenceNode, List
            }
        }

+        private static void HandleServiceProbe(YamlMappingNode yamlMappingNode, ConfigProbe probe)


nit: return ConfigProbe instead of passing it in s.t you can set service.Liveness/service.Readiness directly.

Yeah. That would probably make more sense but it will break compatibility with the rest of the methods. What do you think?

jkotalik · 2020-05-18T17:09:53Z

src/Microsoft.Tye.Core/Serialization/ConfigServiceParser.cs

@@ -187,6 +201,165 @@ private static void HandleServiceVolumes(YamlSequenceNode yamlSequenceNode, List
            }
        }

+        private static void HandleServiceProbe(YamlMappingNode yamlMappingNode, ConfigProbe probe)
+        {
+            foreach (var child in yamlMappingNode.Children)


Can we add some tests for these parsing wise? Much faster for checking these kinds of conditions like making sure initialDelay can't be negative.

I'll try to come up with something. You want it as part of this PR? or break it up in a different one?

I think following the structure here: https://github.com/dotnet/tye/blob/master/test/UnitTests/TyeDeserializationTests.cs#L25.

Mostly just parsing/ validation of tye.yaml to make sure we cover these edge cases correctly.

No preference whether we add them in this PR now or follow up, I'd just like them in general 😄

jkotalik · 2020-05-18T17:12:20Z

src/Microsoft.Tye.Hosting/ReplicaMonitor.cs

+{
+    public class ReplicaMonitor : IApplicationProcessor
+    {
+        private ILogger _logger;


@davidfowl to review this on top of me.

Co-authored-by: Justin Kotalik <jukotali@microsoft.com>

jkotalik · 2020-05-20T17:43:48Z

I'm going to go ahead and merge this, let's do some follow up on top of it 😄 .

Again, thanks @areller for all the work you have done here

areller · 2020-05-20T17:46:43Z

@jkotalik thank you! Will create a PR in the following days

areller added 3 commits April 30, 2020 19:47

each replica has its own cancellation token and records its own state

7e6c2ef

added state to v1replica

8b3e89c

license and formatting

57dc3f4

davidfowl reviewed May 1, 2020

View reviewed changes

areller added 15 commits May 3, 2020 17:28

add liveness and readiness to configuration and model

616df58

ReplicaMonitor

3912db6

revamp test helpers

24b0459

health check tests

d4fbdeb

formatting

e95a361

merge with master

56b9869

formatting

9dcc606

don't run docker tests

3f48f54

add more tests

3b058b3

fix header handling

4659b51

start adding timeout to http probe

ee9b374

add timeout, port, protocol, successThreshold, failureThreshold to pr…

8f396c7

…obe settings

success threshold must be 1 for liveness

0276a09

proxy and ingress don't forward to non ready replicas

569df2a

warnings and formatting

f22149c

areller marked this pull request as ready for review May 7, 2020 03:50

areller requested review from jkotalik and rynowak as code owners May 7, 2020 03:50

areller added 3 commits May 7, 2020 23:02

Merge remote-tracking branch 'dotnet/master' into health

f452feb

# Conflicts: # src/Microsoft.Tye.Hosting/DockerRunner.cs

fix mistake done by merge

aa52cd0

formatting

22e506e

davidfowl reviewed May 8, 2020

View reviewed changes

areller added 3 commits May 10, 2020 17:40

Merge branch 'master' into health

7a72a3e

Merge remote-tracking branch 'dotnet/master' into health

440cf1b

# Conflicts: # src/Microsoft.Tye.Core/ApplicationFactory.cs

run proxy for single replica services if readiness is defined

e86cda1

areller added 6 commits May 12, 2020 00:00

license and formatting

b41ccff

should start at (false, false)

1d3d03e

clean csproj

f89e630

rename service names in health-checks folder

726e2a4

change name of service in tests

7a29291

generate liveness and readiness in k8s manifest

65c7ae0

areller changed the title ~~[WIP] adding health checks~~ adding health checks May 13, 2020

jkotalik reviewed May 13, 2020

View reviewed changes

areller added 3 commits May 13, 2020 18:58

fix review comments

ffef174

using docker restart when docker replica is cancelled

6280485

formatting

a116b4b

jkotalik approved these changes May 18, 2020

View reviewed changes

areller and others added 2 commits May 18, 2020 13:57

Update src/Microsoft.Tye.Core/CoreStrings.resx

eb13a21

Co-authored-by: Justin Kotalik <jukotali@microsoft.com>

Update src/Microsoft.Tye.Core/KubernetesManifestGenerator.cs

602a2bc

Co-authored-by: Justin Kotalik <jukotali@microsoft.com>

jkotalik merged commit f27905b into dotnet:master May 20, 2020

areller deleted the health branch May 23, 2020 03:12

	httpGet.Add("scheme", builderHttp.Protocol!.ToUpper());
	httpGet.Add("scheme", builderHttp.Protocol.ToUpper());

	_logger.LogWarning("cannot start probing replica {name} because probe configuration is not set", _replica.Name);
	_logger.LogWarning("Cannot start probing replica {name} because probe configuration is not set", _replica.Name);

adding health checks #432

adding health checks #432

Conversation

areller commented Apr 30, 2020 • edited Loading

Changes

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

areller May 1, 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

areller commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areller commented May 12, 2020

jkotalik commented May 13, 2020

jkotalik left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik left a 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

jkotalik commented May 20, 2020

areller commented May 20, 2020

areller commented Apr 30, 2020 •

edited

Loading

areller May 1, 2020 •

edited

Loading