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.

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

Improved logging message for checking if node is shutdown. #65677

Merged

Conversation

@MorrisLaw
Copy link
Contributor

commented Jul 2, 2018

What this PR does / why we need it:
The previous error message was "Error getting data for node" which was too broad of a message and not very descriptive. This PR will update it to "Error checking if node is shutdown" so that it is more specific.

NONE
@MorrisLaw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

/sig cloud-provider

@idealhack

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

/lgtm
/ok-to-test
/assign @wlan0

@MorrisLaw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

/retest

1 similar comment
@MorrisLaw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

/retest

@@ -273,7 +273,7 @@ func (cnc *CloudNodeController) MonitorNode() {
// doesn't, delete the node immediately.
exists, err := ensureNodeExistsByProviderID(instances, node)
if err != nil {
glog.Errorf("Error getting data for node %s from cloud: %v", node.Name, err)
glog.Errorf("Error checking if node %s is shutdown: %v", node.Name, err)

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Jul 2, 2018

Member

This should be

Error checking if node %s exists: %v

This comment has been minimized.

Copy link
@jiatongw

jiatongw Jul 3, 2018

Member

error return should be in lower case, like this?
glog.Errorf("error checking if ...")

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Jul 3, 2018

Member

Generally I agree but we don't follow that in a bunch of other places in this file so maybe something to do as a follow up :)

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:node-controller-logging branch from 828847f to 45ab6d7 Jul 3, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 3, 2018

@andrewsykim

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/approve
/lgtm

Thanks @MorrisLaw!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, idealhack, MorrisLaw

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

@MorrisLaw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

Automatic merge from submit-queue (batch tested with PRs 65677, 65711, 65150, 65726). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit cf686a4 into kubernetes:master Jul 3, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation MorrisLaw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@MorrisLaw MorrisLaw deleted the MorrisLaw:node-controller-logging branch Jul 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.