Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver. #186

straussb · 2024-03-20T02:44:09Z

Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver.

The new component scrapes hardware counters from /sys/class/infiniband on disk. The layout of that directory is:

/sys/class/infiniband/<device name>
└── ports
    └── 1
        └── hw_counters
            ├── rdma_read_bytes
            ├── rdma_write_bytes
            ├── rdma_write_recv_bytes
            ├── rx_bytes
            ├── rx_drops
            └── tx_bytes

These are cumulative counters and so they are converted to deltas before sending down the pipeline.
We sum up data from all ports.

The device data is joined with data from the kubelet podresources API which tells us which container a given device is
assigned to.

The metrics are reported at container, pod, and node levels.

This commit also refactors some metric decoration code from cadvisor to a common localnode decorator, intended to be
used by any awscontainerinsightreceiver component that gathers metrics from the local node (as oppoosed to e.g. the k8s
control plane scraper). This is because we want to share the logic of populating PodName, Kubernetes labels, etc.

I also renamed RawContainerInsightsMetric to CIMetricImpl for brevity.

See also accompanying cloudwatch-agent PR: aws/amazon-cloudwatch-agent#1098

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

receiver/awscontainerinsightreceiver/internal/cadvisor/testutils/helpers.go

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

sam6134 · 2024-03-22T14:36:22Z

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

+
+func (s *Scraper) fillMetric(metric stores.CIMetric, metricType string, namespace string, podName string,
+	containerName string, deviceName string, timestamp time.Time, counters *efaCounters) {
+	s.assignRateValueToField(ci.MetricName(metricType, ci.EfaRdmaReadBytes), counters.rdmaReadBytes,


Probably makes sense to have a map, and you can loop over to assignRate value to each field

I think I did what you're asking in the next commit, let me know if you were imagining something different.

sam6134 · 2024-03-22T15:13:06Z

receiver/awscontainerinsightreceiver/internal/stores/localnode.go

+			d.logger.Warn("Can't get containerInstanceId")
+		} else {
+			tags[ci.ContainerInstanceIDKey] = d.ecsInfo.GetContainerInstanceID()
+		}


Why cant we move lines 110-120 to addECSResources func as well

Sure, done. I had moved this from cadvisor_linux and didn't make any modifications.

sam6134 · 2024-03-22T15:25:08Z

receiver/awscontainerinsightreceiver/internal/stores/localnode.go

+	}
+}
+
+func addECSResources(tags map[string]string) {


Might be a silly question, but is there any reason not to reuse this - https://github.com/amazon-contributing/opentelemetry-collector-contrib/blob/main/receiver/awscontainerinsightreceiver/internal/stores/utils.go#L98

Done, moved the Instance* metric type logic to there.

straussb · 2024-03-26T03:36:11Z

For latest revision, I squashed all my commits together and rebased on aws-cwa-dev.

This got rid of the history... if you want to see the previous state of my PR, you can view the tree at: https://github.com/straussb/opentelemetry-collector-contrib/tree/db5503cb45f8e78b169da51c95bc4489f55db3ff

And compare to current state at: https://github.com/straussb/opentelemetry-collector-contrib/compare/db5503cb45f8e78b169da51c95bc4489f55db3ff..efa

The files that had rebase conflicts were:

internal/aws/containerinsight/const.go
receiver/awscontainerinsightreceiver/internal/prometheusscraper/decoratorconsumer/decorator.go
receiver/awscontainerinsightreceiver/internal/stores/utils.go
receiver/awscontainerinsightreceiver/internal/stores/utils_test.go
receiver/awscontainerinsightreceiver/receiver.go

So review those a bit more carefully.

Since last review, I also added a bit more logic in efaSysfs.go and podresourcesclient.go to check permissions/ownership of /sys/class/infiniband and the kubelet podresources socket, for security.

movence · 2024-03-26T12:01:57Z

internal/aws/containerinsight/const.go

+	TypeContainerGPU    = "ContainerGPU"
+	TypePodGPU          = "PodGPU"
+	TypeNodeGPU         = "NodeGPU"
+	TypeClusterGPU      = "ClusterGPU"
 	TypeNeuronContainer = "ContainerNeuron"


This doesn't need to be updated in this commit, but it would be nice to update this to TypeContainerNeuron as well for consistency

Thanks, done.

movence · 2024-03-26T12:07:31Z

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

+
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		return true, nil


is it ok to return this condition as true?

This would essentially mean that we are not running on Linux. But you're right, we should skip in that case so we remember to implement this logic for Windows if we ever add support for that. Changed in next commit.

movence · 2024-03-26T14:09:49Z

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

+	// This was already patched and submitted, see
+	// https://www.spinics.net/lists/linux-rdma/msg68596.html
+	// Remove this as soon as the fix lands in the enterprise distros.
+	if strings.Contains(stringValue, "N/A (no PMA)") {


will this break if the fix gets promoted? how do we track the release of this change?

Good question. I'm going to follow up with a commit to improve partial error handling - still post node-level metrics if we couldn't get pod resources data, as we discussed in the meeting; and also include data for some devices/counters even if reading some other data fails. As part of that commit, I will remove this logic and just skip if parsing to int failed.

(The comment is wrong anyway, I copied from somewhere else, so thanks for pointing this out. There's no upstream fix that will change anything, since we've implemented our own counter-reading code here.)

sam6134 · 2024-03-27T14:38:13Z

receiver/awscontainerinsightreceiver/receiver.go

 			if err != nil {
 				acir.settings.Logger.Debug("Unable to start neuron scraper", zap.Error(err))
 			}
+
+			acir.initEfaSysfsScraper(localnodeDecorator)


I think we should catch a error here as well, and log if the EFA scraper is not able to start

Previously there was no reason that the scraper couldn't start. Added one case now.

sam6134 · 2024-03-27T14:40:56Z

receiver/awscontainerinsightreceiver/internal/efa/efaSysfs.go

+	ReadCounter(deviceName efaDeviceName, port string, counter string) (uint64, error)
+}
+
+type podResourcesStore interface {


Why create a new one, we can reuse the one defined in the neuronScraper

I think it's OK to duplicate in this case - I think the typical Go style would say that each class can independently mock out its dependencies as it chooses to. I thought about moving the interface to podresourcesstore.go but it doesn't make sense to take that flexibility away from the dependent classes.

…ightreceiver. The new component scrapes hardware counters from /sys/class/infiniband on disk. The layout of that directory is: /sys/class/infiniband/<device name> └── ports └── 1 └── hw_counters ├── rdma_read_bytes ├── rdma_write_bytes ├── rdma_write_recv_bytes ├── rx_bytes ├── rx_drops └── tx_bytes These are cumulative counters and so they are converted to deltas before sending down the pipeline. We sum up data from all ports. The device data is joined with data from the kubelet podresources API which tells us which container a given device is assigned to. The metrics are reported at container, pod, and node levels. This commit also refactors some metric decoration code from cadvisor to a common localnode decorator, intended to be used by any awscontainerinsightreceiver component that gathers metrics from the local node (as oppoosed to e.g. the k8s control plane scraper). This is because we want to share the logic of populating PodName, Kubernetes labels, etc. I also renamed RawContainerInsightsMetric to CIMetricImpl for brevity.

straussb requested a review from mxiamxia as a code owner March 20, 2024 02:44

straussb mentioned this pull request Mar 20, 2024

Add support for Elastic Fabric Adapter (EFA) metrics. aws/amazon-cloudwatch-agent#1098

Closed

movence reviewed Mar 22, 2024

View reviewed changes

sam6134 reviewed Mar 22, 2024

View reviewed changes

movence previously approved these changes Mar 22, 2024

View reviewed changes

straussb dismissed movence’s stale review via 7378f34 March 26, 2024 02:48

straussb force-pushed the efa branch from 7378f34 to 2dcb070 Compare March 26, 2024 03:26

movence reviewed Mar 26, 2024

View reviewed changes

sam6134 reviewed Mar 27, 2024

View reviewed changes

straussb added 4 commits March 27, 2024 19:02

remove accidentally committed files

ae84d51

PR comments

3f3084c

fix unit test

ce1f394

straussb force-pushed the efa branch from de29627 to ce1f394 Compare March 27, 2024 19:04

movence previously approved these changes Mar 27, 2024

View reviewed changes

sam6134 self-requested a review March 28, 2024 16:32

sam6134 previously approved these changes Mar 28, 2024

View reviewed changes

bugfix for nil podresourcesstore

f1384a8

straussb dismissed stale reviews from sam6134 and movence via f1384a8 March 28, 2024 18:16

aditya-purang approved these changes Mar 28, 2024

View reviewed changes

movence approved these changes Mar 28, 2024

View reviewed changes

straussb merged commit eed8454 into amazon-contributing:aws-cwa-dev Mar 28, 2024
51 of 67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver. #186

Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver. #186

straussb commented Mar 20, 2024 •

edited

Loading

sam6134 Mar 22, 2024

straussb Mar 22, 2024

sam6134 Mar 22, 2024

straussb Mar 22, 2024

sam6134 Mar 22, 2024

straussb Mar 22, 2024

straussb commented Mar 26, 2024

movence Mar 26, 2024 •

edited

Loading

straussb Mar 26, 2024

movence Mar 26, 2024

straussb Mar 26, 2024

movence Mar 26, 2024 •

edited

Loading

straussb Mar 26, 2024 •

edited

Loading

sam6134 Mar 27, 2024

straussb Mar 27, 2024

sam6134 Mar 27, 2024

straussb Mar 27, 2024 •

edited

Loading

Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver. #186

Add Elastic Fabric Adapter (EFA) metric collection to awscontainerinsightreceiver. #186

Conversation

straussb commented Mar 20, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straussb commented Mar 26, 2024

movence Mar 26, 2024 • 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

movence Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

straussb Mar 26, 2024 • 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

straussb Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

straussb commented Mar 20, 2024 •

edited

Loading

movence Mar 26, 2024 •

edited

Loading

movence Mar 26, 2024 •

edited

Loading

straussb Mar 26, 2024 •

edited

Loading

straussb Mar 27, 2024 •

edited

Loading