-
Notifications
You must be signed in to change notification settings - Fork 272
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
Changes from 12 commits
15b1b51
4cb146d
cb8f573
7ff1941
aad5630
94be6e9
856fed7
fd0c8e4
5ba7994
78a5a62
14ce9b1
492552d
a3647b3
9e05aba
b5857f0
7b7e580
ba6b6ab
d043c33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,24 +28,32 @@ namespace OpenTelemetry.Extensions.Docker.Resources; | |
public class DockerResourceDetector : IResourceDetector | ||
{ | ||
private const string FILEPATH = "/proc/self/cgroup"; | ||
private const string FILEPATHV2 = "/proc/self/mountinfo"; | ||
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. | ||
/// 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 BuildResource(string path) | ||
internal Resource BuildResourceV1(string path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Source for |
||
{ | ||
var containerId = this.ExtractContainerId(path); | ||
var containerId = this.ExtractContainerIdV1(path); | ||
|
||
if (string.IsNullOrEmpty(containerId)) | ||
{ | ||
|
@@ -58,11 +66,30 @@ internal Resource BuildResource(string path) | |
} | ||
|
||
/// <summary> | ||
/// Extracts Container Id from 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 BuildResourceV2(string path) | ||
{ | ||
var containerId = this.ExtractContainerIdV2(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> | ||
/// 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 | ||
{ | ||
|
@@ -88,6 +115,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> | ||
|
@@ -116,6 +174,27 @@ private string GetIdFromLine(string line) | |
return containerId; | ||
} | ||
|
||
private string GetIdFromLineV2(string line) | ||
{ | ||
int hostnameIndex = line.LastIndexOf("/" + HOSTNAME); | ||
if (hostnameIndex < 0) | ||
{ | ||
return null; | ||
} | ||
|
||
string earlierSection = line.Substring(0, hostnameIndex); | ||
int startIndex = earlierSection.LastIndexOf('/'); | ||
|
||
string containerId = this.RemovePrefixAndSuffixIfneeded(earlierSection, startIndex, hostnameIndex); | ||
|
||
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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,13 @@ public class DockerResourceDetectorTests | |
private const string CONTAINERID = | ||
"d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356"; | ||
|
||
// cgroupv2 line with container Id | ||
private const string CGROUPLINEV2 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you also need to check suffix or prefix for cgroup v2? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"; | ||
|
||
// Expected Container Id | ||
private const string CONTAINERIDV2 = "d86d75589bf6cc254f3e2cc29debdf85dde404998aa128997a819ff991827356"; | ||
|
||
[Fact] | ||
public void TestValidContainer() | ||
{ | ||
|
@@ -65,25 +72,31 @@ public void TestValidContainer() | |
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))); | ||
} | ||
|
||
using (TempFile tempFile = new TempFile()) | ||
{ | ||
tempFile.Write(CGROUPLINEV2); | ||
Assert.Equal(CONTAINERIDV2, this.GetContainerId(dockerResourceDetector.BuildResourceV2(tempFile.FilePath))); | ||
} | ||
} | ||
|
||
|
@@ -96,11 +109,11 @@ public void TestInvalidContainer() | |
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.BuildResource(Path.GetTempPath()), Resource.Empty); | ||
Assert.Equal(dockerResourceDetector.BuildResourceV1(Path.GetTempPath()), Resource.Empty); | ||
} | ||
|
||
private string GetContainerId(Resource resource) | ||
|
There was a problem hiding this comment.
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, thatExtractContainerIdV2
should use it, instead ofFILEPATH
. Proper fix for it will also solve problem mentioned by @alexeypukhov , as that functions should check different file.There was a problem hiding this comment.
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.