KIC: Cleanup of old named resources

[oshoval]

What this PR does / why we need it:
Since Kubevirt ipam controller resource names were renamed [1]
since 0.94.0, we introduce the special remove that in case the old named
objects exist, will remove them.
This allows the lifecycle lane to pass, as it checks for leftovers,
which otherwise exists.

[1] #1816

Special notes for your reviewer:

Release note:

None

[oshoval]

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1819/pull-cluster-network-addons-operator-unit-test/1808027843397947392

seems it fails due to something like this
operator-framework/operator-registry#178 (comment)

so we would need to use deselector
or to remove the replacement field as the link above says (trying this first, it works fwiw)
EDIT - we can also instead removing the field, point to previous release

[oshoval]

/cc @RamLavi
ptal

[RamLavi]

Not sure what you mean by "Delete release 0.94.0"
It's a version, U/S user may already consume it.
Can we just remove support from it? it breaks the entire lifecycle

[oshoval]

it is very new and not released beside on upstream main
i think it is safe to delete it, we did that in the past

if we don't delete it lifecycle fails #1818 (comment)

since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above)
the alternative is to add it to deselector
imo it is better and safe to delete

[RamLavi]

it is very new and not released beside on upstream main i think it is safe to delete it, we did that in the past

if we don't delete it lifecycle fails #1818 (comment)

since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above) the alternative is to add it to deselector

It is not permitted to remove a supported release. Once it is tagged - you can't go back.
From what I see - CNAO API wasn't changed - am I correct?
Regarding the now "old" manifests - CNAO needs to handle them, IIRC we have a special place for that. I suggest we go towards that approach
/hold

[oshoval]

it is very new and not released beside on upstream main i think it is safe to delete it, we did that in the past
if we don't delete it lifecycle fails #1818 (comment)
since the kubevirt ipam controller component name was changed it leaves leftovers (details in the link above) the alternative is to add it to deselector

It is not permitted to remove a supported release. Once it is tagged - you can't go back. From what I see - CNAO API wasn't changed - am I correct? Regarding the now "old" manifests - CNAO needs to handle them, IIRC we have a special place for that. I suggest we go towards that approach /hold

CNAO API wasn't change, correct.

This is a release on main branch which is unstable
Having a function like cleanUpMultusOldName will need to delete a list of resources (as they all were changed)
I think it is a total overkill in this case.

lets at least do the deselect and say remove support in release notes?
if any user uses 0.94.0 we can just request to clean and reinstall one time
again it is a release on main branch, unstable, and the probability someone already uses it and will have problems is rare
(at most there is just release notes)
Dragging logic like cleanUpMultusOldName which will be here even more complex is wrong imho
in this case.

Anyhow if you insist i will just do it, honestly it is overkill imho

[RamLavi]

lets at least do the deselect and say remove support in release notes?

I disagree. Every release should be supported lifecycle-wize. main branch is theoretically unstable but it doesn't mean we get to break the lifecycle. This is precisely the reason the lane is there for.
I am not convinced that this case justify breaking the lane.

[oshoval]

lets at least do the deselect and say remove support in release notes?

I disagree. Every release should be supported lifecycle-wize. main branch is theoretically unstable but it doesn't mean we get to break the lifecycle. This is precisely the reason the lane is there for. I am not convinced that this case justify breaking the lane.

we even didn't even finish delivering the feature, the release was created only so HCO can consume
and we renamed the repo as soon as possible, which caused this.
adding this removal function in so early stage consumes focus and reduce simplification imho
(only one release is dropped)
being robust to changes here is the right thing imho
unstable branch should not be committed the the same factors as stable ones

anyhow as mentioned above, will just do that

[oshoval]

Done (added special remove), lets see it pass (didn't try locally)
maybe this new commit can be added as a prior PR to the release

[oshoval]

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1819/pull-e2e-cluster-network-addons-operator-lifecycle-k8s/1808100330181758976
passed with the special clean-up changes

will arrange the PR

/hold cancel

[oshoval]

@RamLavi ptal

if needed i can split this PR to 2 different PRs
one is the fix and then comes the release
i think it should work as well

[oshoval]

split the commits, this PR will be just the cleanup
and then we would able to take #1818
(both PRs were tested together already)

[oshoval]

workflow fix
#1820

[RamLavi]

@RamLavi ptal

if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

[oshoval]

@RamLavi ptal
if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

the other way around
the renaming was already merged, so this is the cleanup
and then a new release can be introduced
it is better that new releases are PRs which are automated once possible
the PR can be standalone, it is not that they depend and need to be bundled
they just both need to be merged
the fact the logic is there doesnt check what release are we on, because the renaming is already included
anyhow it is really nits imho

[RamLavi]

@RamLavi ptal
if needed i can split this PR to 2 different PRs one is the fix and then comes the release i think it should work as well

why do we need to split? this commit has no place but after a new release was created. Am I missing something?

the other way around the renaming was already merged, so this is the cleanup and then a new release can be introduced it is better that new releases are PRs which are automated once possible the PR can be standalone, it is not that they depend and need to be bundled they just both need to be merged the fact the logic is there doesnt check what release are we on, because the renaming is already included anyhow it is really nits imho

oh I see then.

[oshoval]

/override pull-e2e-cluster-network-addons-operator-workflow-k8s

[kubevirt-bot]

@oshoval: Overrode contexts on behalf of oshoval: pull-e2e-cluster-network-addons-operator-workflow-k8s

In response to this:

/override pull-e2e-cluster-network-addons-operator-workflow-k8s

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[RamLavi]

code looks good! just needs to move to the right location

[RamLavi]

missed something: please add the PR that renamed the objects on the commit desc.

[oshoval]

done

[RamLavi]

/lgtm
/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [RamLavi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[oshoval]

btw all those cleanups (not just this one)
becomes outdated at some stage and can be removed
it also cause API calls, maybe we can consider a boolean that once the flow saw that nothing should be removed (but conf.ipam == true), or all that was to removed, removed successfully, short circuit the function
unless those API calls arent expansive

alternative is to not scan for this type of leftovers (but i guess it is less nice solution)

[RamLavi]

btw all those cleanups (not just this one) becomes outdated at some stage and can be removed it also cause API calls, maybe we can consider a boolean that once the flow saw that nothing should be removed (but conf.ipam == true), or all that was to removed, removed successfully, short circuit the function unless those API calls arent expansive

alternative is to not scan for this type of leftovers (but i guess it is less nice solution)

The same logic needs to apply like the nmstate tests - if it should no longer be supported then we can remove the special clean.