CSHARP-4718: Enable Container and kubernetes awareness #1295

adelinowona · 2024-03-18T18:58:32Z

Enables container and kubernetes awareness in the driver as specified in the MongoDB Handshake Spec .

What is the motivation for this change?

Allows Product to track user usage of docker and kubernetes.

adelinowona · 2024-03-18T19:15:39Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs

@@ -176,6 +179,23 @@ name switch
                    _ => null,
                };

+            BsonDocument GetContainerDocument()
+            {
+                var isExecutionContainerDocker = File.Exists("/.dockerenv");


I hesitated a little about checking for existence of a file to detect if we are in a docker container. Doing this makes us a dependent on the file system which might have potential issues related to file permissions or availability. I did some digging and found there is a environment variable called DOTNET_RUNNING_IN_CONTAINER which is set automatically by official Microsoft .Net images More info . Problem is using this environment variable relies on the .net runtime to set it which is guaranteed if users use the official .Net images for their docker containers. However, it doesn't cover cases where users don't use the official .net images or choose to manually install a .net runtime in their containers. So I think checking for the file existence is our best bet unless we want to use both strategies and let them compensate for each other?

adelinowona · 2024-03-18T19:19:55Z

tests/MongoDB.Driver.Core.Tests/Core/Connections/ClientDocumentHelperTests.cs

@@ -132,6 +132,17 @@ public void CreateDriverDocument_should_return_expected_result()
            result.Should().Be($"{{ name : 'mongo-csharp-driver', version : '{driverVersion}' }}");
        }

+        [Fact]
+        public void CreateEnvDocument_should_return_expected_result()


Using file existence as check for docker container presence means we can't really unit test it because we can't easily mock FIle.Exist as it is a static class. There is a way to bypass that but it will mean complicating the code for checking docker container presence just for the sake of testing. However, I manually tested by running a mongodb .net application in a docker container with the changes to test and the docker detection logic works.

See mine comment above.

sanych-sun · 2024-03-18T20:17:15Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs

+            BsonDocument GetContainerDocument()
+            {
+                var isExecutionContainerDocker = File.Exists("/.dockerenv");
+                var isOrchestratorKubernetes = Environment.GetEnvironmentVariable("KUBERNETES_SERVICE_HOST") != null;


It might makes sense to use EnvironmentVariableProvider here. It might be substitutable and it will help us when we will approach task to run tests in parallel.

We probably ought to implement something similar for file system. With the same motivation: having such substitutable components could help us to isolate each test without necessarily to have file on the host machine or setting environment variables, as each such host adjustment could affect other tests.

sanych-sun · 2024-03-18T20:36:00Z

tests/MongoDB.Driver.Core.Tests/Core/Connections/ClientDocumentHelperTests.cs

@@ -132,6 +132,17 @@ public void CreateDriverDocument_should_return_expected_result()
            result.Should().Be($"{{ name : 'mongo-csharp-driver', version : '{driverVersion}' }}");
        }

+        [Fact]
+        public void CreateEnvDocument_should_return_expected_result()


See mine comment above.

sanych-sun · 2024-03-20T16:31:57Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs

@@ -176,6 +179,23 @@ name switch
                    _ => null,
                };

+            BsonDocument GetContainerDocument()
+            {
+                var isExecutionContainerDocker = File.Exists("/.dockerenv");


Is Windows file system OK with such check? Should we wrap this with try-catch (some permissions restriction could lead to exception I suppose)?

BTW, this check looks like a Linux container detection. What about Windows containers? Should we try to detect them as well?

JamesKovacs

Looking good. I'm OK with the current impl or switching to our own mocks for IFileSystem and IEnvironment (or whatever we want to call it. We do need the two additional test cases noted.

As for Windows container detection, if we find a way to reliably detect Docker on Windows, then we need to create a new DRIVERS ticket and update the spec so all drivers perform the detection in the same way.

JamesKovacs · 2024-03-21T17:12:02Z

tests/MongoDB.Driver.Core.Tests/Core/Connections/ClientDocumentHelperTests.cs

+                result.Should().Be($"{{ name : 'vercel', container : {{ orchestrator : 'kubernetes' }} }}");
+            }
+        }
+


We should also verify:

client.env.container.runtime is set to docker when /.dockerenv is present.

client.env.container is not present if neither Docker nor Kubernetes is deteced.

Done. Also I think I found a reliable way to detect Docker for Windows. I will reach out the owners of the DRIVERS ticket for this work to discuss further if spec needs to be updated or if we are ok with drivers not detecting Windows containers given their low usage.

JamesKovacs

LGTM

sanych-sun

Looks good, minor changes requested.

sanych-sun · 2024-03-28T20:44:53Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs


        private static void Initialize()
        {
            __driverDocument = new Lazy<BsonDocument>(CreateDriverDocument);
            __envDocument = new Lazy<BsonDocument>(CreateEnvDocument);
            __osDocument = new Lazy<BsonDocument>(CreateOSDocument);
            __platformString = new Lazy<string>(GetPlatformString);
+            __environmentVariableProvider = new Lazy<IEnvironmentVariableProvider>(() => new EnvironmentVariableProvider());


Why it's Lazy? It could point to EnvironmentVariableProvider.Instance by default.

sanych-sun · 2024-03-28T20:46:03Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs


        private static void Initialize()
        {
            __driverDocument = new Lazy<BsonDocument>(CreateDriverDocument);
            __envDocument = new Lazy<BsonDocument>(CreateEnvDocument);
            __osDocument = new Lazy<BsonDocument>(CreateOSDocument);
            __platformString = new Lazy<string>(GetPlatformString);
+            __environmentVariableProvider = new Lazy<IEnvironmentVariableProvider>(() => new EnvironmentVariableProvider());
+            __fileSystemProvider = new Lazy<IFileSystemProvider>(() => new FileSystemProvider());


Same here: I suppose we do not need Lazy, just use Instance by default.

sanych-sun

LGTM

BorisDog

LGTM, some minor comments for your consideration.
File and environment vars mocking approach is a great addition.

BorisDog · 2024-03-28T22:02:45Z

src/MongoDB.Driver.Core/Core/Misc/FileWrapper.cs

+        bool Exists(string name);
+    }
+
+    internal class FileWrapper : IFile


minor: sealed?

BorisDog · 2024-03-28T22:06:08Z

src/MongoDB.Driver.Core/Core/Misc/FileSystemProvider.cs

+    internal class FileSystemProvider : IFileSystemProvider
+    {
+        private static readonly IFileSystemProvider __instance = new FileSystemProvider();
+        public static IFileSystemProvider Instance => __instance;


minor: maybe read-only property is sufficient ?
public static IFileSystemProvider Instance { get; } = new FileSystemProvider();

BorisDog · 2024-03-28T22:10:15Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs

@@ -30,17 +31,31 @@ internal class ClientDocumentHelper
        private static Lazy<BsonDocument> __envDocument;
        private static Lazy<BsonDocument> __osDocument;
        private static Lazy<string> __platformString;
+        private static IEnvironmentVariableProvider __environmentVariableProvider;


Do we want to use __environmentVariableProvider for all other variables in this file?
Follow up ticket for that? This is also pre-requisite for future tests parallelization.

Yes a follow up is to be made.

BorisDog · 2024-03-28T22:11:45Z

tests/MongoDB.Driver.Core.Tests/Core/Connections/ClientDocumentHelperTests.cs

+
+            ClientDocumentHelper.SetEnvironmentVariableProvider(environmentVariableProviderMock.Object);
+            ClientDocumentHelper.SetFileSystemProvider(fileSystemProviderMock.Object);
+            using (new DisposableEnvironmentVariable("VERCEL"))


Ideally we'd just use environmentVariableProviderMock for "VERCEL".

Yeah this is connected to replacing uses of Environment with EnvironmentVariableProvider in ClientDocumentHelper.cs

BorisDog · 2024-03-28T22:15:09Z

src/MongoDB.Driver.Core/Core/Connections/ClientDocumentHelper.cs

+
+                if (isExecutionContainerDocker || isOrchestratorKubernetes)
+                {
+                    return new BsonDocument


minor: styling shortcut for your consideration

return new() { .... }

CSHARP-4718: Enable Container and kubernetes awareness

2761868

adelinowona commented Mar 18, 2024

View reviewed changes

adelinowona marked this pull request as ready for review March 18, 2024 19:20

adelinowona requested a review from a team as a code owner March 18, 2024 19:20

adelinowona requested review from JamesKovacs and BorisDog and removed request for a team March 18, 2024 19:20

sanych-sun reviewed Mar 18, 2024

View reviewed changes

sanych-sun reviewed Mar 20, 2024

View reviewed changes

JamesKovacs requested changes Mar 21, 2024

View reviewed changes

adelinowona added 3 commits March 25, 2024 11:21

add support for mocking container awareness tests

9b8f20c

use lazy loading

ef3d58c

fix naming

2e2a32f

adelinowona requested review from sanych-sun and JamesKovacs March 25, 2024 16:12

JamesKovacs approved these changes Mar 26, 2024

View reviewed changes

sanych-sun requested changes Mar 28, 2024

View reviewed changes

remove lazy pattern usage

fd641bc

adelinowona requested a review from sanych-sun March 28, 2024 21:11

sanych-sun approved these changes Mar 28, 2024

View reviewed changes

BorisDog approved these changes Mar 28, 2024

View reviewed changes

Incorporate review comments

db86804

adelinowona merged commit 5f7fc33 into mongodb:master Apr 2, 2024
1 check was pending

adelinowona deleted the csharp4718 branch April 2, 2024 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-4718: Enable Container and kubernetes awareness #1295

CSHARP-4718: Enable Container and kubernetes awareness #1295

adelinowona commented Mar 18, 2024

adelinowona Mar 18, 2024

adelinowona Mar 18, 2024

sanych-sun Mar 18, 2024

sanych-sun Mar 18, 2024

sanych-sun Mar 18, 2024 •

edited

Loading

sanych-sun Mar 18, 2024

sanych-sun Mar 20, 2024

JamesKovacs left a comment

JamesKovacs Mar 21, 2024

adelinowona Mar 25, 2024

JamesKovacs left a comment

sanych-sun left a comment

sanych-sun Mar 28, 2024

sanych-sun Mar 28, 2024

sanych-sun left a comment

BorisDog left a comment

BorisDog Mar 28, 2024

BorisDog Mar 28, 2024

BorisDog Mar 28, 2024

adelinowona Apr 2, 2024

BorisDog Mar 28, 2024

adelinowona Apr 2, 2024

BorisDog Mar 28, 2024

CSHARP-4718: Enable Container and kubernetes awareness #1295

CSHARP-4718: Enable Container and kubernetes awareness #1295

Conversation

adelinowona commented Mar 18, 2024

What is the motivation for this change?

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

sanych-sun Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKovacs left a comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

BorisDog 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

sanych-sun Mar 18, 2024 •

edited

Loading