Fix PV ProviderID (#4039)

Signed-off-by: Sean Holcomb <seanholcomb@gmail.com>

core/pkg/clustercache/helper.go

@@ -1,5 +1,9 @@
 package clustercache
 
+import (
+	"regexp"
+)
+
 func GetLoadBalancerIngressAddress(service *Service) []string {
 	var addresses []string
 	for _, loadBalancerIngress := range service.Status.LoadBalancer.Ingress {
@@ -13,3 +17,24 @@ func GetLoadBalancerIngressAddress(service *Service) []string {
 	}
 	return addresses
 }
+
+// Capture "vol-0fc54c5e83b8d2b76" from "aws://us-east-2a/vol-0fc54c5e83b8d2b76"
+var persistentVolumeAWSRegex = regexp.MustCompile("aws:/[^/]*/[^/]*/([^/]+)")
+
+func GetPVProviderID(pv *PersistentVolume) string {
+	providerID := pv.Name
+	if pv.Spec.GCEPersistentDisk != nil {
+		providerID = pv.Spec.GCEPersistentDisk.PDName
+	} else if pv.Spec.AzureDisk != nil {
+		providerID = pv.Spec.AzureDisk.DiskName
+	} else if pv.Spec.AWSElasticBlockStore != nil {
+		providerID = pv.Spec.AWSElasticBlockStore.VolumeID
+		match := persistentVolumeAWSRegex.FindStringSubmatch(providerID)
+		if len(match) >= 2 {
+			providerID = match[1]
+		}
+	} else if pv.Spec.CSI != nil {
+		providerID = pv.Spec.CSI.VolumeHandle
+	}
+	return providerID
+}

core/pkg/clustercache/helper_test.go

@@ -87,3 +87,179 @@ func TestGetLoadBalancerIngressAddress(t *testing.T) {
 		})
 	}
 }
+
+func Test_getPVProviderID(t *testing.T) {
+	tests := []struct {
+		name string
+		pv   *PersistentVolume
+		want string
+	}{
+		{
+			name: "gce persistent disk uses pd name",
+			pv: &PersistentVolume{
+				Name: "pv-gce",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{PDName: "gke-pd-1"},
+					},
+				},
+			},
+			want: "gke-pd-1",
+		},
+		{
+			name: "azure disk uses disk name",
+			pv: &PersistentVolume{
+				Name: "pv-azure",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						AzureDisk: &v1.AzureDiskVolumeSource{DiskName: "azure-disk-1"},
+					},
+				},
+			},
+			want: "azure-disk-1",
+		},
+		{
+			name: "aws ebs with aws:// prefixed volume id is parsed",
+			pv: &PersistentVolume{
+				Name: "pv-aws",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
+							VolumeID: "aws://us-east-2a/vol-0fc54c5e83b8d2b76",
+						},
+					},
+				},
+			},
+			want: "vol-0fc54c5e83b8d2b76",
+		},
+		{
+			name: "aws ebs with bare volume id is left unchanged",
+			pv: &PersistentVolume{
+				Name: "pv-aws",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
+							VolumeID: "vol-abc123",
+						},
+					},
+				},
+			},
+			want: "vol-abc123",
+		},
+		{
+			name: "aws ebs with empty volume id yields empty string",
+			pv: &PersistentVolume{
+				Name: "pv-aws",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{VolumeID: ""},
+					},
+				},
+			},
+			want: "",
+		},
+		{
+			name: "csi uses volume handle",
+			pv: &PersistentVolume{
+				Name: "pv-csi",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{VolumeHandle: "vol-csi-1"},
+					},
+				},
+			},
+			want: "vol-csi-1",
+		},
+		{
+			// Documents current behavior: a CSI source with an empty handle
+			// returns "" rather than falling back to pv.Name.
+			name: "csi with empty volume handle returns empty string",
+			pv: &PersistentVolume{
+				Name: "pv-csi",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{VolumeHandle: ""},
+					},
+				},
+			},
+			want: "",
+		},
+		{
+			name: "no recognized source falls back to pv name",
+			pv: &PersistentVolume{
+				Name: "pv-nfs",
+				Spec: v1.PersistentVolumeSpec{},
+			},
+			want: "pv-nfs",
+		},
+		{
+			// GCE branch is checked before CSI, so GCE wins when both are set.
+			name: "gce takes precedence over csi",
+			pv: &PersistentVolume{
+				Name: "pv-both",
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{PDName: "gce-wins"},
+						CSI:               &v1.CSIPersistentVolumeSource{VolumeHandle: "csi-loses"},
+					},
+				},
+			},
+			want: "gce-wins",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := GetPVProviderID(tt.pv); got != tt.want {
+				t.Errorf("getPVProviderID() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_persistentVolumeAWSRegex(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string // expected capture group 1, or "" for no match
+	}{
+		{
+			name:  "standard aws:// volume id",
+			input: "aws://us-east-2a/vol-0fc54c5e83b8d2b76",
+			want:  "vol-0fc54c5e83b8d2b76",
+		},
+		{
+			name:  "trailing path segment stops at slash",
+			input: "aws://us-east-2a/vol-123/extra",
+			want:  "vol-123",
+		},
+		{
+			name:  "bare volume id does not match",
+			input: "vol-abc123",
+			want:  "",
+		},
+		{
+			name:  "too few segments does not match",
+			input: "aws://vol-123",
+			want:  "",
+		},
+		{
+			name:  "empty string does not match",
+			input: "",
+			want:  "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			match := persistentVolumeAWSRegex.FindStringSubmatch(tt.input)
+			got := ""
+			if len(match) >= 2 {
+				got = match[1]
+			}
+			if got != tt.want {
+				t.Errorf("persistentVolumeAWSRegex on %q = %q, want %q", tt.input, got, tt.want)
+			}
+		})
+	}
+}

core/pkg/compute/kubemodel/persistentvolume.go

@@ -21,10 +21,10 @@ func (km *KubeModel) computePersistentVolumes(kms *kubemodel.KubeModelSet, start
 	pvInfoResult, _ := pvInfoResultFuture.Await()
 	for _, res := range pvInfoResult {
 		pvMap[res.UID] = &kubemodel.PersistentVolume{
-			UID:             res.UID,
-			Name:            res.PersistentVolume,
-			StorageClass:    res.StorageClass,
-			CSIVolumeHandle: res.CSIVolumeHandle,
+			UID:          res.UID,
+			Name:         res.PersistentVolume,
+			StorageClass: res.StorageClass,
+			ProviderID:   res.ProviderID,
 		}
 	}
 

core/pkg/compute/kubemodel/persistentvolume_test.go

@@ -54,7 +54,28 @@ func TestComputePersistentVolumes(t *testing.T) {
 			want: map[string]*kubemodel.PersistentVolume{},
 		},
 		{
-			name: "pv with storage class and csi volume handle",
+			name: "pv with storage class and provider id",
+			overrides: map[string]any{
+				source.QueryKMPVInfo: []*source.PVInfoResult{
+					{UID: "pv-1", PersistentVolume: "pvc-data-0", StorageClass: "gp2", ProviderID: "vol-abc123"},
+				},
+				source.QueryPVUptime: []*source.UptimeResult{
+					{UID: "pv-1", First: start, Last: end},
+				},
+			},
+			want: map[string]*kubemodel.PersistentVolume{
+				"pv-1": {
+					UID:          "pv-1",
+					Name:         "pvc-data-0",
+					StorageClass: "gp2",
+					ProviderID:   "vol-abc123",
+					Start:        start,
+					End:          end,
+				},
+			},
+		},
+		{
+			name: "pv with csi volume handle",
 			overrides: map[string]any{
 				source.QueryKMPVInfo: []*source.PVInfoResult{
 					{UID: "pv-1", PersistentVolume: "pvc-data-0", StorageClass: "gp2", CSIVolumeHandle: "vol-abc123"},
@@ -65,12 +86,12 @@ func TestComputePersistentVolumes(t *testing.T) {
 			},
 			want: map[string]*kubemodel.PersistentVolume{
 				"pv-1": {
-					UID:             "pv-1",
-					Name:            "pvc-data-0",
-					StorageClass:    "gp2",
-					CSIVolumeHandle: "vol-abc123",
-					Start:           start,
-					End:             end,
+					UID:          "pv-1",
+					Name:         "pvc-data-0",
+					StorageClass: "gp2",
+					ProviderID:   "",
+					Start:        start,
+					End:          end,
 				},
 			},
 		},

core/pkg/model/kubemodel/kubemodel_codecs.go

@@ -4216,7 +4216,6 @@ func (target *KubeModelSet) UnmarshalBinaryWithContext(ctx *DecodingContext) (er
 		if buff.ReadUInt8() == uint8(0) {
 			target.Metadata = nil
 		} else {
-
 			// --- [begin][read][struct](Metadata) ---
 			a := new(Metadata)
 			buff.ReadInt() // [compatibility, unused]
@@ -4234,6 +4233,7 @@ func (target *KubeModelSet) UnmarshalBinaryWithContext(ctx *DecodingContext) (er
 	}
 	// field version check
 	if uint8(1) <= version {
+
 		// --- [begin][read][struct](Window) ---
 		b := new(Window)
 		buff.ReadInt() // [compatibility, unused]
@@ -4650,6 +4650,7 @@ func (target *KubeModelSet) UnmarshalBinaryWithContext(ctx *DecodingContext) (er
 				if buff.ReadUInt8() == uint8(0) {
 					zzzzzzzzz = nil
 				} else {
+
 					// --- [begin][read][struct](ReplicaSet) ---
 					rrr := new(ReplicaSet)
 					buff.ReadInt() // [compatibility, unused]
@@ -4740,7 +4741,6 @@ func (target *KubeModelSet) UnmarshalBinaryWithContext(ctx *DecodingContext) (er
 				if buff.ReadUInt8() == uint8(0) {
 					zzzzzzzzzzz = nil
 				} else {
-
 					// --- [begin][read][struct](PersistentVolume) ---
 					ffff := new(PersistentVolume)
 					buff.ReadInt() // [compatibility, unused]
@@ -4922,7 +4922,6 @@ func (target *KubeModelSet) UnmarshalBinaryWithContext(ctx *DecodingContext) (er
 				if buff.ReadUInt8() == uint8(0) {
 					zzzzzzzzzzzzzzz = nil
 				} else {
-
 					// --- [begin][read][struct](Device) ---
 					lllll := new(Device)
 					buff.ReadInt() // [compatibility, unused]
@@ -5602,6 +5601,7 @@ func (stream *KubeModelSetStream) Stream() iter.Seq2[BingenFieldInfo, *BingenVal
 					if buff.ReadUInt8() == uint8(0) {
 						zzzzzzzzz = nil
 					} else {
+
 						// --- [begin][read][struct](ReplicaSet) ---
 						lll := new(ReplicaSet)
 						buff.ReadInt() // [compatibility, unused]
@@ -5720,7 +5720,6 @@ func (stream *KubeModelSetStream) Stream() iter.Seq2[BingenFieldInfo, *BingenVal
 					if buff.ReadUInt8() == uint8(0) {
 						zzzzzzzzzzz = nil
 					} else {
-
 						// --- [begin][read][struct](PersistentVolume) ---
 						www := new(PersistentVolume)
 						buff.ReadInt() // [compatibility, unused]
@@ -5958,7 +5957,6 @@ func (stream *KubeModelSetStream) Stream() iter.Seq2[BingenFieldInfo, *BingenVal
 					if buff.ReadUInt8() == uint8(0) {
 						zzzzzzzzzzzzzzz = nil
 					} else {
-
 						// --- [begin][read][struct](Device) ---
 						uuuu := new(Device)
 						buff.ReadInt() // [compatibility, unused]
@@ -6065,6 +6063,7 @@ func (target *Metadata) MarshalBinaryWithContext(ctx *EncodingContext) (err erro
 		// --- [begin][write][slice]([]Diagnostic) ---
 		buff.WriteInt(len(target.Diagnostics)) // slice length
 		for i := range target.Diagnostics {
+
 			// --- [begin][write][struct](Diagnostic) ---
 			buff.WriteInt(0) // [compatibility, unused]
 			errC := target.Diagnostics[i].MarshalBinaryWithContext(ctx)
@@ -6188,6 +6187,7 @@ func (target *Metadata) UnmarshalBinaryWithContext(ctx *DecodingContext) (err er
 			l := buff.ReadInt() // slice len
 			h := make([]Diagnostic, l)
 			for i := range l {
+
 				// --- [begin][read][struct](Diagnostic) ---
 				n := new(Diagnostic)
 				buff.ReadInt() // [compatibility, unused]
@@ -6210,6 +6210,7 @@ func (target *Metadata) UnmarshalBinaryWithContext(ctx *DecodingContext) (err er
 	}
 	// field version check
 	if uint8(1) <= version {
+
 		// --- [begin][read][alias](DiagnosticLevel) ---
 		var o int
 		p := buff.ReadInt() // read int
@@ -7434,10 +7435,10 @@ func (target *PersistentVolume) MarshalBinaryWithContext(ctx *EncodingContext) (
 	}
 
 	if ctx.IsStringTable() {
-		d := ctx.Table.AddOrGet(target.CSIVolumeHandle)
+		d := ctx.Table.AddOrGet(target.ProviderID)
 		buff.WriteInt(d) // write table index
 	} else {
-		buff.WriteString(target.CSIVolumeHandle) // write string
+		buff.WriteString(target.ProviderID) // write string
 	}
 
 	buff.WriteFloat64(target.SizeBytes) // write float64
@@ -7552,7 +7553,7 @@ func (target *PersistentVolume) UnmarshalBinaryWithContext(ctx *DecodingContext)
 		n = buff.ReadString() // read string
 	}
 	m := n
-	target.CSIVolumeHandle = m
+	target.ProviderID = m
 
 	p := buff.ReadFloat64() // read float64
 	target.SizeBytes = p
@@ -9638,7 +9639,6 @@ func (target *ResourceQuotaSpecHard) UnmarshalBinaryWithContext(ctx *DecodingCon
 
 	// field version check
 	if uint8(1) <= version {
-
 		// --- [begin][read][alias](ResourceQuantities) ---
 		var a map[Resource]ResourceQuantity
 		if buff.ReadUInt8() == uint8(0) {
@@ -9687,7 +9687,6 @@ func (target *ResourceQuotaSpecHard) UnmarshalBinaryWithContext(ctx *DecodingCon
 	}
 	// field version check
 	if uint8(1) <= version {
-
 		// --- [begin][read][alias](ResourceQuantities) ---
 		var l map[Resource]ResourceQuantity
 		if buff.ReadUInt8() == uint8(0) {
@@ -10048,7 +10047,6 @@ func (target *ResourceQuotaStatusUsed) UnmarshalBinaryWithContext(ctx *DecodingC
 
 	// field version check
 	if uint8(1) <= version {
-
 		// --- [begin][read][alias](ResourceQuantities) ---
 		var a map[Resource]ResourceQuantity
 		if buff.ReadUInt8() == uint8(0) {
@@ -10097,7 +10095,6 @@ func (target *ResourceQuotaStatusUsed) UnmarshalBinaryWithContext(ctx *DecodingC
 	}
 	// field version check
 	if uint8(1) <= version {
-
 		// --- [begin][read][alias](ResourceQuantities) ---
 		var l map[Resource]ResourceQuantity
 		if buff.ReadUInt8() == uint8(0) {

core/pkg/model/kubemodel/mock.go

@@ -228,13 +228,13 @@ func NewMockKubeModelSet(start, end time.Time) *KubeModelSet {
 
 	// --- PersistentVolume ---
 	kms.RegisterPersistentVolume(&PersistentVolume{
-		UID:             "pv-uid",
-		Name:            "pvc-abc123",
-		StorageClass:    "gp2",
-		CSIVolumeHandle: "vol-0abc123def456789",
-		SizeBytes:       50e9,
-		Start:           start,
-		End:             end,
+		UID:          "pv-uid",
+		Name:         "pvc-abc123",
+		StorageClass: "gp2",
+		ProviderID:   "vol-0abc123def456789",
+		SizeBytes:    50e9,
+		Start:        start,
+		End:          end,
 	})
 
 	// --- Device ---

core/pkg/model/kubemodel/pv.go

@@ -7,13 +7,13 @@ import (
 
 // @bingen:generate:PersistentVolume
 type PersistentVolume struct {
-	UID             string    `json:"uid"`
-	Name            string    `json:"name"`
-	StorageClass    string    `json:"storageClass"`
-	CSIVolumeHandle string    `json:"csiVolumeHandle,omitempty"`
-	SizeBytes       float64   `json:"size"`
-	Start           time.Time `json:"start"`
-	End             time.Time `json:"end"`
+	UID          string    `json:"uid"`
+	Name         string    `json:"name"`
+	StorageClass string    `json:"storageClass"`
+	ProviderID   string    `json:"providerID,omitempty"`
+	SizeBytes    float64   `json:"size"`
+	Start        time.Time `json:"start"`
+	End          time.Time `json:"end"`
 }
 
 func (p *PersistentVolume) ValidatePersistentVolume(window Window) error {

modules/collector-source/pkg/scrape/clustercache.go

@@ -688,19 +688,17 @@ func (ccs *ClusterCacheScraper) GetScrapePVs(pvs []*clustercache.PersistentVolum
 func (ccs *ClusterCacheScraper) scrapePVs(pvs []*clustercache.PersistentVolume) []metric.Update {
 	var scrapeResults []metric.Update
 	for _, pv := range pvs {
-		providerID := pv.Name
-		var csiVolumeHandle string
-		// if a more accurate provider ID is available, use that
-		if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" {
-			providerID = pv.Spec.CSI.VolumeHandle
-			csiVolumeHandle = pv.Spec.CSI.VolumeHandle
-		}
+		providerID := clustercache.GetPVProviderID(pv)
+
 		pvInfo := map[string]string{
-			source.UIDLabel:             string(pv.UID),
-			source.PVLabel:              pv.Name,
-			source.StorageClassLabel:    pv.Spec.StorageClassName,
-			source.ProviderIDLabel:      providerID,
-			source.CSIVolumeHandleLabel: csiVolumeHandle,
+			source.UIDLabel:          string(pv.UID),
+			source.PVLabel:           pv.Name,
+			source.StorageClassLabel: pv.Spec.StorageClassName,
+			source.ProviderIDLabel:   providerID,
+		}
+
+		if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" {
+			pvInfo[source.CSIVolumeHandleLabel] = pv.Spec.CSI.VolumeHandle
 		}
 
 		scrapeResults = append(scrapeResults, metric.Update{
@@ -1339,7 +1337,7 @@ func getPersistentVolumeClaimClass(claim *clustercache.PersistentVolumeClaim) st
 // toResourceUnitValue accepts a resource name and quantity and returns the sanitized resource, the unit, and the value in the units.
 // Returns an empty string for resource and unit if there was a failure.
 func toResourceUnitValue(resourceName v1.ResourceName, quantity resource.Quantity) (resource string, unit string, value float64) {
-	resource = promutil.SanitizeLabelName(string(resourceName))
+	resource = resourceName.String()
 
 	switch resourceName {
 	case v1.ResourceCPU:

modules/collector-source/pkg/scrape/clustercache_test.go

@@ -1351,6 +1351,182 @@ func Test_kubernetesScraper_scrapePVs(t *testing.T) {
 				},
 			},
 		},
+		{
+			// Non-CSI PV: provider ID comes from the in-tree AWS EBS source and
+			// the csi_volume_handle label must not be present at all.
+			name: "aws ebs non-csi pv omits csi_volume_handle label",
+			scrapes: []scrape{
+				{
+					PVs: []*clustercache.PersistentVolume{
+						{
+							Name: "pv-aws",
+							UID:  "uuid-aws",
+							Spec: v1.PersistentVolumeSpec{
+								StorageClassName: "gp2",
+								PersistentVolumeSource: v1.PersistentVolumeSource{
+									AWSElasticBlockStore: &v1.AWSElasticBlockStoreVolumeSource{
+										VolumeID: "aws://us-east-2a/vol-0fc54c5e83b8d2b76",
+									},
+								},
+								Capacity: v1.ResourceList{
+									v1.ResourceStorage: resource.MustParse("8192"),
+								},
+							},
+						},
+					},
+					Timestamp: start1,
+				},
+			},
+			expected: []metric.Update{
+				{
+					Name: metric.KubecostPVInfo,
+					Labels: map[string]string{
+						source.UIDLabel:          "uuid-aws",
+						source.PVLabel:           "pv-aws",
+						source.StorageClassLabel: "gp2",
+						source.ProviderIDLabel:   "vol-0fc54c5e83b8d2b76",
+					},
+					Value: 0,
+					AdditionalInfo: map[string]string{
+						source.UIDLabel:          "uuid-aws",
+						source.PVLabel:           "pv-aws",
+						source.StorageClassLabel: "gp2",
+						source.ProviderIDLabel:   "vol-0fc54c5e83b8d2b76",
+					},
+				},
+				{
+					Name: metric.KubePersistentVolumeCapacityBytes,
+					Labels: map[string]string{
+						source.UIDLabel:          "uuid-aws",
+						source.PVLabel:           "pv-aws",
+						source.StorageClassLabel: "gp2",
+						source.ProviderIDLabel:   "vol-0fc54c5e83b8d2b76",
+					},
+					Value:          8192,
+					AdditionalInfo: nil,
+				},
+			},
+		},
+		{
+			// GCE PD in-tree source: provider ID is the PD name, no csi label.
+			name: "gce pd non-csi pv omits csi_volume_handle label",
+			scrapes: []scrape{
+				{
+					PVs: []*clustercache.PersistentVolume{
+						{
+							Name: "pv-gce",
+							UID:  "uuid-gce",
+							Spec: v1.PersistentVolumeSpec{
+								StorageClassName: "standard",
+								PersistentVolumeSource: v1.PersistentVolumeSource{
+									GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+										PDName: "gke-pvc-abc123",
+									},
+								},
+							},
+						},
+					},
+					Timestamp: start1,
+				},
+			},
+			expected: []metric.Update{
+				{
+					Name: metric.KubecostPVInfo,
+					Labels: map[string]string{
+						source.UIDLabel:          "uuid-gce",
+						source.PVLabel:           "pv-gce",
+						source.StorageClassLabel: "standard",
+						source.ProviderIDLabel:   "gke-pvc-abc123",
+					},
+					Value: 0,
+					AdditionalInfo: map[string]string{
+						source.UIDLabel:          "uuid-gce",
+						source.PVLabel:           "pv-gce",
+						source.StorageClassLabel: "standard",
+						source.ProviderIDLabel:   "gke-pvc-abc123",
+					},
+				},
+			},
+		},
+		{
+			// CSI source present but VolumeHandle empty: csi_volume_handle label
+			// is omitted, and provider_id currently ends up empty (documents the
+			// gap where the old code fell back to pv.Name).
+			name: "csi pv with empty volume handle omits csi label and has empty provider id",
+			scrapes: []scrape{
+				{
+					PVs: []*clustercache.PersistentVolume{
+						{
+							Name: "pv-csi-empty",
+							UID:  "uuid-csi-empty",
+							Spec: v1.PersistentVolumeSpec{
+								StorageClassName: "gp3",
+								PersistentVolumeSource: v1.PersistentVolumeSource{
+									CSI: &v1.CSIPersistentVolumeSource{
+										VolumeHandle: "",
+									},
+								},
+							},
+						},
+					},
+					Timestamp: start1,
+				},
+			},
+			expected: []metric.Update{
+				{
+					Name: metric.KubecostPVInfo,
+					Labels: map[string]string{
+						source.UIDLabel:          "uuid-csi-empty",
+						source.PVLabel:           "pv-csi-empty",
+						source.StorageClassLabel: "gp3",
+						source.ProviderIDLabel:   "",
+					},
+					Value: 0,
+					AdditionalInfo: map[string]string{
+						source.UIDLabel:          "uuid-csi-empty",
+						source.PVLabel:           "pv-csi-empty",
+						source.StorageClassLabel: "gp3",
+						source.ProviderIDLabel:   "",
+					},
+				},
+			},
+		},
+		{
+			// No recognized volume source: provider ID falls back to pv.Name.
+			name: "pv with no known volume source falls back to name",
+			scrapes: []scrape{
+				{
+					PVs: []*clustercache.PersistentVolume{
+						{
+							Name: "pv-nfs",
+							UID:  "uuid-nfs",
+							Spec: v1.PersistentVolumeSpec{
+								StorageClassName: "nfs",
+							},
+						},
+					},
+					Timestamp: start1,
+				},
+			},
+			expected: []metric.Update{
+				{
+					Name: metric.KubecostPVInfo,
+					Labels: map[string]string{
+						source.UIDLabel:          "uuid-nfs",
+						source.PVLabel:           "pv-nfs",
+						source.StorageClassLabel: "nfs",
+						source.ProviderIDLabel:   "pv-nfs",
+					},
+					Value: 0,
+					AdditionalInfo: map[string]string{
+						source.UIDLabel:          "uuid-nfs",
+						source.PVLabel:           "pv-nfs",
+						source.StorageClassLabel: "nfs",
+						source.ProviderIDLabel:   "pv-nfs",
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1362,7 +1538,7 @@ func Test_kubernetesScraper_scrapePVs(t *testing.T) {
 			}
 
 			if len(scrapeResults) != len(tt.expected) {
-				t.Errorf("Expected result length of %d, got %d", len(tt.expected), len(scrapeResults))
+				t.Fatalf("Expected result length of %d, got %d: %+v", len(tt.expected), len(scrapeResults), scrapeResults)
 			}
 
 			for i, expected := range tt.expected {
@@ -1370,6 +1546,117 @@ func Test_kubernetesScraper_scrapePVs(t *testing.T) {
 				if !reflect.DeepEqual(expected, got) {
 					t.Errorf("Result did not match expected at index %d: got %v, want %v", i, got, expected)
 				}
+				// csi_volume_handle must only be present for CSI volumes with a
+				// non-empty handle.
+				if _, ok := got.Labels[source.CSIVolumeHandleLabel]; ok {
+					if got.Labels[source.CSIVolumeHandleLabel] == "" {
+						t.Errorf("index %d: csi_volume_handle label present but empty", i)
+					}
+				}
+			}
+		})
+	}
+}
+
+func TestToResourceUnitValue(t *testing.T) {
+	tests := []struct {
+		name         string
+		resourceName v1.ResourceName
+		quantity     resource.Quantity
+		wantResource string
+		wantUnit     string
+		wantValue    float64
+	}{
+		{
+			name:         "cpu is reported in cores",
+			resourceName: v1.ResourceCPU,
+			quantity:     resource.MustParse("500m"),
+			wantResource: "cpu",
+			wantUnit:     "core",
+			wantValue:    0.5,
+		},
+		{
+			name:         "memory is reported in bytes",
+			resourceName: v1.ResourceMemory,
+			quantity:     resource.MustParse("1Ki"),
+			wantResource: "memory",
+			wantUnit:     "byte",
+			wantValue:    1024,
+		},
+		{
+			name:         "storage is reported in bytes",
+			resourceName: v1.ResourceStorage,
+			quantity:     resource.MustParse("2Ki"),
+			wantResource: "storage",
+			wantUnit:     "byte",
+			wantValue:    2048,
+		},
+		{
+			name:         "ephemeral storage is reported in bytes",
+			resourceName: v1.ResourceEphemeralStorage,
+			quantity:     resource.MustParse("3Ki"),
+			wantResource: "ephemeral-storage",
+			wantUnit:     "byte",
+			wantValue:    3072,
+		},
+		{
+			name:         "pods are reported as integers",
+			resourceName: v1.ResourcePods,
+			quantity:     resource.MustParse("10"),
+			wantResource: "pods",
+			wantUnit:     "integer",
+			wantValue:    10,
+		},
+		{
+			// Regression guard: the resource name is no longer sanitized, so the
+			// hyphen and case are preserved verbatim ("hugepages-2Mi", not
+			// "hugepages_2Mi").
+			name:         "huge pages keep their raw name and are bytes",
+			resourceName: v1.ResourceName(v1.ResourceHugePagesPrefix + "2Mi"),
+			quantity:     resource.MustParse("4Ki"),
+			wantResource: "hugepages-2Mi",
+			wantUnit:     "byte",
+			wantValue:    4096,
+		},
+		{
+			// Regression guard: extended resource names keep '.' and '/'
+			// ("nvidia.com/gpu", not "nvidia_com_gpu").
+			name:         "extended resource keeps dotted slashed name and is integer",
+			resourceName: v1.ResourceName("nvidia.com/gpu"),
+			quantity:     resource.MustParse("2"),
+			wantResource: "nvidia.com/gpu",
+			wantUnit:     "integer",
+			wantValue:    2,
+		},
+		{
+			name:         "attachable volume resource keeps raw name and is bytes",
+			resourceName: v1.ResourceName(v1.ResourceAttachableVolumesPrefix + "aws-ebs"),
+			quantity:     resource.MustParse("5"),
+			wantResource: "attachable-volumes-aws-ebs",
+			wantUnit:     "byte",
+			wantValue:    5,
+		},
+		{
+			name:         "unrecognized native resource returns empty",
+			resourceName: v1.ResourceName("kubernetes.io/somethingelse"),
+			quantity:     resource.MustParse("1"),
+			wantResource: "",
+			wantUnit:     "",
+			wantValue:    0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotResource, gotUnit, gotValue := toResourceUnitValue(tt.resourceName, tt.quantity)
+			if gotResource != tt.wantResource {
+				t.Errorf("resource = %q, want %q", gotResource, tt.wantResource)
+			}
+			if gotUnit != tt.wantUnit {
+				t.Errorf("unit = %q, want %q", gotUnit, tt.wantUnit)
+			}
+			if gotValue != tt.wantValue {
+				t.Errorf("value = %v, want %v", gotValue, tt.wantValue)
 			}
 		})
 	}

pkg/metrics/kubemetrics.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/opencost/opencost/core/pkg/clustercache"
 	"github.com/opencost/opencost/core/pkg/clusters"
-	"github.com/opencost/opencost/core/pkg/util/promutil"
 
 	"github.com/prometheus/client_golang/prometheus"
 	batchv1 "k8s.io/api/batch/v1"
@@ -190,7 +189,7 @@ func getPersistentVolumeClaimClass(claim *clustercache.PersistentVolumeClaim) st
 // toResourceUnitValue accepts a resource name and quantity and returns the sanitized resource, the unit, and the value in the units.
 // Returns an empty string for resource and unit if there was a failure.
 func toResourceUnitValue(resourceName v1.ResourceName, quantity resource.Quantity) (resource string, unit string, value float64) {
-	resource = promutil.SanitizeLabelName(string(resourceName))
+	resource = resourceName.String()
 
 	switch resourceName {
 	case v1.ResourceCPU:

pkg/metrics/pvmetrics.go

@@ -69,11 +69,9 @@ func (kpvcb KubePVCollector) Collect(ch chan<- prometheus.Metric) {
 
 		if _, disabled := disabledMetrics["kubecost_pv_info"]; !disabled {
 			storageClass := pv.Spec.StorageClassName
-			providerID := pv.Name
+			providerID := clustercache.GetPVProviderID(pv)
 			var csiVolumeHandle string
-			// if a more accurate provider ID is available, use that
-			if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeHandle != "" {
-				providerID = pv.Spec.CSI.VolumeHandle
+			if pv.Spec.CSI != nil {
 				csiVolumeHandle = pv.Spec.CSI.VolumeHandle
 			}
 			m := newKubecostPVInfoMetric("kubecost_pv_info", pv.Name, pvUID, storageClass, providerID, csiVolumeHandle, float64(1))