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.

Sign up for GitHub

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

Jump to bottom

feat(slimfaas): call only at least when one pod is read #19

Merged

guillaume-chervet merged 31 commits into main from feature/call-only-when-pod-ready

Jul 3, 2023

Collaborator

guillaume-chervet commented Jun 26, 2023 •

edited

Loading

Now we wait at least one pod is in started state before doing the first HTTP try.


          feat(slimfaas): call only at least when one pod is read

d99bb7d

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 14:23

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 14:23

— with

GitHub Actions Inactive

fix

003fde4

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 14:48

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 14:48

— with

GitHub Actions Inactive


          test

0d373c0

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 15:24

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 26, 2023 15:24

— with

GitHub Actions Inactive

sss

749f938

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 10:05

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 10:05

— with

GitHub Actions Inactive


          update

68eeb8e

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 12:00

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 12:00

— with

GitHub Actions Inactive


          test

db2777d

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 13:58

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 13:58

— with

GitHub Actions Inactive


          ssss

1346cda

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 14:04

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 14:04

— with

GitHub Actions Inactive


          ssss

f488d08

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 14:39

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 14:39

— with

GitHub Actions Inactive


          test

07a4a32

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 15:11

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 15:11

— with

GitHub Actions Inactive

mmm

f6bc411

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 15:17

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 27, 2023 15:17

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 28, 2023 13:20

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 28, 2023 13:20

— with

GitHub Actions Inactive

Contributor

g7ed6e commented Jun 28, 2023

please squash or rewrite the git history when time to merge comes.

g7ed6e reviewed

View reviewed changes

src/SlimFaas/HistorySynchronizationWorker.cs Outdated

@@ @@ -18,7 +18,7 @@ public class HistorySynchronizationWorker: BackgroundService @@
                       _historyHttpMemoryService = historyHttpMemoryService;
                       _historyHttpRedisService = historyHttpRedisService;
                       _logger = logger;
-                      _delay = delay;
+                      _delay = int.Parse(Environment.GetEnvironmentVariable("HISTORY_SYNCHRONISATION_WORKER_DELAY_MILLISECONDS")  ?? delay.ToString());

Contributor

g7ed6e Jun 28, 2023

isn't int.Parse throwing with null or empty values ? i would sugget using TryParse here

src/SlimFaas/Kubernetes/KubernetesService.cs Outdated

    
            @@ -56,13 +65,13 @@ public KubernetesService(IConfiguration config, ILogger<KubernetesService> logge
          
                      try

                      {

                          using var client = new Kubernetes(_k8SConfig);

                          var patchString = "{\"spec\": {\"replicas\": " + request.Replicas + "}}";

                          var patchString = $"{{\"spec\": {{\"replicas\": {request?.Replicas}}}}}";

Contributor

g7ed6e Jun 28, 2023

if request is null it will output null in the json string

src/SlimFaas/Kubernetes/KubernetesService.cs Outdated

                           var patch = new V1Patch(patchString, V1Patch.PatchType.MergePatch);
-                          await client.PatchNamespacedDeploymentScaleAsync(patch, request.Deployment, request.Namespace);
+                          await client.PatchNamespacedDeploymentScaleAsync(patch, request?.Deployment, request?.Namespace);

Contributor

g7ed6e Jun 28, 2023

Should client.PatchNamespacedDeploymentScaleAsync be called when request is null ?

src/SlimFaas/Kubernetes/KubernetesService.cs Outdated Show resolved Hide resolved

src/SlimFaas/Kubernetes/KubernetesService.cs Outdated

                       }
                   }
+                  private static IList<PodInformation> MapPodInformations(V1PodList list)

Contributor

g7ed6e Jun 28, 2023

here we could return IEnumerable and yield return filetred results. The caller would call .ToList to get the List already filtered

src/SlimFaas/Kubernetes/KubernetesService.cs Outdated

@@ @@ -27,6 +28,7 @@ public record DeploymentsInformations @@
               public record DeploymentInformation
               {
                   public string Deployment { get; set; }
+                  public IList<PodInformation>? Pods { get; set; }

Contributor

g7ed6e Jun 28, 2023

I think we should enable add <WarningsAsErrors>Nullable</WarningsAsErrors> in csproj files to enforce correct usage/handling of NRT


          add NonNullable as Error

cea63fd

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 08:41

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 08:41

— with

GitHub Actions Inactive


          feedback from PR

0d2d889

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 09:09

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 09:09

— with

GitHub Actions Inactive


          feedback from sonar

685a228

sonarcloud bot commented Jun 30, 2023

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.0% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 09:17

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 09:17

— with

GitHub Actions Inactive

Collaborator Author

guillaume-chervet commented Jun 30, 2023

Pour le merge je ferais un squash merge

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 12:06

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 12:06

— with

GitHub Actions Inactive


          add case from statrup

e3007ee

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 15:01

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

June 30, 2023 15:01

— with

GitHub Actions Inactive

g7ed6e reviewed

View reviewed changes

src/SlimFaas/EnvironmentVariables.cs Outdated

+              public static class EnvironmentVariables
+              {
+                  public static int ReadInteger<T>(ILogger<T> logger, string environmentVariableName, int defaultDelay)

Contributor

g7ed6e Jun 30, 2023

defaultDelay => defaultValue

src/SlimFaas/EnvironmentVariables.cs Outdated

+              public static class EnvironmentVariables
+              {
+                  public static int ReadInteger<T>(ILogger<T> logger, string environmentVariableName, int defaultDelay)

Contributor

g7ed6e Jun 30, 2023

It looks strange to pass ILogger here. I would suggest something like
TryReadInteger(string environmentVariable, out int value).
The caller would interpret a false result to apply the correct default value and use its own ILogger instance.

src/SlimFaas/ReplicasService.cs Outdated

                       foreach (var function in Deployments.Functions)
                       {
-                          var updatedFunction = tasks.FirstOrDefault(t => t.Result.Deployment == function.Deployment);
-                          updatedFunctions.Add(function with { Replicas = updatedFunction != null ? updatedFunction.Result.Replicas : function.Replicas });
+                          var updatedFunction = tasks.Find(t => t.Result?.Deployment == function.Deployment);

Contributor

g7ed6e Jun 30, 2023

In this foreach loop .Result Task property is accessed. The may cause deadlocks. We should have the following code before entering this loop.

ReplicaRequest?[] replicaResuests = await Task.WhenAll(tasks);

and loop through this array.

tests/SlimFaas.Tests/HistorySynchronizationWorkerShould.cs

@@ @@ -12,19 +13,17 @@ public async Task SyncLastTicksBetweenDatabaseAndMemory() @@
                       var redisMockService = new RedisMockService();
                       var historyHttpRedisService = new HistoryHttpRedisService(redisMockService);
                       var kubernetesService = new Mock<IKubernetesService>();
-                      var deploymentsInformations = new DeploymentsInformations()
+                      var deploymentsInformations = new DeploymentsInformations(Functions: new List<DeploymentInformation>()

Contributor

g7ed6e Jun 30, 2023

it looks weird to pass an empty list here, i think DeploymentsInformations should:

be renamed DeploymentInformation
initialize its own porperties internally

tests/SlimFaas.Tests/HistorySynchronizationWorkerShould.cs

		@@ -39,10 +38,10 @@ public async Task SyncLastTicksBetweenDatabaseAndMemory()
		await Task.Delay(200);

Contributor

g7ed6e Jun 30, 2023

there's a lot of magic values that should either be centralized somewhere (a static class with const) or moved as const of the type they belong to.

tests/SlimFaas.Tests/ReplicasSynchronizationWorkerShould.cs

@@ @@ -1,22 +1,24 @@ @@
               using System.Collections;
               using Microsoft.Extensions.Logging;
               using Moq;
+              using SlimFaas.Kubernetes;
               namespace SlimFaas.Tests;
               public class DeploymentsTestData:IEnumerable<object[]>

Contributor

g7ed6e Jun 30, 2023

another option to get strongly types is to inherit from TheoryData. Then in the constructor, just call Add() to add tests iterations. This allow to remove the noise of object[].


          refactor(slimfaas): centralize env variables const

7a93b97

guillaume-chervet mentioned this pull request

Finish feeback from PullRequest https://github.com/AxaFrance/SlimFaas/pull/19 #22

Open

guillaume-chervet temporarily deployed to MLOpsPython

July 3, 2023 10:19

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

July 3, 2023 10:19

— with

GitHub Actions Inactive

guillaume-chervet added 2 commits

July 3, 2023 15:44


          update README.md

cf5c5fc


          Merge branch 'main' into feature/call-only-when-pod-ready

753917a

guillaume-chervet merged commit 5c5a1f3 into main

1 check passed

guillaume-chervet temporarily deployed to MLOpsPython

July 3, 2023 13:51

— with

GitHub Actions Inactive

guillaume-chervet temporarily deployed to MLOpsPython

July 3, 2023 13:51

— with

GitHub Actions Inactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment