k8s: k8s.persistent_volume: new resource, assign CSI attrs #1499
Conversation
model/container/resources.yaml
Outdated
| - ref: container.csi.plugin.name | ||
| - ref: container.csi.volume.id |
There was a problem hiding this comment.
quick question: could these change during container lifetime?
There was a problem hiding this comment.
I suppose that would depend on the container runtime. I could see something like hot-plugging for container volumes, but I'm not aware of anything that does that.
Is that a heuristic you're using to decide if this is a resource attribute?
There was a problem hiding this comment.
yes, we currently have a problem with mutable resource attributes - there is no way to update them once they are set when otel is configured, see #1181 (comment) for the details.
Having said this, it seems in (at least) most of the cases, it's not possible, so we can add them as resource attributes with a following clause
| - ref: container.csi.plugin.name | |
| - ref: container.csi.volume.id | |
| - ref: container.csi.plugin.name | |
| requirement_level: | |
| recommended: if and only if container runtime does not support changing CSI plugin during container lifetime. The attribute MUST NOT be set if its value may change during OpenTelemetry SDK instance lifetime. | |
| - ref: container.csi.volume.id | |
| requirement_level: | |
| recommended: if and only if container runtime does not support changing CSI plugin during container lifetime. The attribute MUST NOT be set if its value may change during OpenTelemetry SDK instance lifetime. |
unless @jsuereth has any problem with it
There was a problem hiding this comment.
I'm not sure if this attribute has to do with the container, I think this is an attribute of the Persistent Volume based on the implementation at https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32055/files#diff-ff719bd25c1ddf9644468b5e873d43ad7a8e95d8ff5a59b685cf4b6ecb6c1f77R54-R115?
@open-telemetry/semconv-k8s-approvers any thoughts on this?
There was a problem hiding this comment.
I'm not sure if this attribute has to do with the container
I came to say the same thing. It's not a container resource attribute. One clear tell is it's not 1:1 to a container, rather 1:N.
It seems there's no k8s.volume section, so I think that'll have to go in first. I assume this can map to core Volume v1?
There was a problem hiding this comment.
I assume this can map to core Volume v1?
I'm not sure about how we should model this. It appears that csi can be an attribute of more than 1 K8s resource types. So the question is which one we should start with for that specific use case.
I see that csi also appears in the PersistentVolumeSpec which seems to reflect what we do in the implementation?
@jinja2 any comments here?
There was a problem hiding this comment.
A k8s Volume can have a CSIVolumeSource. This has fields for driver, but not handle (i.e. container.csi.volume.id). So for Volume, it seems only plugin.name would be valid as an optional attribute.
A PersistentVolume can have CSIPersistentVolumeSource, which has driver (container.csi.plugin.name) and volumeHandle (container.csi.volume.id), so I think this would have both attrs as optional.
I think I was proposing that we just collapse these into one k8s.volume representation, marking them both optional, but that does mean the non-persistent-volumes contain a schema with an attribute that isn't possible. I feel this is a slightly more desirable tradeoff as opposed to having another bag of attrs under say k8s.persistent_volume, with the thinking that the end-user doesn't care about the distinction here.
There was a problem hiding this comment.
I think I was proposing that we just collapse these into one k8s.volume representation, marking them both optional, but that does mean the non-persistent-volumes contain a schema with an attribute that isn't possible.
I find this a bit weird tbh specially when it will be part of the schema. I think the schema definition should be precise. Could we only define exactly what we are going to use in the implementation for now?
There was a problem hiding this comment.
I think the schema definition should be precise. Could we only define exactly what we are going to use in the implementation for now?
I think I crossed some wires with:
I think I was proposing that we just collapse these into one k8s.volume representation
We don't really need that as this categorization isn't exactly showing up anywhere the user interacts, so perhaps just calling this k8s.persistent_volume is what we're after? I did just that now.
There was a problem hiding this comment.
If this makes sense, perhaps we could go ahead and start k8s.volume as well, but just ref the plugin.name attr as volume.id isn't present? I don't need it for the impl, but given the context/review cycle it might be easier to establish these here and now?
gdvalle
changed the title
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
not stale |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
gdvalle
changed the title
There was a problem hiding this comment.
I think the reason we are stuck here is that there is no clear way right now to link metrics to resources in the spec: #1649 (comment)
The volume metrics that we'd want to define are listed in #1485. It would help if we start with defining them first in SemConv since the Collector's PR actually tries to add (resource) attributes to them when they are not yet defined in SemConv. This complicates things.
In addition we have already defined k8s.volume.name and k8s.volume.type in the registry but not yet as part of a resource.
Following the Collector's code flow I still think that the csi.* attributes look to be related to the Persistent Volume resource primarily. We first get the PVC, then we get the PV that it references and then out of this PV we get the csi.* info.
In general this touches the following open questions/problems:
- Figure out how volume, persistent_volume etc are related as resources.
- Figure out in which resource(s)
container.csi.*attributes should become members. - Figure out how volume metrics are related to the resources mentioned above.
ps: I would be fine if we proceed with the persistent volume definition in case @open-telemetry/specs-semconv-maintainers and @open-telemetry/semconv-k8s-approvers agree, I'm just raising my concerns here since we are kind of moving in uncharted waters here.
| name: k8s.persistent_volume | ||
| brief: > | ||
| A Kubernetes Persistent Volume object. | ||
| attributes: |
There was a problem hiding this comment.
I guess this would at least need to define the k8s.persistent_volume.name and k8s.persistent_volume.uid attributes.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This follows up from #1337, and adds the CSI attributes as resource attributes for kubernetes volumes for use in open-telemetry/opentelemetry-collector-contrib#32055.
xref https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32055/files#r1805969619