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

Increase loging verbosity for deleting stateful set pods #60579

Merged
merged 1 commit into from Mar 14, 2018

Conversation

@gmarek
Member

gmarek commented Feb 28, 2018

We should always log reasons for deleting StatefulSet Pods.
@jdumars - what's the current process for putting such changes into the release? It's literally 0-risk change that helps with debugging.

cc @ttz21

NONE
@k8s-cherrypick-bot

This comment has been minimized.

k8s-cherrypick-bot commented Feb 28, 2018

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@jdumars

This comment has been minimized.

Member

jdumars commented Feb 28, 2018

Bug fixes are still accepted into the 1.10 release at the team's discretion. This one is slightly different, but seems in the spirit of cleanup/bug fix. If you can get it approved and LGTM'd by someone specifically in SIG-Apps, I will add it to the milestone.

@kow3ns kow3ns added this to In Progress in Workloads Feb 28, 2018

@kow3ns

I don't think we can do this based on our logging conventions. Originally I had some of these log statements at V(2) and @smarterclayton and @kargakis talked me down to V(4).

@enisoc

This comment has been minimized.

Member

enisoc commented Feb 28, 2018

Have the conventions changed since then? Currently under V(2) it lists exactly what we're doing here:

System state changing (killing pod)

@kow3ns

This comment has been minimized.

Member

kow3ns commented Feb 28, 2018

@enisoc We already report Pod lifecycle events via the event_recorder. Again, I did initially have these statements at V(2), but the other controllers log such events at V(4). Certainly we should not use V(0), and we are going to change the log level, we should do so in a consistent way across the controller manager process imo.

@kargakis

This comment has been minimized.

Member

kargakis commented Feb 28, 2018

@gmarek

This comment has been minimized.

Member

gmarek commented Mar 1, 2018

No there's nothing that specifies a reason. I can only tell that the Pod got deleted, which is good and I wouldn't want to loose that, but I can't tell why. While I don't really care why normal Pods disappear as long as replacement ones get created, I generally do care a lot about StatefulSet Pods dying and I'd like to be able to understand why.

V(2) sounds fine.

@thockin

This comment has been minimized.

Member

thockin commented Mar 5, 2018

This change is Reviewable

@gmarek

This comment has been minimized.

Member

gmarek commented Mar 6, 2018

@kow3ns PTAL

@mfojtik

This comment has been minimized.

Contributor

mfojtik commented Mar 9, 2018

@tnozicka FYI

@kow3ns

kow3ns approved these changes Mar 9, 2018

@kow3ns

This comment has been minimized.

Member

kow3ns commented Mar 9, 2018

/approve

@erictune

This comment has been minimized.

Member

erictune commented Mar 9, 2018

observations:

Kubernetes binaries are recommended to log at -v=2 per [https://github.com/kubernetes/community/blob/master/contributors/devel/logging.md]

Most Statefulsets are small.

Events are logged, so logging will not increase out of proportion if this change is merged.

If events are already logged, but are missing the reason, and events have a reason field, then why not fix the events to have the reason and not add this change?

@krmayankk

This comment has been minimized.

Contributor

krmayankk commented Mar 12, 2018

@gmarek @kow3ns agree with @erictune , if this reason is not available in events, we should fix that . it would be super useful to the user looking at kubectl describe to see the reason right there.

@gmarek

This comment has been minimized.

Member

gmarek commented Mar 12, 2018

I'm fine with logging events, but there are no events being emitted on those cases, except very generic 'Deleting Pods' ones afaict.

If someone will enhance events before 1.10 we can close this pr. Otherwise I'd like to merge it. @erictune @kow3ns

@erictune

This comment has been minimized.

Member

erictune commented Mar 14, 2018

/lgtm

If someone wants to later revert this and improve the event, that is fine. This seems to be providing real debugging value and has the benefit of having been written already.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erictune, gmarek, kow3ns

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 14, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 14, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@erictune @gmarek @kow3ns

Pull Request Labels
  • sig/apps: Pull Request will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help
@jdumars

This comment has been minimized.

Member

jdumars commented Mar 14, 2018

Thank you very much @erictune

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 14, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit 0261114 into kubernetes:master Mar 14, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation gmarek 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-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 Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

Workloads automation moved this from In Progress to Done Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment