Deprecate mediatedDevicesTypes in favor of mediatedDeviceTypes #2402
Conversation
kubevirt-bot
added
release-note
Pull Request Test Coverage Report for Build 5399287220
💛 - Coveralls |
|
hco-e2e-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-sno-aws, ci/prow/okd-hco-e2e-upgrade-index-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/test-infra repository. |
|
hco-e2e-upgrade-index-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-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/test-infra repository. |
|
/retest |
|
hco-e2e-upgrade-prev-index-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-index-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/test-infra repository. |
|
/retest |
| Expect(mdevConf.MediatedDeviceTypes).To(HaveLen(2)) | ||
| Expect(mdevConf.MediatedDeviceTypes).To(ContainElements("nvidia-222", "nvidia-230")) |
There was a problem hiding this comment.
Do you ant also to check that mdevConf.MediatedDevicesTypes is empty?
| Expect(mdevConf.NodeMediatedDeviceTypes[1].NodeSelector).To(HaveKeyWithValue("testLabel2", "true")) | ||
|
|
||
| }) | ||
| It("should update the permitted host devices configuration from the HC", func() { | ||
| It("should propagate the mediated devices configuration from the HC with node selectors with new APIs", func() { |
There was a problem hiding this comment.
Not sure about "new API". This is very clear today. But one day (tomorrow?) it won't be new anymore...
There was a problem hiding this comment.
yes. it makes sense: let's call them mediatedDeviceTypes (explicitly) and deprecated (for better visibility).
| if len(hc.Spec.MediatedDevicesConfiguration.MediatedDevicesTypes) > 0 && len(hc.Spec.MediatedDevicesConfiguration.MediatedDeviceTypes) == 0 { //nolint SA1019 | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/spec/mediatedDevicesConfiguration/mediatedDeviceTypes", | ||
| Value: hc.Spec.MediatedDevicesConfiguration.MediatedDevicesTypes, //nolint SA1019 | ||
| }) | ||
| } | ||
| for i, hcoNodeMdevTypeConf := range hc.Spec.MediatedDevicesConfiguration.NodeMediatedDeviceTypes { | ||
| if len(hcoNodeMdevTypeConf.MediatedDevicesTypes) > 0 && len(hcoNodeMdevTypeConf.MediatedDeviceTypes) == 0 { //nolint SA1019 | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: fmt.Sprintf("/spec/mediatedDevicesConfiguration/nodeMediatedDeviceTypes/%d/mediatedDeviceTypes", i), | ||
| Value: hcoNodeMdevTypeConf.MediatedDevicesTypes, //nolint SA1019 | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we also want to clear to MediatedDevicesTypes fields?
There was a problem hiding this comment.
No, I think we should not: they are deprecated but still there and some external code could also try to read it and we don't want to break it.
With github.com/kubevirt/kubevirt/pull/8525 mediatedDevicesTypes got deprecated in favor of mediatedDeviceTypes on kubevirt/kubevirt. Let's do the same on the HCO side for consistency. The mutating webhook will take care of the migration to the new API and the validating webhook will prevent inconsistencies between the two APIs. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2054863 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
tiraboschi
force-pushed
the
mediateddevice
branch
from
d6f538f
to
f0cbe7b
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
okd-hco-e2e-operator-sdk-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/okd-hco-e2e-operator-sdk-gcp 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/test-infra repository. |
|
hco-e2e-upgrade-index-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-sno-azure, ci/prow/okd-hco-e2e-upgrade-index-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/test-infra repository. |
|
hco-e2e-upgrade-index-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-azure, ci/prow/hco-e2e-upgrade-prev-index-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/test-infra repository. |
|
@tiraboschi: 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/test-infra repository. I understand the commands that are listed here. |
|
/override-bot |
|
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/test-infra repository. |
|
[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
With kubevirt#2402 we deprecated MediatedDevicesTypes and NodeMediatedDeviceTypes.MediatedDevicesTypes for MediatedDeviceTypes and NodeMediatedDeviceTypes.MediatedDeviceTypes. We also introduced a mutating webhook to handle the conversion. Unfortunately make the two new names mandatory is breaking the validation of the new version of the CRD on OLM upgrades since only the previous names were there with the old CRD. We already have func test covering this area, but they are fooled by the mutating webhook that instead is not involved into the CRD validation process on the OLM side. On the other side, add an additional upgrade lane just for this is probably too expensive. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2241327 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
) With #2402 we deprecated MediatedDevicesTypes and NodeMediatedDeviceTypes.MediatedDevicesTypes for MediatedDeviceTypes and NodeMediatedDeviceTypes.MediatedDeviceTypes. We also introduced a mutating webhook to handle the conversion. Unfortunately make the two new names mandatory is breaking the validation of the new version of the CRD on OLM upgrades since only the previous names were there with the old CRD. We already have func test covering this area, but they are fooled by the mutating webhook that instead is not involved into the CRD validation process on the OLM side. On the other side, add an additional upgrade lane just for this is probably too expensive. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2241327 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
…en upgrades (kubevirt#2534) With kubevirt#2402 we deprecated MediatedDevicesTypes and NodeMediatedDeviceTypes.MediatedDevicesTypes for MediatedDeviceTypes and NodeMediatedDeviceTypes.MediatedDeviceTypes. We also introduced a mutating webhook to handle the conversion. Unfortunately make the two new names mandatory is breaking the validation of the new version of the CRD on OLM upgrades since only the previous names were there with the old CRD. We already have func test covering this area, but they are fooled by the mutating webhook that instead is not involved into the CRD validation process on the OLM side. On the other side, add an additional upgrade lane just for this is probably too expensive. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2241327 This is a manual cherry-pick of kubevirt#2534 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
…en upgrades (#2534) (#2537) With #2402 we deprecated MediatedDevicesTypes and NodeMediatedDeviceTypes.MediatedDevicesTypes for MediatedDeviceTypes and NodeMediatedDeviceTypes.MediatedDeviceTypes. We also introduced a mutating webhook to handle the conversion. Unfortunately make the two new names mandatory is breaking the validation of the new version of the CRD on OLM upgrades since only the previous names were there with the old CRD. We already have func test covering this area, but they are fooled by the mutating webhook that instead is not involved into the CRD validation process on the OLM side. On the other side, add an additional upgrade lane just for this is probably too expensive. Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2241327 This is a manual cherry-pick of #2534 Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
What this PR does / why we need it:
With github.com/kubevirt/kubevirt/pull/8525
mediatedDevicesTypes got deprecated in favor of mediatedDeviceTypes on kubevirt/kubevirt.
Let's do the same on the HCO side for consistency.
The mutating webhook will take care of the migration to the new API and the validating webhook will
prevent inconsistencies between the two APIs.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2054863
Reviewer Checklist
Jira Ticket:
Release note: