Remove old api version from the SSP CRD on upgrade #3329
Conversation
kubevirt-bot
added
the
release-note-none
kubevirt-bot
added
the
dco-signoff: yes
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
2551548 to
de7f9e4
Compare
| sspCrd := "ssps.ssp.kubevirt.io" | ||
| sspApiVersionToRemove := "v1beta1" |
| if !slices.Contains(crd.Status.StoredVersions, removedApiVersion) { | ||
| return nil | ||
| } | ||
| var newStoredVersions []string | ||
| for _, ver := range crd.Status.StoredVersions { | ||
| if ver == removedApiVersion { | ||
| continue | ||
| } | ||
| newStoredVersions = append(newStoredVersions, ver) | ||
| } |
There was a problem hiding this comment.
I think we can use methods from the slices package, instead of this loop.; e.g.
| if !slices.Contains(crd.Status.StoredVersions, removedApiVersion) { | |
| return nil | |
| } | |
| var newStoredVersions []string | |
| for _, ver := range crd.Status.StoredVersions { | |
| if ver == removedApiVersion { | |
| continue | |
| } | |
| newStoredVersions = append(newStoredVersions, ver) | |
| } | |
| i := slices.Index(crd.Status.StoredVersions, removedApiVersion) | |
| if i == -1 { | |
| return nil | |
| } | |
| slices.Delete(crd.Status.StoredVersions, i, i+1) |
I think it's cleaner. WDYT?
There was a problem hiding this comment.
But actually, I think we should try to use json patch here. So maybe the we only need slices.Index: if it -1, we can we're fine. if not, we can build a json patch based on this index.
| crd.Status.StoredVersions = newStoredVersions | ||
| if err := r.client.Status().Update(req.Ctx, &crd); err != nil { | ||
| req.Logger.Error(err, fmt.Sprintf("failed to remove %v from status of CRD %v", removedApiVersion, crdName)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Let's try to do it with json patch
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
de7f9e4 to
6db5666
Compare
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure In response to this:
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. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure, ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-upgrade-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws In response to this:
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. |
|
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws In response to this:
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. |
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
6db5666 to
ceed8c4
Compare
Pull Request Test Coverage Report for Build 13749340768Details
💛 - Coveralls |
| patchCommand := []map[string]interface{}{ | ||
| { | ||
| "op": "remove", | ||
| "path": fmt.Sprintf("/status/storedVersions/%d", versionIndex), | ||
| }, | ||
| } | ||
| patchBytes, err := json.Marshal(patchCommand) | ||
| if err != nil { | ||
| req.Logger.Error(err, "failed to marshal patch") | ||
| return err | ||
| } |
There was a problem hiding this comment.
Since the only dynamic element here is the index, I think it will be more efficient to use string & fmt here. Marshaling the object each time has a higher performance cost. Also, we won't need the error handling.
WDYT about this?
| patchCommand := []map[string]interface{}{ | |
| { | |
| "op": "remove", | |
| "path": fmt.Sprintf("/status/storedVersions/%d", versionIndex), | |
| }, | |
| } | |
| patchBytes, err := json.Marshal(patchCommand) | |
| if err != nil { | |
| req.Logger.Error(err, "failed to marshal patch") | |
| return err | |
| } | |
| patchBytes := []byte(fmt.Sprintf`[{"op":"remove","path":"/status/storedVersions/%d"}]`, versionIndex) |
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure In response to this:
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. |
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
ceed8c4 to
038fbce
Compare
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure In response to this:
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. |
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws In response to this:
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. |
|
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure In response to this:
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. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure In response to this:
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. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure In response to this:
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. |
|
hco-e2e-upgrade-operator-sdk-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-azure In response to this:
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. |
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
038fbce to
9bd89e8
Compare
| if !slices.Contains(crd.Status.StoredVersions, removedApiVersion) { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
We don't need this. We're doing it in line 1063
Since version 1.10, SSP moved to a new api version - v1beta2. In latest release, SSP completely removed the old API v1beta1 from their CRD. This might result in an upgrade issue with OLM, if the cluster has initially started with version 1.9 or below. This PR removes the old SSP api version v1beta1 from the storedVersions list in the SSP CRD status, to allow a smooth upgrade to 1.14.0. Signed-off-by: Oren Cohen <[email protected]>
orenc1
force-pushed
the
remove_old_ssp_apiversion_crd
branch
from
9bd89e8 to
38564bf
Compare
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nunnatsa 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
/cherry-pick release-1.14 |
|
@nunnatsa: once the present PR merges, I will cherry-pick it on top of In response to this:
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. |
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure In response to this:
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. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure In response to this:
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. |
|
/retest |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure In response to this:
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. |
|
/retest |
|
hco-e2e-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-aws In response to this:
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. |
|
@orenc1: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws In response to this:
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. |
|
/retest |
|
this fix can't work on main and 1.14. need to do it at 1.13 when |
Since version 1.10, SSP moved to a new api version -
v1beta2.In latest release, SSP completely removed the old API
v1beta1from their CRD.This might result in an upgrade issue with OLM, if the cluster has initially started with version 1.9 or below.
This PR removes the old SSP api version v1beta1 from the storedVersions list in the SSP CRD status, to allow a smooth upgrade to 1.14.0.
What this PR does / why we need it:
Reviewer Checklist
Jira Ticket:
Release note: