Increase loging verbosity for deleting stateful set pods

[gmarek]

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]

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]

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]

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]

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

System state changing (killing pod)

[kow3ns]

@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.

[0xmichalis]

Agreed with Ken. There is also the API audit log that can be introspected if required.

…

On Wed, Feb 28, 2018 at 9:49 PM, Kenneth Owens ***@***.***> wrote: @enisoc <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60579 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADuFf0DAJ3iXXy-JLIXPNAKtdfgSIcujks5tZbvMgaJpZM4SWgij> .

[gmarek]

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 change is 

[gmarek]

@kow3ns PTAL

[mfojtik]

@tnozicka FYI

[kow3ns]

/approve

[erictune]

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]

@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]

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]

/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]

[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:

• pkg/controller/statefulset/OWNERS [kow3ns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-github-robot]

/test all

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

[k8s-github-robot]

[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

• Additional instructions
• Commands for setting labels

[jdumars]

Thank you very much @erictune

[k8s-github-robot]

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.