Repository navigation
test(otel): cover Karpenter node-label and pod-label counterpart removal - #775
Conversation
a34903a to
00274b1
Compare
eb38de0 to
c0c287a
Compare
| // scope. The node forms of four of them were already denied, so a datapoint carrying the | ||
| // pod form is the duplicate this change removes. | ||
| func TestPodLabelCounterpartsRemoved(t *testing.T) { | ||
| for _, metricName := range nodeLabelEnrichedNames() { |
There was a problem hiding this comment.
I think this loops over the wrong set. nodeLabelEnrichedNames() pulls in node-exporter and the node-level kubeletstats metrics (k8s.node.cpu.usage etc), and those never carry k8s.pod.label.* in the first place. So 9 of the 19 subtests can't fail.
It also skips the cluster-scraper path. helm-charts#383 adds the same 3 pod keys to the cluster-scraper denylist, and KSM pod metrics do carry pod labels (TestKSM_PodBucket_HasPodLabels), so that path should be covered too. ksm_test.go:1717 already does a similar check for pod-template-hash.
Suggest something like:
names := append(podMetricNames(), ksmPodBucket...)
names = append(names, ksmContainerBucket...)
for _, metricName := range names {TestPodLabelsStillEnriched below already uses podMetricNames(), so this would also line the two tests up. Note podScopedMetricNames() won't work as a drop in since it still includes node-exporter.
There was a problem hiding this comment.
Good catch, fixed. Both pod-label tests now loop over podMetricNames() plus ksmPodBucket and ksmContainerBucket. The node-level subtests that could not fail are gone, and the cluster-scraper path is covered.
| // deniedKarpenterNodeLabels samples the karpenter.k8s.aws/instance- entry in | ||
| // unconditional_removal_prefixes, plus the three karpenter.sh/ entries in | ||
| // unconditional_removal_keys. | ||
| var deniedKarpenterNodeLabels = []string{ |
There was a problem hiding this comment.
helm-charts#383 also adds the k8s.node.label.eks.amazonaws.com/instance- prefix to both denylists, but nothing here (or in #776) checks it. Could we add it to this list so the prefix is covered?
Also, the Karpenter tests only look at daemonset metrics, so the cluster-scraper side of the node label change isn't tested. KSM metrics carry node labels too (ksm_test.go:322), so adding a KSM node metric to the loop would cover it.
There was a problem hiding this comment.
Done. The removal test now checks the whole karpenter.k8s.aws/instance- and eks.amazonaws.com/instance- prefixes instead of sample keys. Only an Auto Mode cluster exercises the eks.amazonaws.com/instance- prefix. Both Karpenter tests now also read ksmNodeBucket.
c0c287a to
5ab0575
Compare
| var retainedKarpenterNodeLabels = []string{ | ||
| karpenterNodePoolLabel, | ||
| "k8s.node.label.karpenter.sh/capacity-type", | ||
| "k8s.node.label.karpenter.k8s.aws/ec2nodeclass", |
There was a problem hiding this comment.
Auto Mode nodes carry eks.amazonaws.com/nodeclass, not karpenter.k8s.aws/ec2nodeclass
There was a problem hiding this comment.
Good catch. The retention test now splits the nodes by eks.amazonaws.com/compute-type. Auto Mode nodes must keep karpenter.sh/nodepool, karpenter.sh/capacity-type, eks.amazonaws.com/nodeclass and eks.amazonaws.com/compute-type. Karpenter nodes keep their own list, and a kind of node that the cluster does not have is skipped.
5ab0575 to
7b30422
Compare
7b30422 to
475fdd7
Compare
Description of the issue
The Container Insights attribute denylist in
awsattributelimit/cw_k8s_ci_v0is gaining Karpenter and EKS Auto Mode node labels plus three pod labels. Both the node agent and the cluster scraper run that denylist, and it had no integration coverage. Thekarpenter.k8s.aws/andkarpenter.sh/domains are not denied by prefix, because 9 keys in them are kept. Three narrowerkarpenter.k8s.aws/instance-prefixes and 11 exact keys deny the instance labels derived from the instance type.eks.amazonaws.com/instance-denies their EKS Auto Mode copies.instance-tenancyis the one instance label that is kept, because tenancy is a launch property. The 3karpenter.sh/keys are enumerated. So the interesting failure mode is not "a key survived" but "a retained key was removed too", which nothing tested.Description of changes
Adds four tests to the standard suite, next to the existing
nfd_labels_droppedsubtest ofTestNodeLabelEnrichment(node-feature-discovery, thefeature.node.kubernetes.io/family), which exercises the same denylist path. Two cover the Karpenter keys and two cover the pod-label counterparts. Each test reads the metrics of both agents, because both run the denylist: the node agent's DaemonSet metrics and the cluster scraper's kube-state-metrics metrics.TestKarpenterInstanceLabelsRemovedasserts that no key under thekarpenter.k8s.aws/instance-andeks.amazonaws.com/instance-prefixes reaches resource scope, exceptinstance-tenancy. The same holds for the 3karpenter.sh/node-management flags:initialized,registeredanddo-not-sync-taints. The test matches the whole prefixes rather than a sample of keys. So if Karpenter adds an instance label that the chart does not deny, the test fails. Only an EKS Auto Mode cluster exercises theeks.amazonaws.com/instance-prefix.TestKarpenterGroupingLabelsRetainedasserts that the four keys Karpenter sets on every node it manages still arrive on at least one node. They arekarpenter.sh/nodepool,karpenter.sh/capacity-type,karpenter.k8s.aws/ec2nodeclassandkarpenter.k8s.aws/instance-tenancy. EKS Auto Mode nodes, which haveeks.amazonaws.com/compute-typeset toauto, are checked against their own list:karpenter.sh/nodepool,karpenter.sh/capacity-type,eks.amazonaws.com/nodeclassandeks.amazonaws.com/compute-type. A kind of node that the cluster does not have is skipped. It requires one node rather than every node, because a cluster can mix Karpenter-managed nodes with a static node group.The pair is the point. A regression can widen the narrow prefixes or keys to the whole
karpenter.k8s.aws/instance-prefix or thekarpenter.k8s.aws/andkarpenter.sh/domains. That regression fails the retention test, even where it passes the removal test. Both tests read the series of the nodes that carrykarpenter.sh/nodepool, on every instance type. If there are none, they fail rather than skip, because a domain-prefix regression strips every Karpenter attribute. The suite'squeryCache.Getpins node-agent metrics toclusterHostTypes(t3.medium). The Karpenter NodePool can launch any c, m, r or t type from medium to xlarge, so a pinned read can miss the Karpenter node. The filter also keeps each read under the endpoint's 500-series limit on a large cluster.capacity-reservation-id,-type,-interruptibleandplacement-group-id,-partitionare retained by the denylist but not asserted, because Karpenter only sets them on nodes launched into a capacity reservation or a placement group, so they are absent from an ordinary cluster.TestPodLabelCounterpartsRemovedasserts that none of the three newk8s.pod.label.*keys reach resource scope. Two are the pod-side counterparts of node labels the denylist already removed,topology.kubernetes.io/regionandtopology.kubernetes.io/zone. The third ispod-template-generation. The test reads the pod and container metrics of both agents on every node. These are the node agent's pod-scoped cAdvisor and kubeletstats metrics and the kube-state-metrics pod and container metrics. Node-scoped metrics have no pod, so they never carryk8s.pod.label.*and cannot fail.TestPodLabelsStillEnrichedis the complement, and the same shape of guard as the Karpenter pair. It reads the same metrics and requires at least onek8s.pod.label.*attribute other than the three denied keys. If the denylist widens to thek8s.pod.label.prefix, this test fails. Without it, the removal assertion passes while every pod label is stripped.License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
gofmtclean andgo vet -tags integration ./test/otel/standard/passes; the test binary builds with all four test functions registered.The
instance-tenancyexception and its retention assertion have not run on a cluster yet. The other assertions passed on an EKS test cluster, 68 of 68 subtests. WithclusterHostTypesset to an instance type that no Karpenter node has, as in CI, they still pass. A read through that pin finds no Karpenter node, and the retention check then fails on every DaemonSet metric. Three mutations each deny a label that is present on the cluster:karpenter.sh/capacity-type, thekubernetes.io/node-label prefix and the pod labelapp.kubernetes.io/name. Each mutation fails every subtest of the matching removal test, on the metrics of both agents.These are integration tests, so they need a live cluster running the paired configuration change. They will fail against a cluster that has not yet taken the denylist update — that is the intended behaviour, and the reason this PR is paired one-to-one with the config change rather than bundled with the other Container Insights test additions.
Related pull requests
PR Checklist
gofmtclean,go vet -tags integrationpasses, test binary builds