at quick glance, I'm not 100% sure about the correctness of these lines.
but actually, even the original code:

	if len(newName) > 63 {
			newName = newName[:63]

makes me doubtful. Do we really want to truncate the name at 64 chars?
wouldn't it lead to unexpected clashes when using similar-but-ending-differently long names?

Raising an error could be saner (and thus making the AppWrapper impossible to process), it's a common K8s thing to abort on long names.

I have tried in the past to concatenate the namespace and name of a job and observed that this would indeed run long for many of our jobs. The use of the hyphen is also problematic because it is ambiguous. We should have separate labels for the name and namespace.

Thank you @kpouget, you raised a very good point. For what I could read, it seems that it is done to meet with DNS standards RFC 1035/1123. Since Kubernetes namespaces and names are often used to form a DNS subdomain, K8s enforces it here.

After looking at this code in particular again, I can confirm that we can keep the original code here as the namespace is used in the creation of the object anyways.

Now, to deal with very long names, we could raise an error and stop the operation as suggested. But I was thinking what if we handle the error this way? Here are some ideas:

• Hashing the name for uniqueness?
• Or, Truncate to 60 characters and append i.e., 3 random numbers?

What do you think?

Thanks @tardieu, having separate labels for the name and namespace does look like the best option instead of using a hyphen.

Now, to deal with very long names, we could raise an error and stop the operation as suggested. But I was thinking what if we handle the error this way?
do you know how this DNS name is used? is it something that the AppWrapper owner will ever want to use?
if it's purely internal, then using a hash, or truncating the name + random chars seems to be a good idea

I don't why I didn't think of that, thanks @astefanutti :)

Can that label be removed altogether? It'll avoid those cluster-wide list queries, and be fully deterministic. Because even with that fix, you could imagine case when labels clash.

For normal cleanup of eg an appwrapper containing a pytorchjob, we should only delete the pytorch job and let the training operator do the rest. But we are also exploring adding "forceful" cleanup (kill -9) as a last resort where we delete (forcefully) any resource that can be traced back to an appwrapper. For the latter at least, it makes sense to select on labels.

That would assume the underlying operators propagate the label, right? It seems owner references and cascading deletion would be a more robust and generally supported mechanism. Granted it would restrict the generic items to be within the same namespace as that of the AppWrapper (cross namespace ownership is not allowed), but even that may possibly be seen as a good restriction.

This indeed assumes that wrapped resources in the appwrapper are properly labelled and that labels are propagated by operators for resources indirectly created. Again, this is only a last resort, but an important last resort. This definitely does not preclude trying to do the right thing first. Ownership references are not an option in general due to the cross namespace restriction. Transitive ownership and cascading deletion is good practice but there is no guarantee operators follow these practices.

For more context, we do experience node failures scenarios where the kube scheduler hence typically operators fail to terminate pods. The pods remain in terminating state until forcefully deleted (irrespective of grace periods), hence the need to orchestrate forceful deletion in MCAD.

To be clear, even for forceful deletion, I am not advocating for relying solely on labels, but labels are one of the tools we want to use to scope the forceful deletion.

I think this will almost always fail for length >60 because the string you create is not deterministic. You should use a hash for getting the last 3 digits

Here too