[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kimwnasptd for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

We need to consider what will happen to existing TensorBoard instances when this update is applied to the cluster.

We should test what happens, it looks like we are setting the ownerReferences of the Service/VirtualService to the TensorBoard CRD instance, so at worst, we can tell people to delete and recreate all their TensorBoard CRD instances:

• https://github.com/kubeflow/kubeflow/blob/master/components/tensorboard-controller/controllers/tensorboard_controller.go#L114

@thesuperzapper Could we re-run the tests it seems like the unit test of notebook-controller fails on some dependency, and is unrelated to the changes in the PR. 🙏🏻

Also, the integration tests and multi-arch build for the same pass. 😄

@mishraprafful can you please check what happens to the existing resources when you upgrade from the old version of the controller to the new version (specifically, does it remove the old Service/{notebook_name}, or leave it dangling).

If it leaves it, we should consider having the controller look for Services / VirtualSerivces which have an owner reference of a Notebook, but are not named with the notebook-XXX or tensorboard-XXX prefix.

@mishraprafful There are failling tests. Can you take a look?

Sure @rimolive Looking into it.

Also, there was a historical PR to fix this issue, which also accounted for the case that the name is too long #6701

They put 63 characters, but I am not sure if VirtualServices/Services can actually be longer than that?

@rimolive the tests are failing to pull some dependencies, the tests pass when I try them locally. Maybe we could re-trigger the tests as this seems like an intermediate issue.

Feel free to point out if I'm missing something 🙌🏻

@thesuperzapper Service name has to be less than 64

The name of a Service object must be a valid RFC 1035 label name.

ref

Just an update: now that #7589 is merged, I'm testing a last few things, should add some more commits soon to finish the functionality.

@thesuperzapper @rimolive @kimwnasptd All issues addressed, tests passing, PR description updated. 🚀

@mishraprafful I am going to push this to 1.9.1, because we came across a better way to do this while building Notebooks 2.0.

The key idea is to use the generateName field of every Kubernetes object, rather than using the name directly, which is how Kubernetes creates Pod names from deployments with those {DEPLOYMENT_NAME}-{POD_HASH}-{RANDOM_SUFFIX} names.

Obviously, this means we need to account for random names (which is also great for backwards compatibly).

Rather than assuming the Service/VirtualService/ReplicaSet has the same name as the Notebook (or notebook-{NOTEBOOK_NAME}), we should instead loop over the corresponding resource type in the target namespace and find the ones which have metadata.ownerReferences set to the Notebook being reconciled.

If there are no such resources, we can create a new one using the generateName set to nb-{NOTEBOOK_NAME}- (with the owner reference already set to avoid problems if the controller crashes).

We will need to be careful about the Service names, because obviously the VirtualService needs to point to the correct Service name, but this can simply be handled by ordering the reconciliation correctly, so we know the Service name before reconciling the VirtualService.

@thesuperzapper Apologies for the huge delay (life happened 😉 ), but I've updated the PR according to your last comment now.

The key idea is to use the generateName field of every Kubernetes object, rather than using the name directly, which is how Kubernetes creates Pod names from deployments with those {DEPLOYMENT_NAME}-{POD_HASH}-{RANDOM_SUFFIX} names.

Thought that the default advice is to use deterministic names and take advantage of these subsequently, as per https://github.com/kubernetes-sigs/controller-runtime/blob/main/FAQ.md#q-my-cache-might-be-stale-if-i-read-from-a-cache-how-should-i-deal-with-that.

Using generateName means extra work, as per that FAQ

In the few cases when you cannot take advantage of deterministic names (e.g. when using generateName), it may be useful in to track which actions you took, and assume that they need to be repeated if they don't occur after a given time (e.g. using a requeue result). This is what the ReplicaSet controller does.

After a quick look, seems to me Indexer is not used, as is in Notebooks 2.0

	// setup field indexers on the manager cache. we use these indexes to efficiently
	// query the cache for things like which Workspaces are using a particular WorkspaceKind
	if err := helper.SetupManagerFieldIndexers(mgr); err != nil {
		setupLog.Error(err, "unable to setup field indexers")
		os.Exit(1)
	}

https://github.com/kubeflow/notebooks/blob/9d02acfdd707282385221590bc953f892099a94c/workspaces/controller/cmd/main.go#L129-L134