feat: allow external DataImportCron to manage DataSources #1260
Conversation
kubevirt-bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
kubevirt-bot
added
the
dco-signoff: yes
|
|
@sourcery-ai review |
Reviewer's Guide by SourceryThis pull request introduces changes to allow DataSources created by SSP to be managed by external DataImportCrons. The changes include modifications to the reconcile logic to prevent reconciliation of DataSources managed by external DataImportCrons, added tests to verify the new functionality, and error handling to prevent conflicts between DataImportCron templates and external DataImportCrons. Sequence diagram for dataSourceAutoUpdateEnabled with external DataImportCronsequenceDiagram
participant DS as DataSource
participant K8sClient as Kubernetes Client
participant DIC as DataImportCron
DS->>K8sClient: Get DataSource
K8sClient-->>DS: Returns DataSource with dataImportCronLabel
DS->>DS: Check if DataImportCron template exists
alt DataImportCron template does not exist
DS->>DS: externalCronExists = true
DS->>DS: Return reconcileDataSource = false, reconcileDataImportCron = false
else DataImportCron template exists
DS->>DS: externalCronExists = false
end
Sequence diagram for dataSourceAutoUpdateEnabled with DataImportCron templatesequenceDiagram
participant DS as DataSource
participant K8sClient as Kubernetes Client
participant DIC as DataImportCron
DS->>K8sClient: Get DataSource
K8sClient-->>DS: Returns DataSource
DS->>DS: Check if DataImportCron template exists
alt DataImportCron template exists
DS->>DS: Check if external DataImportCron exists
alt external DataImportCron exists
DS->>DS: Return error
else external DataImportCron does not exist
DS->>DS: Check if DataSource uses golden image PVC
alt DataSource uses golden image PVC
DS->>DS: Return reconcileDataSource = true, reconcileDataImportCron = false
else DataSource does not use golden image PVC
DS->>DS: Return reconcileDataSource = false, reconcileDataImportCron = true
end
end
else DataImportCron template does not exist
DS->>DS: Return reconcileDataSource = true, reconcileDataImportCron = false
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @akrejcir - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why
autoUpdateEnabledResult_RENAMEwas renamed. - The logic in
dataSourceAutoUpdateEnabledis complex; consider breaking it down into smaller, well-named functions to improve readability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // TODO -- rewrite comment | ||
| // If PVC exists, DataSource does not use auto-update. | ||
| // Otherwise, DataSource uses auto-update. |
There was a problem hiding this comment.
suggestion: Address the 'rewrite comment' TODO.
The current comment regarding golden image PVC behavior is ambiguous. Refining this comment to clearly explain the intended logic would make the code easier to follow.
| // TODO -- rewrite comment | |
| // If PVC exists, DataSource does not use auto-update. | |
| // Otherwise, DataSource uses auto-update. | |
| // Check if a golden image PVC exists for this DataSource. | |
| // If the golden image PVC exists, this indicates that a golden image is present, | |
| // meaning auto-update is disabled. | |
| // If the PVC does not exist, auto-update is enabled. |
| const dataImportCronLabel = "cdi.kubevirt.io/dataImportCron" | ||
|
|
||
| func dataSourceAutoUpdateEnabled(dataSource *cdiv1beta1.DataSource, cronByDataSource map[client.ObjectKey]*ssp.DataImportCronTemplate, request *common.Request) (bool, error) { | ||
| func dataSourceAutoUpdateEnabled(dataSource *cdiv1beta1.DataSource, cronByDataSource map[client.ObjectKey]*ssp.DataImportCronTemplate, request *common.Request) (autoUpdateEnabledResult_RENAME, error) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting helper functions and using early returns to simplify the dataSourceAutoUpdateEnabled function and reduce nested conditionals, improving readability and maintainability of the code.
The new changes do add complexity by mixing multiple decision points in one function. Consider extracting helper functions to isolate key decision points and flatten the nested conditionals. For example, you could extract determining if an external cron is present and the PVC & Ready condition checks into separate helper functions:
func externalCronExists(found *cdiv1beta1.DataSource, cronTemplate *ssp.DataImportCronTemplate) bool {
if found == nil {
return false
}
label, ok := found.GetLabels()[dataImportCronLabel]
return ok && (cronTemplate == nil || cronTemplate.Name != label)
}Then restructure dataSourceAutoUpdateEnabled using early returns:
func dataSourceAutoUpdateEnabled(dataSource *cdiv1beta1.DataSource, cronByDataSource map[client.ObjectKey]*ssp.DataImportCronTemplate, request *common.Request) (autoUpdateEnabledResult_RENAME, error) {
objectKey := client.ObjectKeyFromObject(dataSource)
cronTemplate, cronExists := cronByDataSource[objectKey]
found := &cdiv1beta1.DataSource{}
err := request.Client.Get(request.Context, objectKey, found)
if err != nil && !errors.IsNotFound(err) {
return autoUpdateEnabledResult_RENAME{}, err
}
if errors.IsNotFound(err) {
found = nil
}
if externalCronExists(found, cronTemplate) {
// Return false for both when external cron is detected.
return autoUpdateEnabledResult_RENAME{reconcileDataSource: false, reconcileDataImportCron: false}, nil
}
if !cronExists {
return autoUpdateEnabledResult_RENAME{reconcileDataSource: true, reconcileDataImportCron: false}, nil
}
usesGolden, err := dataSourceUsesGoldenImagePVC(found, dataSource, request)
if err != nil {
return autoUpdateEnabledResult_RENAME{}, err
}
if usesGolden {
return autoUpdateEnabledResult_RENAME{reconcileDataSource: true, reconcileDataImportCron: false}, nil
}
return autoUpdateEnabledResult_RENAME{reconcileDataSource: false, reconcileDataImportCron: true}, nil
}This refactoring isolates decision logic into well-named helpers and reduces nesting, making the flow easier to understand while preserving functionality.
akrejcir
force-pushed
the
allow-external-dic
branch
from
89043bf to
92e6176
Compare
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
akrejcir
changed the title
kubevirt-bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Hey @akrejcir - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for i := range existingCrons.Items { | ||
| cron := &existingCrons.Items[i] |
There was a problem hiding this comment.
| for i := range existingCrons.Items { | |
| cron := &existingCrons.Items[i] | |
| for i, cron := range existingCrons.Items { |
Should be fine?
| return false, nil | ||
| func chooseReconciliationObjects(dataSource *cdiv1beta1.DataSource, cronTemplate *ssp.DataImportCronTemplate, existingCron *cdiv1beta1.DataImportCron, request *common.Request) (reconcileObjects, error) { | ||
| if existingCron != nil { | ||
| if cronTemplate != nil && common.CheckOwnerAnnotation(existingCron, request.Instance) { |
There was a problem hiding this comment.
What should happen if SSP owns the existing cron but cron template is nil? Will it be removed?
There was a problem hiding this comment.
Yes. Then the operator assumes that the existing cron was previously created from a cron template by SSP.
Here is the code that removes owned crons that don't have template:
ssp-operator/internal/operands/data-sources/reconcile.go
Lines 514 to 531 in 92e6176
| if externalDataSource.GetLabels() == nil { | ||
| externalDataSource.SetLabels(map[string]string{}) | ||
| } |
There was a problem hiding this comment.
I've changed the tests.
Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
The DataSources created by SSP can now be managed by a DataImportCron that was not created by SSP. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>
akrejcir
force-pushed
the
allow-external-dic
branch
from
92e6176 to
9390d11
Compare
|
|
@akrejcir: 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. |
| }] = cron | ||
| } | ||
| } | ||
|
|
||
| var dataSourceInfos []dataSourceInfo | ||
| for i := range d.sources { | ||
| dataSource := d.sources[i] // Make a copy |
There was a problem hiding this comment.
would it make sense to store directly pointer to the object in this variable, so you don't have to add & in other places?
There was a problem hiding this comment.
I wanted to make a copy, because it is modified in the line below:
dataSource.Namespace = internal.GoldenImagesNamespace
It's a shallow copy. It was enough, because we don't change any pointer fields.
I think dataSource := &(d.sources[i]) would not make a copy.
|
When external DataImporCron is created, it does not trigger SSP reconciliation, so the operator has no chance to react to it, and give management of DataSource to CDI. I need to rethink this. /hold |
kubevirt-bot
added
the
do-not-merge/hold
kubevirt-bot
added
the
do-not-merge/work-in-progress
kubevirt-bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
The functionality implemented in this PR would be more difficult with the upcoming heterogeneous cluster functionality. Closing this for now, we can reopen if we resume development. /close |
|
@akrejcir: Closed this PR. 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. |
What this PR does / why we need it:
The DataSources created by SSP can now be managed by a DataImportCron that was not created by SSP.
Which issue(s) this PR fixes:
Fixes: https://issues.redhat.com/browse/CNV-49658
Release note: