Deprecate disableMDevConfiguration in favor of mediatedDevicesConfiguration.enabled#4057
Deprecate disableMDevConfiguration in favor of mediatedDevicesConfiguration.enabled#4057Ronilerr wants to merge 4 commits intokubevirt:mainfrom
Conversation
kubevirt-bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Ronilerr
force-pushed
the
enabledConfig
branch
from
2bbf1d5 to
3b21d30
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The change of the
decentralizedLiveMigrationdefault fromtruetofalsein multiple CRD/spec default blocks seems unrelated to deprecatingdisableMDevConfigurationand may be accidental; please confirm whether this behavioral change is intended and, if not, revert it. - The deprecation comment for
DisableMDevConfiguration(“This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed…”) is a bit awkward; consider rephrasing to something likeDeprecated: this field is ignored; use mediatedDevicesConfiguration.enabled instead. It will be removed in the next API version.for clarity and consistency across Go types and CRDs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change of the `decentralizedLiveMigration` default from `true` to `false` in multiple CRD/spec default blocks seems unrelated to deprecating `disableMDevConfiguration` and may be accidental; please confirm whether this behavioral change is intended and, if not, revert it.
- The deprecation comment for `DisableMDevConfiguration` (“This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed…”) is a bit awkward; consider rephrasing to something like `Deprecated: this field is ignored; use mediatedDevicesConfiguration.enabled instead. It will be removed in the next API version.` for clarity and consistency across Go types and CRDs.
## Individual Comments
### Comment 1
<location path="api/v1beta1/hyperconverged_types.go" line_range="469-470" />
<code_context>
- // +optional
- // +kubebuilder:default=false
- // +default=false
+ // Deprecated: This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed in the next version of the API.
DisableMDevConfiguration *bool `json:"disableMDevConfiguration,omitempty"`
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify and clean up the deprecation message wording.
The deprecation message is grammatically awkward (mixed tenses, extra space before the comma) and could confuse users. Prefer something like:
"Deprecated: This field is ignored; use spec.mediatedDevicesConfiguration.enabled instead. It will be removed in the next version of the API."
Also, explicitly including the full field path (`spec.mediatedDevicesConfiguration.enabled`) and correct casing helps users know exactly what to migrate to.
```suggestion
// Deprecated: This field is ignored; use spec.mediatedDevicesConfiguration.enabled instead. It will be removed in the next version of the API.
DisableMDevConfiguration *bool `json:"disableMDevConfiguration,omitempty"`
```
</issue_to_address>
### Comment 2
<location path="pkg/webhooks/mutator/hyperConvergedMutator.go" line_range="100-107" />
<code_context>
patches = mutateEvictionStrategy(hc, patches)
+ if hc.Spec.FeatureGates.DisableMDevConfiguration != nil && *hc.Spec.FeatureGates.DisableMDevConfiguration && //nolint:staticcheck // Read deprecated field to migrate CRs.
+ hc.Spec.MediatedDevicesConfiguration != nil && hc.Spec.MediatedDevicesConfiguration.Enabled == nil {
+ patches = append(patches, jsonpatch.JsonPatchOperation{
+ Operation: "add",
+ Path: "/spec/mediatedDevicesConfiguration/enabled",
+ Value: false,
+ })
+ }
+
if hc.Spec.MediatedDevicesConfiguration != nil {
</code_context>
<issue_to_address>
**question:** Migration logic ignores CRs that only used the deprecated feature gate without a MediatedDevicesConfiguration block.
Because this runs only when `DisableMDevConfiguration == true` and `Spec.MediatedDevicesConfiguration != nil`, CRs that previously relied *only* on the feature gate (no `mediatedDevicesConfiguration` block) will never get `enabled=false` materialized. With `ptr.Deref(mdevsConfig.Enabled, true)`, those clusters will effectively lose the "globally disabled via feature gate" behavior once they add any MDEV config. Please clarify whether this behavior change is intentional; if not, consider also creating `mediatedDevicesConfiguration.enabled=false` for CRs that have the feature gate set but no block yet.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Deprecated: This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed in the next version of the API. | ||
| DisableMDevConfiguration *bool `json:"disableMDevConfiguration,omitempty"` |
There was a problem hiding this comment.
suggestion (typo): Clarify and clean up the deprecation message wording.
The deprecation message is grammatically awkward (mixed tenses, extra space before the comma) and could confuse users. Prefer something like:
"Deprecated: This field is ignored; use spec.mediatedDevicesConfiguration.enabled instead. It will be removed in the next version of the API."
Also, explicitly including the full field path (spec.mediatedDevicesConfiguration.enabled) and correct casing helps users know exactly what to migrate to.
| // Deprecated: This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed in the next version of the API. | |
| DisableMDevConfiguration *bool `json:"disableMDevConfiguration,omitempty"` | |
| // Deprecated: This field is ignored; use spec.mediatedDevicesConfiguration.enabled instead. It will be removed in the next version of the API. | |
| DisableMDevConfiguration *bool `json:"disableMDevConfiguration,omitempty"` |
Signed-off-by: ronilerr <rrabinov@redhat.com>
Ronilerr
force-pushed
the
enabledConfig
branch
from
3b21d30 to
db18ca9
Compare
…on.enabled Signed-off-by: ronilerr <rrabinov@redhat.com>
Signed-off-by: ronilerr <rrabinov@redhat.com>
Ronilerr
force-pushed
the
enabledConfig
branch
from
db18ca9 to
2a45f8a
Compare
|
hco-e2e-operator-sdk-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-gcp, ci/prow/hco-e2e-upgrade-operator-sdk-aws DetailsIn 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 DetailsIn 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. |
…diatedDevicesConfiguration.enabled; Controller: migrate existing CRs; add tests Signed-off-by: ronilerr <rrabinov@redhat.com>
Ronilerr
force-pushed
the
enabledConfig
branch
from
2a45f8a to
53a5ac7
Compare
|
|
hco-e2e-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-azure DetailsIn 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. |
|
@Ronilerr: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-upgrade-prev-operator-sdk-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure DetailsIn 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 DetailsIn 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. |
nunnatsa
left a comment
nunnatsa
left a comment
There was a problem hiding this comment.
Added a few comments, but let's wait with it until we make the API changes.
| // Controller-driven upgrade: if deprecated disableMDevConfiguration is true and | ||
| // mediatedDevicesConfiguration.enabled is missing, set enabled=false so the CR | ||
| // is migrated without requiring a user edit. updateHyperConverged will persist it. | ||
| if r.migrateMDevConfigurationIfNeeded(req) { | ||
| req.Logger.Info("Migrated HCO spec: set mediatedDevicesConfiguration.enabled from deprecated disableMDevConfiguration") | ||
| } | ||
|
|
There was a problem hiding this comment.
This code should be in the mutating webhook.
| return nil | ||
| } | ||
|
|
||
| // migrateMDevConfigurationIfNeeded performs a controller-driven upgrade: when the deprecated |
| Expect(foundResource.Finalizers).To(Equal([]string{FinalizerName})) | ||
| }) | ||
|
|
||
| It("should migrate mediatedDevicesConfiguration.enabled from deprecated disableMDevConfiguration on reconcile", func() { |
| // +optional | ||
| // +kubebuilder:default=false | ||
| // +default=false | ||
| // Deprecated: This field is ignored and use mediatedDevicesConfiguration.Enabled instead , it will be removed in the next version of the API. |
There was a problem hiding this comment.
we're not ignoring it, but it is deprecated.
|
|
||
| func toKvMediatedDevicesConfiguration(hc *hcov1beta1.HyperConverged) *kubevirtcorev1.MediatedDevicesConfiguration { | ||
| mdevsConfig := hc.Spec.MediatedDevicesConfiguration | ||
| disabled := ptr.Deref(hc.Spec.FeatureGates.DisableMDevConfiguration, false) |
There was a problem hiding this comment.
we do need it. we don't ignoring the FG. if only the FG is true, then we need to set the KubeVirt's enabled field to false. If both the FG is set and our enabled field is set, we'll go with the field.
| Operation: "replace", | ||
| Path: "/spec/mediatedDevicesConfiguration/enabled", | ||
| Value: nil, |
There was a problem hiding this comment.
use the "remove" op instead.
| } | ||
|
|
||
| func getMutatePatches(hc *hcov1beta1.HyperConverged) []jsonpatch.JsonPatchOperation { | ||
| func getMutatePatches(hc *hcov1beta1.HyperConverged, oldHC *hcov1beta1.HyperConverged, operation admissionv1.Operation) []jsonpatch.JsonPatchOperation { |
There was a problem hiding this comment.
I think it will be clearer to have two functions - one for update and one for create, instead of asking for the operation each time.
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "replace", | ||
| Path: "/spec/featureGates/disableMDevConfiguration", | ||
| Value: !newEnabled, | ||
| }) |
There was a problem hiding this comment.
or drop it instead of changing it?
| Path: fmt.Sprintf("/spec/mediatedDevicesConfiguration/nodeMediatedDeviceTypes/%d/mediatedDeviceTypes", i), | ||
| Value: hcoNodeMdevTypeConf.MediatedDevicesTypes, //nolint SA1019 | ||
| Path: "/spec/mediatedDevicesConfiguration", | ||
| Value: map[string]interface{}{"enabled": false}, |
There was a problem hiding this comment.
use any instead of interface{}
| Value: map[string]interface{}{"enabled": false}, | |
| Value: map[string]any{"enabled": false}, |
kubevirt-bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
4 participants
What this PR does / why we need it:
Reviewer Checklist
Jira Ticket:
Release note: