Skip to content
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.

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

Added CGroupv2 support into Docker Extensions #839

Merged
merged 18 commits into from
Jan 9, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
15b1b51
Initial changes
anoopbabu29 Dec 6, 2022
4cb146d
Adding cgroupv2 to test and updating the approach such that it checks…
anoopbabu29 Dec 15, 2022
cb8f573
Merge branch 'main' of https://github.com/anoopbabu29/opentelemetry-d…
anoopbabu29 Dec 16, 2022
7ff1941
Merge branch 'open-telemetry:main' into cgroupv2-sup
anoopbabu29 Dec 16, 2022
aad5630
Merge branch 'cgroupv2-sup' of https://github.com/anoopbabu29/opentel…
anoopbabu29 Dec 19, 2022
94be6e9
Respoding to Comments and build checks
anoopbabu29 Dec 19, 2022
856fed7
Merge branch 'main' into cgroupv2-sup
anoopbabu29 Dec 19, 2022
fd0c8e4
Adjusted build such that Detect checks both versions, and BuildResour…
anoopbabu29 Dec 22, 2022
5ba7994
Merge branch 'cgroupv2-sup' of https://github.com/anoopbabu29/opentel…
anoopbabu29 Dec 22, 2022
78a5a62
Merge branch 'main' into cgroupv2-sup
anoopbabu29 Dec 22, 2022
14ce9b1
Adjusting BuildResource at suggestion
anoopbabu29 Dec 23, 2022
492552d
Merge branch 'cgroupv2-sup' of https://github.com/anoopbabu29/opentel…
anoopbabu29 Dec 23, 2022
a3647b3
Added same tests as the go version as well as their regex expression
anoopbabu29 Jan 4, 2023
9e05aba
Merge branch 'main' into cgroupv2-sup
anoopbabu29 Jan 4, 2023
b5857f0
Addressing Comments by adding Enums and cleaning up tests
anoopbabu29 Jan 5, 2023
7b7e580
Merge branch 'main' into cgroupv2-sup
anoopbabu29 Jan 6, 2023
ba6b6ab
Fix parse mode version in test
anoopbabu29 Jan 6, 2023
d043c33
Merge branch 'main' into cgroupv2-sup
utpilla Jan 9, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Text.RegularExpressions;
using OpenTelemetry.Extensions.Docker.Utils;
using OpenTelemetry.Resources;

Expand All @@ -28,24 +29,51 @@ namespace OpenTelemetry.Extensions.Docker.Resources;
public class DockerResourceDetector : IResourceDetector
{
private const string FILEPATH = "/proc/self/cgroup";
private const string FILEPATHV2 = "/proc/self/mountinfo";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any usage of FILEPATHV2 in detector. I'd guess, that ExtractContainerIdV2 should use it, instead of FILEPATH. Proper fix for it will also solve problem mentioned by @alexeypukhov , as that functions should check different file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change has been made to check both versions in the Detect method and assuming the version being used in the BuildResource method. This way, the problem mentioned by @alexeypukhov would be solved as you mentioned since no container id would be able to be found when checking for cgroupv1 if cgroupv2 is being used.

private const string HOSTNAME = "hostname";

/// <summary>
/// Detects the resource attributes from Docker.
/// </summary>
/// <returns>Resource with key-value pairs of resource attributes.</returns>
public Resource Detect()
{
return this.BuildResource(FILEPATH);
var cGroupBuild = this.BuildResourceV1(FILEPATH);
if (cGroupBuild == Resource.Empty)
{
cGroupBuild = this.BuildResourceV2(FILEPATHV2);
}

return cGroupBuild;
}

/// <summary>
/// Builds the resource attributes from Container Id in file path of cgroupv1.
/// </summary>
/// <param name="path">File path where container id exists.</param>
/// <returns>Returns Resource with list of key-value pairs of container resource attributes if container id exists else empty resource.</returns>
internal Resource BuildResourceV1(string path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Source for BuildResourceV1 / BuildResourceV2 are very similar, same as ExtractContainerIdV1 / ExtractContainerIdV2.
To have less duplicated code, I'd probably create enum ParseMode {V1, V2} and pass it as second argument to BuildResource / ExtractContainerId with different logic called when you fetch string containerId = ... in ExtractContainerId.

{
var containerId = this.ExtractContainerIdV1(path);

if (string.IsNullOrEmpty(containerId))
{
return Resource.Empty;
}
else
{
return new Resource(new List<KeyValuePair<string, object>>() { new KeyValuePair<string, object>(DockerSemanticConventions.AttributeContainerID, containerId), });
}
}

/// <summary>
/// Builds the resource attributes from Container Id in file path.
/// Builds the resource attributes from Container Id in file path of cgroupv2.
/// </summary>
/// <param name="path">File path where container id exists.</param>
/// <returns>Returns Resource with list of key-value pairs of container resource attributes if container id exists else empty resource.</returns>
internal Resource BuildResource(string path)
internal Resource BuildResourceV2(string path)
{
var containerId = this.ExtractContainerId(path);
var containerId = this.ExtractContainerIdV2(path);

if (string.IsNullOrEmpty(containerId))
{
Expand All @@ -58,11 +86,11 @@ internal Resource BuildResource(string path)
}

/// <summary>
/// Extracts Container Id from path.
/// Extracts Container Id from path using the cgroupv1 format.
/// </summary>
/// <param name="path">cgroup path.</param>
/// <returns>Container Id, Null if not found or exception being thrown.</returns>
private string ExtractContainerId(string path)
private string ExtractContainerIdV1(string path)
{
try
{
Expand All @@ -88,6 +116,37 @@ private string ExtractContainerId(string path)
return null;
}

/// <summary>
/// Extracts Container Id from path using the cgroupv2 format.
/// </summary>
/// <param name="path">File path where container id exists.</param>
/// <returns>Returns Resource with list of key-value pairs of container resource attributes if container id exists else empty resource.</returns>
private string ExtractContainerIdV2(string path)
{
try
{
if (!File.Exists(path))
{
return null;
}

foreach (string line in File.ReadLines(path))
{
string containerId = (!string.IsNullOrEmpty(line) && line.Contains(HOSTNAME)) ? this.GetIdFromLineV2(line) : null;
if (!string.IsNullOrEmpty(containerId))
{
return containerId;
}
}
}
catch (Exception ex)
{
DockerExtensionsEventSource.Log.ExtractResourceAttributesException($"{nameof(DockerResourceDetector)} : Failed to extract Container id from path", ex);
}

return null;
}

/// <summary>
/// Gets the Container Id from the line after removing the prefix and suffix.
/// </summary>
Expand Down Expand Up @@ -116,6 +175,23 @@ private string GetIdFromLine(string line)
return containerId;
}

private string GetIdFromLineV2(string line)
{
string containerId = null;
var match = Regex.Match(line, @".*/.+/([\w+-.]{64})/.*$");
if (match.Success)
{
containerId = match.Groups[1].Value;
}

if (string.IsNullOrEmpty(containerId) || !EncodingUtils.IsValidHexString(containerId))
{
return null;
}

return containerId;
}

private string RemovePrefixAndSuffixIfneeded(string input, int startIndex, int endIndex)
{
startIndex = (startIndex == -1) ? 0 : startIndex + 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,50 +57,149 @@ public class DockerResourceDetectorTests
private const string CONTAINERID =
"d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356";

// cgroupv2 line with container Id
private const string CGROUPLINEV2 =
Copy link

@alexeypukhov alexeypukhov Dec 23, 2022

Choose a reason for hiding this comment

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

do you also need to check suffix or prefix for cgroup v2?
Also just a thought. Should we check prefix and suffix end to end, I mean with reading from file? If GetContainerId works with extracted cgroup string no matter V1 or V2, maybe we should have a separate test just for this function and do not mix it with reading the line from the file and parsing it depending on V1 or V2

Copy link
Contributor Author

@anoopbabu29 anoopbabu29 Dec 23, 2022

Choose a reason for hiding this comment

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

I can check for them as they are separate implementations. I figured that most of the functionality was similar enough to warrant not having them but I believe you are correct that they should exist, although the suffix isn't a necessary test in my opinion since its not a real case I believe.

"13:name=systemd:/pod/d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356/hostname";

// cgroupv2 Expected Container Id
private const string CONTAINERIDV2 = "d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356";

// hostname
private const string CGROUPLINEV2WITHHOSTNAME =
"473 456 254:1 /docker/containers/dc64b5743252dbaef6e30521c34d6bbd1620c8ce65bdb7bf9e7143b61bb5b183/hostname /etc/hostname rw,relatime - ext4 /dev/vda1 rw";

private const string CONTAINERIDV2WITHHOSTNAME = "dc64b5743252dbaef6e30521c34d6bbd1620c8ce65bdb7bf9e7143b61bb5b183";

// minikube containerd mountinfo
private const string CGROUPLINEV2WITHMINIKUBECONTAINERD =
"1537 1517 8:1 /var/lib/containerd/io.containerd.grpc.v1.cri/sandboxes/fb5916a02feca96bdeecd8e062df9e5e51d6617c8214b5e1f3ff9320f4402ae6/hostname /etc/hostname rw,relatime - ext4 /dev/sda1 rw";

private const string CONTAINERIDV2WITHMINIKUBECONTAINERD = "fb5916a02feca96bdeecd8e062df9e5e51d6617c8214b5e1f3ff9320f4402ae6";

// minikube docker mountinfo
private const string CGROUPLINEV2WITHDOCKER =
"2327 2307 8:1 /var/lib/docker/containers/a1551a1d7e1881d6c18d2c9ec462cab6ad3666825f0adb2098e9d5b198fd7e19/hostname /etc/hostname rw,relatime - ext4 /dev/sda1 rw";

private const string CONTAINERIDV2WITHDOCKER = "a1551a1d7e1881d6c18d2c9ec462cab6ad3666825f0adb2098e9d5b198fd7e19";

// minikube docker mountinfo2
private const string CGROUPLINEV2WITHDOCKER2 =
"929 920 254:1 /docker/volumes/minikube/_data/lib/docker/containers/0eaa6718003210b6520f7e82d14b4c8d4743057a958a503626240f8d1900bc33/hostname /etc/hostname rw,relatime - ext4 /dev/vda1 rw";

private const string CONTAINERIDV2WITHDOCKER2 = "0eaa6718003210b6520f7e82d14b4c8d4743057a958a503626240f8d1900bc33";

// podman mountinfo
private const string CGROUPLINEV2WITHPODMAN =
"1096 1088 0:104 /containers/overlay-containers/1a2de27e7157106568f7e081e42a8c14858c02bd9df30d6e352b298178b46809/userdata/hostname /etc/hostname rw,nosuid,nodev,relatime - tmpfs tmpfs rw,size=813800k,nr_inodes=203450,mode=700,uid=1000,gid=1000";

private const string CONTAINERIDV2WITHPODMAN = "1a2de27e7157106568f7e081e42a8c14858c02bd9df30d6e352b298178b46809";

// Invalid cgroup line (contains a z)"
private const string INVALIDHEXCGROUPLINEV2 =
Copy link
Contributor

Choose a reason for hiding this comment

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

We have so many const defined in that file now. I like @pellared suggestion to look into structure of tests in open-telemetry/opentelemetry-go#3508 - they looks nice with a dictionary of named cased with input / expected result pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also turn it into a theory with InlineData if that works better in this context

"13:name=systemd:/var/lib/containerd/io.containerd.grpc.v1.cri/sandboxes/fb5916a02feca96bdeecd8e062df9e5e51d6617c8214b5e1f3fz9320f4402ae6/hostname";

[Fact]
public void TestValidContainer()
public void TestValidContainerV1()
{
var dockerResourceDetector = new DockerResourceDetector();

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEWITHPREFIX);
Assert.Equal(CONTAINERIDWITHPREFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResource(tempFile.FilePath)));
Assert.Equal(CONTAINERIDWITHPREFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResourceV1(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEWITHSUFFIX);
Assert.Equal(CONTAINERIDWITHSUFFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResource(tempFile.FilePath)));
Assert.Equal(CONTAINERIDWITHSUFFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResourceV1(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEWITHPREFIXandSUFFIX);
Assert.Equal(CONTAINERIDWITHPREFIXANDSUFFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResource(tempFile.FilePath)));
Assert.Equal(CONTAINERIDWITHPREFIXANDSUFFIXREMOVED, this.GetContainerId(dockerResourceDetector.BuildResourceV1(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINE);
Assert.Equal(CONTAINERID, this.GetContainerId(dockerResourceDetector.BuildResource(tempFile.FilePath)));
Assert.Equal(CONTAINERID, this.GetContainerId(dockerResourceDetector.BuildResourceV1(tempFile.FilePath)));
}
}

[Fact]
public void TestValidContainerV2()
{
var dockerResourceDetector = new DockerResourceDetector();

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2);
Assert.Equal(CONTAINERIDV2, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2WITHHOSTNAME);
Assert.Equal(CONTAINERIDV2WITHHOSTNAME, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2WITHMINIKUBECONTAINERD);
Assert.Equal(CONTAINERIDV2WITHMINIKUBECONTAINERD, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2WITHDOCKER);
Assert.Equal(CONTAINERIDV2WITHDOCKER, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2WITHDOCKER2);
Assert.Equal(CONTAINERIDV2WITHDOCKER2, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}

using (TempFile tempFile = new TempFile())
{
tempFile.Write(CGROUPLINEV2WITHPODMAN);
Assert.Equal(CONTAINERIDV2WITHPODMAN, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath)));
}
}

[Fact]
public void TestInvalidContainer()
public void TestInvalidContainerV1()
{
var dockerResourceDetector = new DockerResourceDetector();

// test invalid containerId (non-hex)
using (TempFile tempFile = new TempFile())
{
tempFile.Write(INVALIDCGROUPLINE);
Assert.Equal(dockerResourceDetector.BuildResource(tempFile.FilePath), Resource.Empty);
Assert.Equal(dockerResourceDetector.BuildResourceV1(tempFile.FilePath), Resource.Empty);
}

// test invalid file
Assert.Equal(dockerResourceDetector.BuildResourceV1(Path.GetTempPath()), Resource.Empty);
}

[Fact]
public void TestInvalidContainerV2()
{
var dockerResourceDetector = new DockerResourceDetector();

// test invalid containerId (non-hex)
using (TempFile tempFile = new TempFile())
{
tempFile.Write(INVALIDHEXCGROUPLINEV2);
Assert.Equal(dockerResourceDetector.BuildResourceV1(tempFile.FilePath), Resource.Empty);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it call BuildResourceV2, same as on line 202?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or probably both. In that case we may need to add this tests to V1 also - as incorrect V2 lines should be still incorrect for V1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was my mistake, I'll make that change. And yes, I'll make a test checking that V1 fails for V2 and vice versa.

}

// test invalid file
Assert.Equal(dockerResourceDetector.BuildResource(Path.GetTempPath()), Resource.Empty);
Assert.Equal(dockerResourceDetector.BuildResourceV1(Path.GetTempPath()), Resource.Empty);
}

private string GetContainerId(Resource resource)
Expand Down