Adding system oom events from kubelet #6718

vishh · 2015-04-11T00:31:54Z

Kubelet will surface the most recent system OOM as events.

Status: Unit tests pending; System test pending

vishh · 2015-04-11T00:32:38Z

vmarmol · 2015-04-13T16:13:46Z

pkg/kubelet/kubelet.go

@@ -1776,6 +1777,51 @@ func (kl *Kubelet) recordNodeOnlineEvent() {
 	kl.recorder.Eventf(kl.nodeRef, "online", "Node %s is now online", kl.hostname)
 }

+// Returns the most recent sys oom event from cadvisor.
+// Returns nil if no sys oom events were observed.
+func (kl *Kubelet) getRecentSysOOMEvent() (*cadvisorApi.OomEventData, util.Time, error) {


WDYT of putting this in the cAdvisor subpackage and exposing a "RecentSysOOMEvent()". Trying to minimize code in kubelet.go :)

erictune · 2015-04-14T17:08:21Z

Is something going to actuate off this NodeCondition, or is it just to aid debugging?
Also, isn't an OOM a transient event rather than a state?
Based on those two things, seems like an event more appropriate than a Condition?

vishh · 2015-04-14T17:11:15Z

The OOM is a transient event. We will have monitoring systems alerting on
Node State, and an event is not suitable for such purposes!

On Tue, Apr 14, 2015 at 10:08 AM, Eric Tune notifications@github.com
wrote:

Is something going to actuate off this NodeCondition, or is it just to aid
debugging?

Also, isn't an OOM a transient event rather than a state?

Based on those two things, seems like an event more appropriate than a
Condition?

—
Reply to this email directly or view it on GitHub
#6718 (comment)
.

erictune · 2015-04-14T17:24:33Z

Turning an event into a state seems like the wrong way to model this. Better for monitoring to monitor the count of OOM events and alert on an excessive rate of events over a period. There may be multiple alerts for different rates and periods, and different groupings (single-node, cluster-wide). Faking this as a state would just confuse that.

dchen1107 · 2015-04-14T17:37:50Z

I agree with @erictune. I think what we really need is filtering those OOM events to figure out why a running container is killed and provided proper ContainerStatus. NodeCondition is changed at the moment when OOM is detected.

erictune · 2015-04-14T17:53:53Z

We should have both node and pod events. If a pod is killed due to OOM,
and we can determine the cause, then we report a pod event.
Additionally we report a node event for admins to monitor overall cluster
rate of OOMs, problematic nodes, to aid debugging in cases when OOM cannot
be matched to Pod, and to help debugging in cases where OOM causes
collateral damage to non-killed pods (which we have seen many times
internally).

On Tue, Apr 14, 2015 at 10:38 AM, Dawn Chen notifications@github.com
wrote:

I agree with @erictune https://github.com/erictune. I think what we
really need is filtering those OOM events to figure out why a running
container is killed and provided proper ContainerStatus. NodeCondition is
changed at the moment when OOM is detected.

—
Reply to this email directly or view it on GitHub
#6718 (comment)
.

vishh · 2015-04-21T23:11:13Z

@dchen1107: I have updated the PR to generate events. Testing is still missing. If the general outline of the PR works, I will add the tests.

vmarmol · 2015-04-22T16:40:44Z

Only downside to the watch approach we do today is that we'll miss OOM events from when we are down. This is probably okay initially. We should be able to fill in the gap when we come back up.

vishh · 2015-04-22T18:13:23Z

Yup. I would argue that we should not let the kubelet go down for too long
:) The alternative will demand checkpoints.

On Wed, Apr 22, 2015 at 9:41 AM, Victor Marmol notifications@github.com
wrote:

Only downside to the watch approach we do today is that we'll miss OOM
events from when we are down. This is probably okay initially. We should be
able to fill in the gap when we come back up.

—
Reply to this email directly or view it on GitHub
#6718 (comment)
.

vishh · 2015-04-22T22:47:47Z

@vmarmol: I added an unit test. The cadvisor API needs to be enhanced a bit to make the unit test useful. PTAL!

vmarmol · 2015-04-23T00:51:56Z

pkg/kubelet/kubelet.go

@@ -558,10 +561,25 @@ func (kl *Kubelet) Run(updates <-chan PodUpdate) {
 	}

 	go kl.syncNodeStatus()
+	// Run the system oom watcher forever.
+	util.Until(kl.runOOMWatcher, time.Microsecond, util.NeverStop)


nit: use util.Forever() (which uses Until under the covers, but makes it clear)

util.Forever is marked as being deprecated.

Whaaaaa? First I hear of it, thanks for the heads up :)

vmarmol · 2015-04-23T15:51:48Z

Huh, the integration test is failing with the error we were seeing yesterday. Were you able to find the cause?

vishh · 2015-04-23T15:52:03Z

@vmarmol: The integration tests are failing because the kubelet is not able to get node information. I will fix that and update the PR. Meanwhile, lets iron out the rest of the code.

vishh · 2015-04-23T15:52:54Z

Not yet.

On Thu, Apr 23, 2015 at 8:52 AM, Victor Marmol notifications@github.com
wrote:

Huh, the integration test is failing with the error we were seeing
yesterday. Were you able to find the cause?

—
Reply to this email directly or view it on GitHub
#6718 (comment)
.

vmarmol · 2015-04-23T15:53:01Z

Thanks @vish!

vishh · 2015-04-28T00:28:13Z

@vmarmol: This PR is safe to be merged now. I am tackling the node events issue via a separate PR.

vmarmol · 2015-04-28T15:49:53Z

pkg/kubelet/kubelet.go

@@ -543,7 +546,9 @@ func (kl *Kubelet) GetNode() (*api.Node, error) {
 		return nil, errors.New("cannot list nodes")
 	}
 	host := kl.GetHostname()
+	glog.V(2).Infof("hostname: %q", host)


Remove these two debug lines? If you want to keep the hostname one can we make it V(4)?

vishh · 2015-04-28T16:33:41Z

PTAL @vmarmol

vmarmol · 2015-04-28T16:43:30Z

pkg/kubelet/cadvisor/cadvisor_fake.go

@@ -52,6 +52,6 @@ func (c *Fake) DockerImagesFsInfo() (cadvisorApiV2.FsInfo, error) {
 	return cadvisorApiV2.FsInfo{}, nil
 }

-func (c *Fake) GetPastEvents(request *events.Request) ([]*cadvisorApi.Event, error) {
-	return []*cadvisorApi.Event{}, nil
+func (self *Fake) WatchEvents(request *events.Request) (*events.EventChannel, error) {


vmarmol · 2015-04-28T16:49:20Z

pkg/kubelet/kubelet.go

@@ -543,6 +546,7 @@ func (kl *Kubelet) GetNode() (*api.Node, error) {
 	}
 	host := kl.GetHostname()
 	for _, n := range l.Items {
+		glog.V(2).Info(n)


vmarmol · 2015-04-28T16:56:03Z

LGTM, thanks @vishh! Will wait for green.

vishh · 2015-04-28T16:56:57Z

I apologize for the slipping in useless log lines. Thanks for the thorough review @vmarmol

vmarmol · 2015-04-28T16:59:53Z

No worries :D thanks for the quick fixes!

vmarmol · 2015-04-28T20:03:58Z

@vishh the integration test failed (again) can you take a look? I don't think it's a flake since it's failed twice now.

useful for recording the timestamp of events that happened in the past.

Kubelet will continuously watch for system OOMs and generate events whenever it encounters a system OOM.

vishh · 2015-04-29T18:22:35Z

@vmarmol: The integration tests did catch a bug :) I updated the PR and it should pass this time around.

Adding system oom events from kubelet

andyxning · 2016-11-19T00:33:46Z

pkg/kubelet/oom_watcher.go

+			cadvisorApi.EventOom: true,
+		},
+		ContainerName:        "/",
+		IncludeSubcontainers: false,


Seems this option, IncludeSubcontainers, should be opened, otherwise i am afraid we can not query cadvisor with correct result. As all containers are under the root container and we should include all descendants

googlebot added the cla: yes label Apr 11, 2015

vishh added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Apr 11, 2015

vishh force-pushed the sys_oom1 branch 2 times, most recently from 60e63b3 to e25ae66 Compare April 11, 2015 00:34

dchen1107 self-assigned this Apr 13, 2015

vmarmol reviewed Apr 13, 2015
View reviewed changes

vishh force-pushed the sys_oom1 branch 2 times, most recently from c413141 to 229e3de Compare April 21, 2015 23:09

vishh changed the title ~~WIP: Adding System OOM node condition~~ Adding system oom events from kubelet Apr 21, 2015

vishh force-pushed the sys_oom1 branch from 229e3de to 5168e2b Compare April 22, 2015 22:46

vmarmol reviewed Apr 23, 2015
View reviewed changes

vishh force-pushed the sys_oom1 branch from 5168e2b to 63ba874 Compare April 23, 2015 18:56

vmarmol reviewed Apr 28, 2015
View reviewed changes

vishh force-pushed the sys_oom1 branch 2 times, most recently from d684531 to ecab94a Compare April 28, 2015 16:33

vmarmol reviewed Apr 28, 2015
View reviewed changes

vishh force-pushed the sys_oom1 branch 2 times, most recently from 553d0a8 to 7e989b4 Compare April 28, 2015 16:47

vmarmol reviewed Apr 28, 2015
View reviewed changes

vishh force-pushed the sys_oom1 branch 2 times, most recently from 63c32b2 to 2ff6a95 Compare April 28, 2015 16:51

vishh added 2 commits April 28, 2015 16:13

Adding events API to cadvisor interface.

2f68cdd

Adding a 'PastEventf' method to EventRecorder interface. This will be

820d0f3

useful for recording the timestamp of events that happened in the past.

vishh force-pushed the sys_oom1 branch from 2ff6a95 to 711efcd Compare April 28, 2015 23:31

Adding events for system ooms.

020950f

Kubelet will continuously watch for system OOMs and generate events whenever it encounters a system OOM.

vishh force-pushed the sys_oom1 branch from 711efcd to 020950f Compare April 29, 2015 18:19

vmarmol added a commit that referenced this pull request Apr 29, 2015

Merge pull request #6718 from vishh/sys_oom1

209b4fc

Adding system oom events from kubelet

vmarmol merged commit 209b4fc into kubernetes:master Apr 29, 2015

vishh mentioned this pull request Apr 29, 2015

Propagate sys oom event and status #2853

Closed

andyxning reviewed Nov 19, 2016

View reviewed changes

andyxning mentioned this pull request Nov 19, 2016

fix oom events emit #37141

Closed

neolit123 mentioned this pull request Dec 31, 2019

Remove recorder.PastEventf method #86727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding system oom events from kubelet #6718

Adding system oom events from kubelet #6718

vishh commented Apr 11, 2015

vishh commented Apr 11, 2015

vmarmol Apr 13, 2015

erictune commented Apr 14, 2015

vishh commented Apr 14, 2015

erictune commented Apr 14, 2015

dchen1107 commented Apr 14, 2015

erictune commented Apr 14, 2015

vishh commented Apr 21, 2015

vmarmol commented Apr 22, 2015

vishh commented Apr 22, 2015

vishh commented Apr 22, 2015

vmarmol Apr 23, 2015

vishh Apr 23, 2015

vmarmol Apr 23, 2015

vmarmol commented Apr 23, 2015

vishh commented Apr 23, 2015

vishh commented Apr 23, 2015

vmarmol commented Apr 23, 2015

vishh commented Apr 28, 2015

vmarmol Apr 28, 2015

vishh Apr 28, 2015

vishh commented Apr 28, 2015

vmarmol Apr 28, 2015

vishh Apr 28, 2015

vmarmol Apr 28, 2015

vmarmol commented Apr 28, 2015

vishh commented Apr 28, 2015

vmarmol commented Apr 28, 2015

vmarmol commented Apr 28, 2015

vishh commented Apr 29, 2015

andyxning Nov 19, 2016 •

edited

Loading

Adding system oom events from kubelet #6718

Adding system oom events from kubelet #6718

Conversation

vishh commented Apr 11, 2015

vishh commented Apr 11, 2015

Choose a reason for hiding this comment

erictune commented Apr 14, 2015

vishh commented Apr 14, 2015

erictune commented Apr 14, 2015

dchen1107 commented Apr 14, 2015

erictune commented Apr 14, 2015

vishh commented Apr 21, 2015

vmarmol commented Apr 22, 2015

vishh commented Apr 22, 2015

vishh commented Apr 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarmol commented Apr 23, 2015

vishh commented Apr 23, 2015

vishh commented Apr 23, 2015

vmarmol commented Apr 23, 2015

vishh commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarmol commented Apr 28, 2015

vishh commented Apr 28, 2015

vmarmol commented Apr 28, 2015

vmarmol commented Apr 28, 2015

vishh commented Apr 29, 2015

andyxning Nov 19, 2016 • edited Loading

Choose a reason for hiding this comment

andyxning Nov 19, 2016 •

edited

Loading