Consider adding UID to klog.ObjectRef

[tallclair]

Is there a reason UID isn't included in klog.ObjectRef?

In the Kubelet, we're seeing a mix of using klog.KObj(pod) and format.Pod(pod). Currently, the latter is preferred as it includes UID, which is often necessary for debugging Kubelet operations, but it would be nice if we could deprecate it in favor of a more standardized approach (KObj), which is contingent on UID being included.

[dgrisonnet]

/triage accepted

cc @pohly

[pohly]

I wasn't involved when ObjectRef was initially introduced (#128, https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/20191115-structured-logging.md#helper-functions-for-log-format).

@serathius: do you remember whether the inclusion of UID was considered and why it was excluded?

Adding the UID to ObjectRef with omitempty would certainly be doable now. The bigger question is whether klog.KObj and friends should populate that field. I agree that it can be necessary to be precise, but it could also be seen as additional log noise by others.

[serathius]

For the klog.KObj function there were multiple additional fields that were proposed (UID, apiGroup, etc) , in the end we went with simplest and most common reference namespace/name assuming we can expand in the future.

If UID is important for Kubelet codebase, maybe we can come with option like klog.KObj(pod, klog.WithUID()) and make it a required for its subdirectories.

[tallclair]

I'm fine with making UID optional, but have a slight preference for a separate method rather than an option (which seems verbose for something used so frequently). Although, I suppose we could always just wrap it in a helper.

[pohly]

How should the UID be printed as string? <namespace>/<name>/<uid>?

I also prefer separate methods. forbidigo then can be used to restrict usage of the less informative klog.KObj in kubelet code.

klog.KObjUID and klog.KObjUIDSlice?

[tallclair]

How should the UID be printed as string? <namespace>/<name>/<uid>?

I vote <namespace>/<name>(<UID>). It's more consistent with the utility we currently use in kubelet (https://github.com/kubernetes/kubernetes/blob/9da4b7ecfb60ea484f8e6c89c42e63506571bc68/pkg/kubelet/util/format/pod.go#L40)

[pohly]

I'm fine with making UID optional, but have a slight preference for a separate method rather than an option (which seems verbose for something used so frequently).

We cannot extend klog.ObjectRef because that would break klog.ObjectRef{"name", "namespace"}. But we can add klog.ObjectInstance as new type and klog.KObjUID as constructor.

I would not add a function for construction.

// KRef returns ObjectRef from name and namespace
func KRef(namespace, name string) ObjectRef {
        return ObjectRef{
                Name:      name,
                Namespace: namespace,
        }
}

This does not add any value over ObjectRef{name, namespace} except for saving a few characters. Worse, why was the order of strings reversed?! Too late now, that ship has sailed.

[tallclair]

I started looking into implementing this, and realized a problem: it would be nice to provide a func KInst(obj KMetadata) ObjectInstance function, but that requires adding a GetUID() types.UID method to the KMetadata interface, which requires importing k8s.io/apimachinery/pkg/types, which I think is a no-go.

So, if we want that convenience function, I think it needs to live in k8s.io/apimachinery, at which point maybe we should just put the whole ObjectInstance struct there.

[pohly]

Darn, the UID typedef. Yes, that's a problem.

There is also NamespacedName in staging/src/k8s.io/apimachinery/pkg/types/namespacedname.go which is a bit under-utilized in Kubernetes. Instead we prefer to pass either name, namespace string or namespace, name string (yes, both orders occur, sometimes even in the same package!).

[pohly]

@tallclair: we could invert the dependencies:

• in klog, define typedef UID string
• in k8s.io/apimachinery/pkg/types, make it an alias: type UID = klog.UID

k8s.io/apimachinery already depends on klog, so this won't make a difference for consumers.