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

Dedupe logging for PD SetUpAt and added a slow SetVolumeOwnership warning #83426

Merged
merged 1 commit into from
Oct 5, 2019
Merged

Dedupe logging for PD SetUpAt and added a slow SetVolumeOwnership warning #83426

merged 1 commit into from
Oct 5, 2019
+14 −17

Conversation

davidz627
Copy link
Contributor

SetUpAt errors actually bubble up to the operation_generator level and are logged in a much better way with more context. The one-off logs are duplicates and split context in different places.

Also we have a known issue with volume ownership being slow and it would be nice to surface a warning when this feature is being used. This would make debugging the issue much easier.

/kind cleanup
/sig storage
/assign @msau42 @jingxu97
/priority backlog

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 2, 2019
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. priority/backlog Higher priority than priority/awaiting-more-evidence. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 2, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2019
@davidz627 davidz627 force-pushed the fix/improveLogs branch 2 times, most recently from 51194ce to ddbff58 Compare October 2, 2019 23:38
@davidz627
Copy link
Contributor Author

/retest

jingxu97
jingxu97 reviewed Oct 3, 2019
View reviewed changes
pkg/volume/gcepd/gce_pd.go
@@ -412,35 +410,31 @@ func (b *gcePersistentDiskMounter) SetUpAt(dir string, mounterArgs volume.Mounte
if err != nil {
notMnt, mntErr := b.mounter.IsLikelyNotMountPoint(dir)
if mntErr != nil {
klog.Errorf("IsLikelyNotMountPoint check failed: %v", mntErr)
return err
return fmt.Errorf("failed to mount: %v. Cleanup IsLikelyNotMountPoint check failed: %v", err, mntErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

not your change. but I am wondering whether it is necessary to check IsLikelyNotMountPoint multiple times. If mount failed, is it better we just try to unmount (maybe a few times) and then try to remove dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is outside of the scope of this PR and I won't change it. Feel free to create an issue :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened an issue here #83515

jingxu97
jingxu97 reviewed Oct 3, 2019
View reviewed changes
pkg/volume/gcepd/gce_pd.go Outdated Show resolved Hide resolved
jingxu97
jingxu97 reviewed Oct 3, 2019
View reviewed changes
pkg/volume/gcepd/gce_pd.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 3, 2019
@davidz627
Copy link
Contributor Author

@jingxu97 resolved comments

@jingxu97
Copy link
Contributor

jingxu97 commented Oct 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019
@davidz627
Copy link
Contributor Author

ping @msau42 for approval :)

@msau42
Copy link
Member

msau42 commented Oct 4, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, msau42

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2019
@k8s-ci-robot k8s-ci-robot merged commit 438918d into kubernetes:master Oct 5, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 5, 2019
@davidz627 davidz627 deleted the fix/improveLogs branch October 7, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingxu97 jingxu97 jingxu97 left review comments

@msau42 msau42 Awaiting requested review from msau42

Assignees

@jingxu97 jingxu97

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davidz627 @jingxu97 @msau42 @k8s-ci-robot