Revert "Adapt ToSystemAffinity calls to new Velero API signature (#20…#2115
Revert "Adapt ToSystemAffinity calls to new Velero API signature (#20…#2115
Conversation
…)" This reverts commit 6b2eff5.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes remove the deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/controller/velero.go (1)
282-291: Please add regression coverage forloadAffinityedge cases.This switches from manual term assembly to direct affinity assignment, so a small test matrix for
nil,[], and multi-entryloadAffinitywould help lock down the intended scheduling behavior here and in the mirrored NodeAgent path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/velero.go` around lines 282 - 291, Add regression tests exercising the new affinity assignment for Velero by covering the edge cases: nil LoadAffinityConfig, empty slice, and multi-entry LoadAffinityConfig; specifically write tests that construct a DPA with dpa.Spec.Configuration.Velero.LoadAffinityConfig set to nil, [] and a slice with multiple entries, call the code path that builds veleroAffinityStruct and invokes kube.ToSystemAffinity, and assert veleroDeployment.Spec.Template.Spec.Affinity is nil for nil/empty cases and matches the expected combined affinity for multi-entry cases; also add the same matrix of tests for the mirrored NodeAgent path to ensure parity (use test names like TestVeleroLoadAffinity_Nil/Empty/Multi and TestNodeAgentLoadAffinity_Nil/Empty/Multi and reference kube.LoadAffinity, kube.ToSystemAffinity, veleroDeployment.Spec.Template.Spec.Affinity and the NodeAgent affinity assembly code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml`:
- Around line 178-181: You removed the deprecated caCertRef field from the
served v1alpha1 CRD schema (leaving only caCert), which breaks compatibility;
restore caCertRef in the served OpenAPI schema for v1alpha1 (keep it marked
deprecated in the description and optional) so existing DPA manifests aren't
rejected or pruned, and ensure its type/format matches the prior definition
(e.g., string/reference) and validation remains permissive—add the caCertRef
property back alongside caCert in the schema for v1alpha1 (and do not remove it
until a documented migration path is provided).
In `@bundle/manifests/oadp.openshift.io_dataprotectiontests.yaml`:
- Around line 135-139: The served DataProtectionTest CRD currently removes the
deprecated field spec.backupLocationSpec.objectStorage.caCertRef (previously
represented as caCert in the schema), which breaks v1alpha1 manifests; restore
the deprecated caCert/caCertRef entry back into the served schema for
DataProtectionTest (and its v1alpha1 schema) and mark it as deprecated/nullable,
ensure its type/format matches the prior definition (string, format: byte) and
add a clear deprecation description; keep the conversion/migration behavior
unchanged (or wire into existing conversion logic in any conversion webhook) so
older manifests remain accepted until explicit removal is staged and documented.
In `@config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml`:
- Around line 178-181: The CRD change removed the deprecated field caCertRef
from the DataProtectionApplication schema which is a breaking change; restore
caCertRef into the served/stored v1alpha1 schema (alongside the new caCert) and
mark it as deprecated (include a deprecation description and/or
x-kubernetes-deprecation message) so existing manifests won’t be dropped during
apply/upgrade; ensure the field name caCertRef appears with the correct type
(string/objectRef form used previously), nullable if previously allowed, and
keep both caCert and caCertRef present until a documented migration/conversion
plan is implemented.
In `@config/crd/bases/oadp.openshift.io_dataprotectiontests.yaml`:
- Around line 135-139: The CRD removed the deprecated caCertRef which breaks
round-trip compatibility for DataProtectionTest in the v1alpha1 schema; restore
caCertRef as a deprecated field alongside caCert (keep its name caCertRef and
mark as deprecated) in the served/storage schema for DataProtectionTest so older
manifests retain their CA reference on update, and include the same
type/format/description semantics as the original deprecated field so it remains
readable by existing clients while preventing its use going forward.
---
Nitpick comments:
In `@internal/controller/velero.go`:
- Around line 282-291: Add regression tests exercising the new affinity
assignment for Velero by covering the edge cases: nil LoadAffinityConfig, empty
slice, and multi-entry LoadAffinityConfig; specifically write tests that
construct a DPA with dpa.Spec.Configuration.Velero.LoadAffinityConfig set to
nil, [] and a slice with multiple entries, call the code path that builds
veleroAffinityStruct and invokes kube.ToSystemAffinity, and assert
veleroDeployment.Spec.Template.Spec.Affinity is nil for nil/empty cases and
matches the expected combined affinity for multi-entry cases; also add the same
matrix of tests for the mirrored NodeAgent path to ensure parity (use test names
like TestVeleroLoadAffinity_Nil/Empty/Multi and
TestNodeAgentLoadAffinity_Nil/Empty/Multi and reference kube.LoadAffinity,
kube.ToSystemAffinity, veleroDeployment.Spec.Template.Spec.Affinity and the
NodeAgent affinity assembly code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa927a6e-a96c-4892-a801-5aeea79021bb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yamlbundle/manifests/oadp.openshift.io_dataprotectiontests.yamlconfig/crd/bases/oadp.openshift.io_dataprotectionapplications.yamlconfig/crd/bases/oadp.openshift.io_dataprotectiontests.yamlgo.modinternal/controller/nodeagent.gointernal/controller/velero.go
| caCert: | ||
| description: |- | ||
| CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| Deprecated: Use CACertRef instead. | ||
| description: CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| format: byte | ||
| type: string |
There was a problem hiding this comment.
This turns caCertRef removal into a served-API break.
The description tweak is fine, but this schema update also drops the deprecated caCertRef field from v1alpha1. Existing DPA manifests or stored objects that still send that key will start failing validation or get pruned on the next write, and I don't see a migration/compatibility path in this PR. Please keep the deprecated field in the served schema until consumers have a documented upgrade path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml` around
lines 178 - 181, You removed the deprecated caCertRef field from the served
v1alpha1 CRD schema (leaving only caCert), which breaks compatibility; restore
caCertRef in the served OpenAPI schema for v1alpha1 (keep it marked deprecated
in the description and optional) so existing DPA manifests aren't rejected or
pruned, and ensure its type/format matches the prior definition (e.g.,
string/reference) and validation remains permissive—add the caCertRef property
back alongside caCert in the schema for v1alpha1 (and do not remove it until a
documented migration path is provided).
| caCert: | ||
| description: |- | ||
| CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| Deprecated: Use CACertRef instead. | ||
| description: CACert defines a CA bundle to use when verifying | ||
| TLS connections to the provider. | ||
| format: byte | ||
| type: string |
There was a problem hiding this comment.
This removes a deprecated field from the served DataProtectionTest API too early.
As shipped, this CRD no longer accepts spec.backupLocationSpec.objectStorage.caCertRef in v1alpha1. That breaks older DPT manifests with no visible conversion or migration story here. Please preserve the deprecated field in the served schema until the removal is explicitly staged and documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bundle/manifests/oadp.openshift.io_dataprotectiontests.yaml` around lines 135
- 139, The served DataProtectionTest CRD currently removes the deprecated field
spec.backupLocationSpec.objectStorage.caCertRef (previously represented as
caCert in the schema), which breaks v1alpha1 manifests; restore the deprecated
caCert/caCertRef entry back into the served schema for DataProtectionTest (and
its v1alpha1 schema) and mark it as deprecated/nullable, ensure its type/format
matches the prior definition (string, format: byte) and add a clear deprecation
description; keep the conversion/migration behavior unchanged (or wire into
existing conversion logic in any conversion webhook) so older manifests remain
accepted until explicit removal is staged and documented.
| caCert: | ||
| description: |- | ||
| CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| Deprecated: Use CACertRef instead. | ||
| description: CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| format: byte | ||
| type: string |
There was a problem hiding this comment.
Preserve caCertRef until you have a migration path.
This schema change also removes caCertRef from a served/stored v1alpha1 API. That is a breaking compatibility change for existing DataProtectionApplication manifests and live objects that still rely on the deprecated field: the next apply/update can silently drop it. Please keep caCertRef in the CRD as deprecated until there is a versioned migration/conversion plan, or document and test the upgrade explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml` around
lines 178 - 181, The CRD change removed the deprecated field caCertRef from the
DataProtectionApplication schema which is a breaking change; restore caCertRef
into the served/stored v1alpha1 schema (alongside the new caCert) and mark it as
deprecated (include a deprecation description and/or x-kubernetes-deprecation
message) so existing manifests won’t be dropped during apply/upgrade; ensure the
field name caCertRef appears with the correct type (string/objectRef form used
previously), nullable if previously allowed, and keep both caCert and caCertRef
present until a documented migration/conversion plan is implemented.
| caCert: | ||
| description: |- | ||
| CACert defines a CA bundle to use when verifying TLS connections to the provider. | ||
| Deprecated: Use CACertRef instead. | ||
| description: CACert defines a CA bundle to use when verifying | ||
| TLS connections to the provider. | ||
| format: byte | ||
| type: string |
There was a problem hiding this comment.
Keep caCertRef deprecated instead of removing it from the v1alpha1 schema.
DataProtectionTest has the same compatibility problem: removing caCertRef from the served/storage schema makes older manifests non-round-trippable and can drop the configured CA reference on update. This should stay in the CRD until you ship a clear migration/versioning story.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/crd/bases/oadp.openshift.io_dataprotectiontests.yaml` around lines 135
- 139, The CRD removed the deprecated caCertRef which breaks round-trip
compatibility for DataProtectionTest in the v1alpha1 schema; restore caCertRef
as a deprecated field alongside caCert (keep its name caCertRef and mark as
deprecated) in the served/storage schema for DataProtectionTest so older
manifests retain their CA reference on update, and include the same
type/format/description semantics as the original deprecated field so it remains
readable by existing clients while preventing its use going forward.
Signed-off-by: Michal Pryc <mpryc@redhat.com>
|
/ok-to-test |
|
/retest |
|
/test unit-test |
openshift-ci
bot
added
the
ok-to-test
|
@mpryc: No presubmit jobs available for openshift/oadp-operator@oadp-1.6 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. |
|
@mpryc: The following test 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. |
Signed-off-by: Michal Pryc <mpryc@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test unit-test |
|
@mpryc: No presubmit jobs available for openshift/oadp-operator@oadp-1.6 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. |
|
/test unit-test |
|
@mpryc: No presubmit jobs available for openshift/oadp-operator@oadp-1.6 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. |
|
/test all |
|
@kaovilai: No presubmit jobs available for openshift/oadp-operator@oadp-1.6 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. |
3 participants
…96)"
This reverts commit 6b2eff5.
Why the changes were made
How to test the changes made
Summary by CodeRabbit
caCertReffield from backup storage configuration schemas in DataProtectionApplication and DataProtectionTest custom resources.