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

Improved readability for messages being logged #58061

Merged
merged 1 commit into from
Jan 13, 2018
Merged

Improved readability for messages being logged #58061

merged 1 commit into from
Jan 13, 2018

Conversation

ravisantoshgudimetla
Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla commented Jan 10, 2018 •
edited
Loading

What this PR does / why we need it:
This improves the readability for messages seen by end-user. /cc @jwforres @bsalamat - For UX
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 #57152

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 10, 2018
@ravisantoshgudimetla ravisantoshgudimetla changed the title Improves readability for messages being logged Improved readability for messages being logged Jan 10, 2018
k82cn
k82cn reviewed Jan 10, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -178,7 +178,7 @@ func (cache *schedulerCache) ForgetPod(pod *v1.Pod) error {

currState, ok := cache.podStates[key]
if ok && currState.pod.Spec.NodeName != pod.Spec.NodeName {
return fmt.Errorf("pod %v state was assumed on a different node", key)
return fmt.Errorf("pod %v was assumed on %v but is on %v", key, pod.Spec.NodeName, currState.pod.Spec.NodeName)
Copy link
Member

Choose a reason for hiding this comment

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

s/but is on/but assigned to/

k82cn
k82cn reviewed Jan 10, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -241,7 +241,7 @@ func (cache *schedulerCache) AddPod(pod *v1.Pod) error {
case ok && cache.assumedPods[key]:
if currState.pod.Spec.NodeName != pod.Spec.NodeName {
// The pod was added to a different node than it was assumed to.
glog.Warningf("Pod %v assumed to a different node than added to.", key)
glog.Warningf("pod %v was assumed to be on %v but got added to %v", key, pod.Spec.NodeName, currState.pod.Spec.NodeName)
Copy link
Member

Choose a reason for hiding this comment

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

start with big letter for log

k82cn
k82cn reviewed Jan 10, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -277,14 +277,14 @@ func (cache *schedulerCache) UpdatePod(oldPod, newPod *v1.Pod) error {
// before Update event, in which case the state would change from Assumed to Added.
case ok && !cache.assumedPods[key]:
if currState.pod.Spec.NodeName != newPod.Spec.NodeName {
glog.Errorf("Pod %v updated on a different node than previously added to.", key)
glog.Errorf("pod %v updated on a different node than previously added to.", key)
Copy link
Member

Choose a reason for hiding this comment

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

start with big letter for log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, @k82cn, this is right, it should start with lower case?

k82cn
k82cn reviewed Jan 10, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -304,7 +304,7 @@ func (cache *schedulerCache) RemovePod(pod *v1.Pod) error {
// before Remove event, in which case the state would change from Assumed to Added.
case ok && !cache.assumedPods[key]:
if currState.pod.Spec.NodeName != pod.Spec.NodeName {
glog.Errorf("Pod %v removed from a different node than previously added to.", key)
glog.Errorf("pod %v was assumed to be on %v but got added to %v", key, pod.Spec.NodeName, currState.pod.Spec.NodeName)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

This should be upper case.

bsalamat
bsalamat reviewed Jan 10, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -313,7 +313,7 @@ func (cache *schedulerCache) RemovePod(pod *v1.Pod) error {
}
delete(cache.podStates, key)
default:
return fmt.Errorf("pod state wasn't added but get removed. Pod key: %v", key)
return fmt.Errorf("pod %v wasn't added so cannot be removed", key)
Copy link
Member

Choose a reason for hiding this comment

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

Please mention cache here:
"pod %v is not found in the scheduler cache, so cannot be removed from it"

@ravisantoshgudimetla
Copy link
Contributor Author

@k82cn @bsalamat I have addressed your comments. PTAL.

@ravisantoshgudimetla
Copy link
Contributor Author

/retest

bsalamat
bsalamat reviewed Jan 11, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -131,7 +131,7 @@ func (cache *schedulerCache) AssumePod(pod *v1.Pod) error {
cache.mu.Lock()
defer cache.mu.Unlock()
if _, ok := cache.podStates[key]; ok {
return fmt.Errorf("pod %v state wasn't initial but get assumed", key)
return fmt.Errorf("Pod %v's state wasn't initialized, so can't be assumed", key)
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to "Pod %v is not found in the cache, so can't be assumed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @bsalamat

Copy link
Member

Choose a reason for hiding this comment

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

we should not return error message started with big letter :)

@bsalamat
Copy link
Member

/lgtm
Thanks, @ravisantoshgudimetla!

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 12, 2018
@k82cn
Copy link
Member

k82cn commented Jan 12, 2018

golint would like 1. start with big letter for log message, and 2. start with litter letter for message of error.

@k82cn
Copy link
Member

k82cn commented Jan 12, 2018

no other comments.

resouer
resouer reviewed Jan 12, 2018
View reviewed changes
pkg/scheduler/schedulercache/cache.go Outdated
@@ -345,7 +345,7 @@ func (cache *schedulerCache) GetPod(pod *v1.Pod) (*v1.Pod, error) {

podState, ok := cache.podStates[key]
if !ok {
return nil, fmt.Errorf("pod %v does not exist", key)
return nil, fmt.Errorf("Pod %v does not exist in scheduler cache", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

As k82cn suggested before, error message should start with lower case.

@bsalamat
Copy link
Member

/lgtm cancel
Ravi, could you please fix the character cases as Klaus suggested? Thanks!

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 12, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

Done @k82cn @bsalamat @resouer - Can someone point me to documentation on how add packages for golint validations? Can taking pkg/scheduler/schedulercache out of hack/.golint_failures help?

@bsalamat
Copy link
Member

I think removing the package from pkg/scheduler/schedulercache out of hack/.golint_failures does solve the problem, but the package may fail golint for other reasons beyond the scope of this PR.

@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Jan 13, 2018 •
edited
Loading

I have updated the PR and I created an issue for golint validation in scheduler pkg. #58234

@ravisantoshgudimetla ravisantoshgudimetla mentioned this pull request Jan 13, 2018
Closed
@bsalamat
Copy link
Member

/lgtm

Thanks a lot, @ravisantoshgudimetla for this PR and also for filing the issue to clean up golint warnings.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, ravisantoshgudimetla

Associated issue: #57152

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your 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 Jan 13, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 50e04f5 into kubernetes:master Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k82cn k82cn k82cn left review comments

@bsalamat bsalamat bsalamat left review comments

@resouer resouer resouer left review comments

Assignees

@resouer resouer

@bsalamat bsalamat

@k82cn k82cn

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

scheduler cache error messages do not read well
6 participants
@ravisantoshgudimetla @bsalamat @k82cn @k8s-ci-robot @k8s-github-robot @resouer