-
Notifications
You must be signed in to change notification settings - Fork 549
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.
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
cephfs: Log clone progress during a clone operation #4918
Conversation
f923e0a
to
3f24712
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if we are using older version of the ceph that doesn't support the new fields? i assume it will be empty just want to confirm on it.
Yes, it will be empty. |
3f24712
to
992ff84
Compare
992ff84
to
500fde7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please test it out and paste the PVC describe output, just to make sure we are surfacing it to user, feel free to add E2E if you think it can be done as well
500fde7
to
946609f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Lets get the testing results
Testing results: PVC Events:
CSI Logs:
I am unable to understand what does this "MISSING" indicates in the log, can someone please help? |
In your PR, I don't see that to be the case 🤔. You could verify that once with you latest PR changes? |
The above output is using the latest changes only. |
/test ci/centos/mini-e2e-helm/k8s-1.31 |
946609f
to
f7ff8c3
Compare
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
f7ff8c3
to
a91c8aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just few small nits.
rest looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 Sorry,
didn't click add review on this requested change.
Please take a look, small nit.
I don't see the code for which you are suggesting the change, can you please re-highlight? |
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks!
internal/cephfs/store/fsjournal.go
Outdated
log.UsefulLog(ctx, err.Error()) | ||
|
||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.UsefulLog(ctx, err.Error()) | |
return nil, err | |
} | |
} | |
log.ErrorLog(ctx, err.Error()) | |
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be logically incorrect to term it as error
here.
Since, there is no error
as such with the clone, rather it is just in progress
and should get complete in sometime. So, I don't think we should log it as an error
log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is returned as a error. I would be fine with using debuglog, we don't use useful log anywhere else.
And your recent change no longer returns err if progress is missing. This needs to be changed as well. Observe the change in curly bracket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right.
Updated the code now, please take a look.
b9023dc
to
c5da289
Compare
Pull request has been modified.
@Mergifyio rebase |
log cephfs clone progress report during cephfs clone operation Signed-off-by: Nikhil-Ladha <[email protected]>
✅ Branch has been successfully rebased |
c5da289
to
68b4f9c
Compare
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 98cf078 |
/test ci/centos/k8s-e2e-external-storage/1.30 |
/test ci/centos/k8s-e2e-external-storage/1.29 |
/test ci/centos/mini-e2e-helm/k8s-1.30 |
/test ci/centos/mini-e2e-helm/k8s-1.29 |
/test ci/centos/mini-e2e/k8s-1.30 |
/test ci/centos/mini-e2e/k8s-1.29 |
/test ci/centos/k8s-e2e-external-storage/1.31 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/mini-e2e-helm/k8s-1.31 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/mini-e2e/k8s-1.31 |
Describe what this PR does
Add clone progress report in the clone status operation and report the same in the logs for user's reference.
Fixes: #4813
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>
: retest the<job-name>
after unrelatedfailure (please report the failure too!)