Removing temporary annotations and labels from pods, pvcs, and pvs

[pranavgaikwad]

Addresses #51.

[pranavgaikwad]

@dymurray Right now I am not deleting annotations used by Restic (list of volumes), do you think we should delete those as well ?

[sseago]

Other than the minor concern about the migrate-type annotation name, it all looks fine to me.

[sseago]

This is the same annotation that we use as a permanent annotation on the Backup resource. We should probably use a different one just to make it clear that they are serving a different function, although from a code point of view, they won't clash since they're applied to different resource types.

[sseago]

A more general comment/question @dymurray -- I know I'd asked in the past whether it was reasonable to leave the other annotations in place post-restore. For the most part, none of the annotations we're adding in the plugin are relevant post-restore. I'm thinking in terms of the common plugin's annotations telling us what the registry hostname and cluster version were on both source and dest cluster. Right now these are preserved in the post-restore objects. While they're useful for debugging, they're not relevant post-restore. Will users want to know which resources are left over from migration? maybe, or maybe not. These pv annotations are in a similar category. Do we really need/want to remove these? Related -- if we do want to remove them, do we want to do the same for the other annotations we're talking about?

[dymurray]

@sseago Ideally we should remove all of these annotations. You are correct that they aren't needed/used post-restore, so they also won't really break anything. We might down the line want some ability to highlight all migrated resources so I could also see a use case for keeping them.

The motivation here was that we want the resources on the dest cluser to match the source cluster as much as possible. It seems desirable that we would like to remove anything that our controller is adding manually. Do you think we shouldn't be removing these until we decide if we want the ability to highlight migrated resources?

[sseago]

I'm not really sure. We may want an option to keep them for debug purposes -- maybe a velero or controller setting to control this? Or would it be sufficient for debugging to just disable this functionality in a custom build of the plugin? So these temporary annotations are just on pods, pvs, and pvcs. The common plugin adds four different annotations to every resource it touches. Removing that will be slightly trickier since if we remove it from the to-be-restored object, we need to make sure that any restore plugin that is using these annotations is pulling them from the unmodified resource (the one that contains the status field) rather than the under-modification resource.

[sseago]

Oh, wait, I just realized that what I proposed above for the common plugin annotations would only work for backup annotations. Restore annotations only go into the resource being restored, not the unmodified one. So common annotation cleanup would have to be registered as a separate restore plugin registered to run last -- i.e. register it as "openshift.io/99-common-restore-cleanup-plugin".

[dymurray]

Oh good point about that... so really this cannot go in this plugin correct? This would break all restore functionality depending on this plugin.

[dymurray]

Scott is right, we should only be removing these annotations/labels after all plugins have been run. This logic will need to go in a separate plugin that runs last.