Skip to content

Refactor PVC event propagation for populators#4137

Open
dsanatar wants to merge 1 commit into
kubevirt:mainfrom
dsanatar:vddk-warning
Open

Refactor PVC event propagation for populators#4137
dsanatar wants to merge 1 commit into
kubevirt:mainfrom
dsanatar:vddk-warning

Conversation

@dsanatar

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:

UpdatePVCBoundContionFromEvents was originally implemented to assist with propagating events from a prime PVC to the target DV so that if something went wrong in the prime PVC, a user could check the DV resource and it's bound condition message would display the latest event message. Unbound DV's already inherit the bound condition message from the associated PVC, so the intent of this function was to get the latest event messages from a prime PVC and set it as the bound condition of the target PVC so that it could be later read by the DV.

However, this function over-complicates this process by also allowing prime PVCs to update their bound condition message to their own latest event message. All that is needed for this propagation is for events from prime PVCs to get copied to the target PVCs (already implemented as apart of #3764) and then only the target PVC needs their bound condition updated for it to appear in the DV. Allowing prime PVCs through this update flow introduced unnecessary Patch calls and more potential for race conditions with no real benefit.

The main motivation for this refactor is also to allow certain cases when the prime PVC's bound condition is explicitly set by a separate component, such as here when a VDDK config map is missing from a cluster. This should take priority over events and should be reported back up to the DV instead of the events.

if vddkImageName == nil {
message := fmt.Sprintf("waiting for %s configmap or %s annotation for VDDK image", common.VddkConfigMap, cc.AnnVddkInitImageURL)
anno[cc.AnnBoundCondition] = "false"
anno[cc.AnnBoundConditionMessage] = message
anno[cc.AnnBoundConditionReason] = common.AwaitingVDDK
if err := r.updatePVC(pvc, r.log); err != nil {

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes https://redhat.atlassian.net/browse/CNV-57727

Special notes for your reviewer:

Release note:

Refactor PVC event propagation for populators

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 14, 2026
@kubevirt-bot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign akalenyu for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 49.608% (+0.04%) from 49.568% — dsanatar:vddk-warning into kubevirt:main

@dsanatar

Copy link
Copy Markdown
Collaborator Author

/retest

update logic to prioritize bound condition msg
from prime pvc if one exists. update function
name to reflect this behavior change

no longer set conditions for prime pvc as well,
only the target pvc needs to be updated in order for
events to get propogated to the DV.

extract out event parsing logic to new
getLatestEventMessage helper func.

Signed-off-by: dsanatar <dsanatar@redhat.com>
Assisted-by: Claude <noreply@anthropic.com>
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026
@kubevirt-bot

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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.

@kubevirt-prow

kubevirt-prow Bot commented Jun 29, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign akalenyu for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-prow

kubevirt-prow Bot commented Jun 29, 2026

Copy link
Copy Markdown

@dsanatar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cdi-unit-test cf95d61 link true /test pull-cdi-unit-test
pull-cdi-goveralls cf95d61 link false /test pull-cdi-goveralls
pull-cdi-apidocs cf95d61 link true /test pull-cdi-apidocs
pull-cdi-unit-test-s390x cf95d61 link false /test pull-cdi-unit-test-s390x
pull-cdi-generate-verify cf95d61 link true /test pull-cdi-generate-verify
pull-containerized-data-importer-e2e-s390x cf95d61 link false /test pull-containerized-data-importer-e2e-s390x
pull-cdi-linter cf95d61 link false /test pull-cdi-linter
pull-containerized-data-importer-fossa cf95d61 link true /test pull-containerized-data-importer-fossa
pull-cdi-verify-go-mod cf95d61 link true /test pull-cdi-verify-go-mod
pull-containerized-data-importer-e2e-hpp-latest cf95d61 link true /test pull-containerized-data-importer-e2e-hpp-latest
pull-containerized-data-importer-e2e-destructive cf95d61 link true /test pull-containerized-data-importer-e2e-destructive
pull-containerized-data-importer-e2e-istio cf95d61 link true /test pull-containerized-data-importer-e2e-istio
pull-containerized-data-importer-e2e-hpp-previous cf95d61 link true /test pull-containerized-data-importer-e2e-hpp-previous
pull-containerized-data-importer-e2e-ceph cf95d61 link true /test pull-containerized-data-importer-e2e-ceph
pull-containerized-data-importer-e2e-ceph-wffc cf95d61 link true /test pull-containerized-data-importer-e2e-ceph-wffc
pull-containerized-data-importer-e2e-upg cf95d61 link true /test pull-containerized-data-importer-e2e-upg
pull-containerized-data-importer-e2e-nfs cf95d61 link true /test pull-containerized-data-importer-e2e-nfs
pull-containerized-data-importer-non-csi-hpp cf95d61 link true /test pull-containerized-data-importer-non-csi-hpp
Details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants