kubelet: revert contextual logging support #110869
Conversation
k8s-ci-robot
added
release-note-none
|
@pohly: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-triage
k8s-ci-robot
added
area/kubelet
sig/node
|
pohly
force-pushed
the
kubelet-shutdown-test-revert
branch
from
08c53e0
to
063c25d
Compare
It turned out that using testing.T for logging had a race condition and potential panic because the tests keep goroutines running and testing.T is not supposed to be used anymore after test completion (kubernetes#110854). Either the code must be fixed to terminate all goroutines before a test ends (seems non-trivial because the usage of goroutines is fairly complex in the shutdown manager code), or ktesting must handle this case. A solution for this is pending in kubernetes/klog#337. Either way, solving this will take a bit longer. In the meantime, we should revert the change to get unit testing stable again. In order to make this a local change, ktesting is kept as a dependency of the test and thus Kubernetes.
pohly
force-pushed
the
kubelet-shutdown-test-revert
branch
from
063c25d
to
8384e72
Compare
| name string | ||
| fields fields | ||
| wantErr bool | ||
| exceptOutputContains string |
There was a problem hiding this comment.
expect here? Or you're on purpose.
There was a problem hiding this comment.
I am reverting to the code before my changes. It was like that.
| // hijack the klog output | ||
| tmpWriteBuffer := new(buffer) | ||
| klog.SetOutput(tmpWriteBuffer) | ||
| klog.LogToStderr(false) |
There was a problem hiding this comment.
Should we defer to revert this with defer LogToStderr(true)?
There was a problem hiding this comment.
Same here - I am just reverting.
|
Totally looks good to me, but would like to leave to someone more familiar with this. |
|
Here's the diff against the code before PR #110504: $ git diff 0669ba386bde2e756bc9c6779ad4a4f036200f28 -- pkg/kubelet/kubelet.go pkg/kubelet/kubelet_test.go pkg/kubelet/nodeshutdown
diff --git a/pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux_test.go b/pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux_test.go
index e682c0963f0..e58f2d4dea1 100644
--- a/pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux_test.go
+++ b/pkg/kubelet/nodeshutdown/nodeshutdown_manager_linux_test.go
@@ -45,6 +45,14 @@ import (
probetest "k8s.io/kubernetes/pkg/kubelet/prober/testing"
"k8s.io/utils/clock"
testingclock "k8s.io/utils/clock/testing"
+
+ // https://github.com/kubernetes/kubernetes/pull/110504 started using
+ // ktesting for the first time in Kubernetes and therefore added it to
+ // "vendor". We need to revert that PR for a short while (see
+ // https://github.com/kubernetes/kubernetes/issues/110854) but to
+ // make the revert local to this directory, we keep importing that
+ // package (no change to vendor).
+ _ "k8s.io/klog/v2/ktesting/init"
)
// lock is to prevent systemDbus from being modified in the case of concurrency. |
There was a problem hiding this comment.
Simple revert that fixes the data race
/lgtm
/assign @mrunalp
k8s-ci-robot
added
the
lgtm
|
/hold Let's merge #111001 instead. |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: endocrimes, pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/close The klog update was merged, the revert should not be needed anymore. |
|
@pohly: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind bug
/kind failing-test
What this PR does / why we need it:
It turned out that using testing.T for logging had a race condition and
potential panic because the tests keep goroutines running and testing.T is not
supposed to be used anymore after test
completion (#110854).
Either the code must be fixed to terminate all goroutines before a test
ends (seems non-trivial because the usage of goroutines is fairly complex in
the shutdown manager code), or ktesting must handle this case. A solution for
this is pending in kubernetes/klog#337.
Either way, solving this will take a bit longer. In the meantime, we should
revert the change to get unit testing stable again.
Which issue(s) this PR fixes:
Related-to: #110854
Special notes for your reviewer:
In order to make this a local change, ktesting is kept as a dependency of the
test and thus Kubernetes.
Does this PR introduce a user-facing change?