clean up raycluster instances at chart uninstall

[akihikokuroda]

Summary

fix: #695

Details and comments

Add pre-delete hook that deletes all remaining raycluster instances at helm uninstall.

[Tansito]

This is quite interesting. Do we want to remove all ray-clusters in pre-install? Thinking that experiments usually takes a lot of time and we will find situations where we should not delete a cluster from a client. Am I missing something?

[akihikokuroda]

We install and delete the Ray operator that creates and deletes that ray cluster based on the raycluster CRD instances. We don't allow the others to manipulate the instances in the namespace. So I think that it's safe and good to delete all in the namespace.

[Tansito]

But for example, if a user is running an experiment and we make a helm upgrade in our cluster does that mean that we are going to remove all the clusters included the one that is running the experiment?

[akihikokuroda]

The helm has different hooks for the upgrade. The pre-delete hook is executed only at uninstall.

[psschwei]

@Tansito @akihikokuroda -- where did we land on merging this?

[akihikokuroda]

So far this doesn't get consensus so I can close or I can update to delete the instances only has the label.

[psschwei]

I recall running into issues trying to uninstall and reinstall the Helm chart (for local dev, I usually just kill the cluster and start over if I need to redeploy), so I'd be interested in seeing it merge.

[akihikokuroda]

I'll work to change only remove the labeled instance. @Tansito What do you think?

[psschwei]

I'll work to change only remove the labeled instance. @Tansito What do you think?

My preference would be to just delete everything -- if we're uninstalling the Helm chart, I would think we want to clean up everything that's out there and not leave any orphans hanging. And if the pre-delete hook only gets called if we uninstall (and not when we upgrade), then I think we should be ok.

[akihikokuroda]

@psschwei OK. The helm chart install / uninstall the CRD so I agree it's OK to delete everything at uninstall. I'll make the change.

[psschwei]

LGTM

/assign @Tansito

[Tansito]

I'm back! As soon as I finish to read everything I will start reviewing this, thanks @akihikokuroda and @psschwei as always ❤️

[Tansito]

@akihikokuroda pffff sorry, I left some comments pending a lot of days ago but I didn't submit them (I don't know what happened 🤷‍♂️ )

[psschwei]

LGTM

[Tansito]

Now it's fine for me! Sorry for the confusion 😂

[akihikokuroda]

@Tansito NP. Thanks!