add log file tailing and logrotate support #1264

sjenning · 2016-05-04T20:17:26Z

Add some "tail -F" support (via polling and reopening) so that cadvisor can continue to track kernel log messages across log rotations.

Looked at doing this with fsnotify but 1) fsnotify depends on golang.org/x/sys/unix and that is about 10k LOC and 2) I couldn't find a tail implementation that actually worked with fsnotify.

k8s-bot · 2016-05-04T20:18:02Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

ncdc · 2016-05-04T20:18:18Z

utils/oomparser/oomparser.go

 	}, nil
 }
+
+// initializes an OomParser object. Returns and OomParser object and an error.


timstclair · 2016-05-04T21:30:49Z

Can we just use inotify?

timstclair · 2016-05-04T21:33:52Z

utils/tail/tail.go

+		glog.Errorf("Open failed on %s", t.filename)
+		return nil, err
+	}
+	t.file.Seek(0, os.SEEK_END)


This is a change in behavior, as we won't get old events.

that is true. i figured it was bug before, sending oom events for things that may have happened days ago in the logs.

Things from days ago are probably not relevant, but events from seconds ago might be. Either way, I think it's out of scope for this PR (feel free to leave a TODO)

sjenning · 2016-05-04T22:20:17Z

@timstclair i'm looking into using inotify. might be less of a hassle than fsnotify was.

sjenning · 2016-05-05T16:41:40Z

@timstclair @ncdc updated with inotify instead of polling

ncdc · 2016-05-05T17:27:33Z

utils/tail/tail.go

+				return
+			}
+			if reopen {
+				break


I'm pretty sure this will break you out of the outer loop. I'd recommend adding some unit tests :)

it breaks me out of the for loop starting line 90 which is what i want. i've done tests so i know it works. i'll have to think about how to unit test it.

My bad, had it backwards

sjenning · 2016-05-10T04:09:29Z

@timstclair new update that i think captures all the previous comments. PTAL when you can.

timstclair · 2016-05-11T01:49:18Z

utils/tail/tail.go

+	t.watcher, err = inotify.NewWatcher()
+	if err != nil {
+		glog.Errorf("Inotify init failed on %s: %v", t.filename, err)
+		return


nit: avoid naked returns

how do i avoid these?

Just change this line to return t, err or return nil, err. Also change the return type to (*Tail, error) since the variable names don't add anything in that case.

IMHO there are only 2 reasons to give return types names:

If the return type is something like (int, int, int, int), names distinguish the different ints (though I would usually prefer a struct for this)

If you need to modify a return value in a defer statement (again, this should be avoided if possible)

timstclair · 2016-05-18T00:32:05Z

Which parts are you having trouble with? The race condition can be avoided by protecting the reader with a mutex, and you can punt on the unit test for now.

timstclair · 2016-05-18T00:58:33Z

FYI, we're hoping to cut a v0.23.2 release tomorrow (2016-05-18) for kubernetes 1.3, and I'd like to get this change in. Please ping me if there's anything I can help with to move this along.

sjenning · 2016-05-18T00:59:49Z

I'll fix it up asap. Update within the hour.
On May 17, 2016 7:58 PM, "Tim St. Clair" notifications@github.com wrote:

FYI, we're hoping to cut a v0.23.2 release tomorrow (2016-05-18) for
kubernetes 1.3, and I'd like to get this change in. Please ping me if
there's anything I can help with to move this along.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1264 (comment)

timstclair · 2016-05-18T01:02:28Z

Great, thanks!

sjenning · 2016-05-18T01:52:12Z

@timstclair I'm close to having this done. Another half hour.

sjenning · 2016-05-18T02:28:03Z

@timstclair i've pushed an update. i reworked watchFile, but now the mutex handling is nasty in there. recommendations welcome.

k8s-bot · 2016-05-18T02:29:21Z

Jenkins GCE e2e

Build/test passed for commit a058132.

Build Log

k8s-bot · 2016-05-18T02:43:24Z

Jenkins GCE e2e

Build/test passed for commit 559dce2.

Build Log

k8s-bot · 2016-05-18T02:47:27Z

Jenkins GCE e2e

Build/test passed for commit 8d80aac.

Build Log

timstclair · 2016-05-18T03:23:37Z

utils/tail/tail.go

+
+type Tail struct {
+	reader     *bufio.Reader
+	sync.Mutex // protects reader


Call this readerLock (if it's embedded it's less clear what it's protecting)

sjenning · 2016-05-18T04:15:03Z

@timstclair ok, i incorporated your comments and introduced a readerState so that the locking can be cleaner. the real purpose is to avoid the race between a reader and the log file opening.

k8s-bot · 2016-05-18T04:15:22Z

Jenkins GCE e2e

Build/test passed for commit 4790ea2.

Build Log

timstclair · 2016-05-18T17:52:48Z

utils/tail/tail.go

+	glog.V(4).Infof("Log file %s moved/deleted", t.filename)
+	t.readerLock.Lock()
+	defer t.readerLock.Unlock()
+	t.readerState = readerStateOpening


Did you mean to set reader to nil here as well? I don't think setting the state does anything otherwise.

timstclair · 2016-05-18T17:55:25Z

Thanks, this looks much better. Just a couple small things, then LGTM.

timstclair · 2016-05-18T21:35:49Z

Taking this as-is, will follow up with a couple fixes. Thanks!

Cleanup tail util from #1264

ncdc reviewed May 4, 2016
View reviewed changes

utils/oomparser/oomparser.go

}, nil

}

// initializes an OomParser object. Returns and OomParser object and an error.

Copy link

Collaborator

ncdc May 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/and/an/

timstclair reviewed May 4, 2016
View reviewed changes

timstclair self-assigned this May 4, 2016

sjenning force-pushed the log-rotate-support branch from 448946c to c0c74f0 Compare May 5, 2016 16:40

ncdc reviewed May 5, 2016
View reviewed changes

sjenning force-pushed the log-rotate-support branch from c0c74f0 to e8eb9ca Compare May 10, 2016 04:08

timstclair reviewed May 11, 2016
View reviewed changes

Random-Liu mentioned this pull request May 17, 2016

KernelMonitor: Reconsider whether to use hpcloud/tail kubernetes/node-problem-detector#3

Closed

sjenning force-pushed the log-rotate-support branch from e8eb9ca to a058132 Compare May 18, 2016 02:26

sjenning force-pushed the log-rotate-support branch from a058132 to 559dce2 Compare May 18, 2016 02:40

sjenning force-pushed the log-rotate-support branch from 559dce2 to 8d80aac Compare May 18, 2016 02:44

timstclair reviewed May 18, 2016
View reviewed changes

add log file tailing and logrotate support

4790ea2

sjenning force-pushed the log-rotate-support branch from 8d80aac to 4790ea2 Compare May 18, 2016 04:12

timstclair reviewed May 18, 2016
View reviewed changes

timstclair merged commit 381f24b into google:master May 18, 2016

timstclair pushed a commit to timstclair/cadvisor that referenced this pull request May 18, 2016

Cleanup tail util from google#1264

ee24f28

timstclair mentioned this pull request May 18, 2016

Cleanup tail util from #1264 #1290

Merged

timstclair pushed a commit that referenced this pull request May 18, 2016

Merge pull request #1290 from timstclair/tail

18e5600

Cleanup tail util from #1264

sjenning deleted the log-rotate-support branch June 13, 2016 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add log file tailing and logrotate support #1264

add log file tailing and logrotate support #1264

sjenning commented May 4, 2016

k8s-bot commented May 4, 2016

ncdc May 4, 2016

timstclair commented May 4, 2016

timstclair May 4, 2016

sjenning May 4, 2016

timstclair May 4, 2016

sjenning commented May 4, 2016

sjenning commented May 5, 2016

ncdc May 5, 2016

sjenning May 5, 2016

ncdc May 5, 2016

sjenning commented May 10, 2016

timstclair May 11, 2016

sjenning May 18, 2016

timstclair May 18, 2016

timstclair commented May 18, 2016

timstclair commented May 18, 2016

sjenning commented May 18, 2016

timstclair commented May 18, 2016

sjenning commented May 18, 2016

sjenning commented May 18, 2016

k8s-bot commented May 18, 2016

k8s-bot commented May 18, 2016

k8s-bot commented May 18, 2016

timstclair May 18, 2016

sjenning commented May 18, 2016

k8s-bot commented May 18, 2016

timstclair May 18, 2016

timstclair commented May 18, 2016

timstclair commented May 18, 2016

add log file tailing and logrotate support #1264

add log file tailing and logrotate support #1264

Conversation

sjenning commented May 4, 2016

k8s-bot commented May 4, 2016

Choose a reason for hiding this comment

timstclair commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented May 4, 2016

sjenning commented May 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timstclair commented May 18, 2016

timstclair commented May 18, 2016

sjenning commented May 18, 2016

timstclair commented May 18, 2016

sjenning commented May 18, 2016

sjenning commented May 18, 2016

k8s-bot commented May 18, 2016

k8s-bot commented May 18, 2016

k8s-bot commented May 18, 2016

Choose a reason for hiding this comment

sjenning commented May 18, 2016

k8s-bot commented May 18, 2016

Choose a reason for hiding this comment

timstclair commented May 18, 2016

timstclair commented May 18, 2016