Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Status reporting for CSI & Smart clones with WFFC storage #2364

Merged
merged 2 commits into from
Aug 5, 2022
Merged

Status reporting for CSI & Smart clones with WFFC storage #2364

merged 2 commits into from
Aug 5, 2022

Conversation

akalenyu
Copy link
Collaborator

@akalenyu akalenyu commented Jul 20, 2022 •
edited
Loading

What this PR does / why we need it:
Currently, fancy clones (CSI/Smart clone) with WFFC storage classes will just hang in "progress" phases such as SnapshotForSmartCloneInProgress instead of WaitForFirstConsumer.
This change reports the correct status for those target DataVolumes.

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 kubevirt/kubevirt#7927

Special notes for your reviewer:
The first commit is about all logging that's not V(1) being broken, unless I'm missing something

Release note:

Status reporting for CSI & Smart clones with WFFC storage

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 20, 2022
@mhenriks
Copy link
Member

No functional test for this? Can we create a temporary storageclass/volumesnapshotclass based on rook-ceph-block that is WFFC?

@akalenyu
Copy link
Collaborator Author

No functional test for this? Can we create a temporary storageclass/volumesnapshotclass based on rook-ceph-block that is WFFC?

/hold
Yeah just wanted to get an idea how this is working before I dive into the WFFC storageclass variation

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2022
@akalenyu
Copy link
Collaborator Author

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2022
@akalenyu
Copy link
Collaborator Author

/test all

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2022
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2022
awels
awels reviewed Aug 3, 2022
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good minor nitpick

pkg/controller/datavolume-controller.go Outdated
@@ -901,7 +908,7 @@ func (r *DatavolumeReconciler) reconcileCsiClonePvc(log logr.Logger,
r.updateCloneStatusPhase(cdiv1.CloneScheduled, datavolume, nil, CsiClone)
}

log.Info("Creating PVC for datavolume")
log.Info("reconcileCsiClonePvc: Creating PVC for datavolume")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be something like:
log.WithName("reconcileCsiClonePvc").Info("Creating PVC for datavolume")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep done

@akalenyu
Fix logging level so we respect it in controllers/operator
d7b41f7 
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@akalenyu
Fix CSI & Smart clones with WFFC storage status reporting
ef82763 
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@awels
Copy link
Member

awels commented Aug 4, 2022

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2022
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2022
@awels
Copy link
Member

awels commented Aug 5, 2022

/retest

@awels
Copy link
Member

awels commented Aug 5, 2022

/retest-required

@kubevirt-bot kubevirt-bot merged commit 210848e into kubevirt:main Aug 5, 2022
@akalenyu
Copy link
Collaborator Author

/cherrypick release-v1.53

@kubevirt-bot kubevirt-bot mentioned this pull request Aug 17, 2022
Closed
@kubevirt-bot
Copy link
Contributor

@akalenyu: new pull request created: #2400

In response to this:

/cherrypick release-v1.53

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.

akalenyu added a commit to akalenyu/containerized-data-importer that referenced this pull request Aug 17, 2022
@akalenyu
Status reporting for CSI & Smart clones with WFFC storage (kubevirt#2364
d82fc70 
)

* Fix logging level so we respect it in controllers/operator

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Fix CSI & Smart clones with WFFC storage status reporting

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Aug 17, 2022
@akalenyu @ShellyKa13
[release-v1.53] Manual backport of 2364, 2375 (#2401)
c8f06b4 
* Status reporting for CSI & Smart clones with WFFC storage (#2364)

* Fix logging level so we respect it in controllers/operator

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Fix CSI & Smart clones with WFFC storage status reporting

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Check for pvc populated before doing any clone validations and actions (#2375)

In case our pvc is already populated no need to check for source PVC
unknown size etc.

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Signed-off-by: Shelly Kagan <skagan@redhat.com>
Co-authored-by: Shelly Kagan <78472213+ShellyKa13@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awels awels awels left review comments

@maya-r maya-r Awaiting requested review from maya-r

Assignees

@awels awels

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SnapshotForSmartClone doesn't work with storageclass volumeBindingMode WaitForFirstConsumer
4 participants
@akalenyu @mhenriks @awels @kubevirt-bot