WIP: New design #27

vimalk78 · 2023-01-31T16:29:34Z

This PR attempts to rewrite the log-metrics-file-exporter with a new design.

Earlier design used linux inotify via package github.com/fsnotify/fsnotify, which abstarcts inotify callbacks with its own defined values. Also adding a IN_MODIFY watch on a log file generates too many events which very quickly leads to overflow error.

New design directly uses golang.org/x/sys/unix package to use inotify for better control, and uses IN_ONESHOT flag to reduce the number of events generated when log files are written. Please see DESIGN.md for details.

/cc @jcantrill @alanconway

/hold

openshift-ci · 2023-01-31T16:29:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vimalk78

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:

~~OWNERS~~ [vimalk78]

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

openshift-merge-robot · 2023-01-31T16:30:10Z

PR needs rebase.

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.

jcantrill · 2023-01-31T18:24:03Z

Dockerfile

+RUN make build
+
+#FROM registry.access.redhat.com/ubi8
+FROM centos:centos8


is this the only way we can install "strace" and "strace-cmd

This was added for debugging purpose only, will remove it.

jcantrill · 2023-01-31T18:24:31Z

Dockerfile

+RUN sed -i 's/mirrorlist/#mirrorlist/g' /etc/yum.repos.d/CentOS-*
+RUN sed -i 's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g' /etc/yum.repos.d/CentOS-*
+#RUN echo 'kernel.yama.ptrace_scope = 0' >> /etc/sysctl.conf
+RUN yum -y install strace


is there a reason we can not collapse these statements?

This was added for debugging purpose only, will remove it.

jcantrill · 2023-01-31T18:25:32Z

.dockerignore

@@ -0,0 +1,7 @@
+*
+!Makefile


Doesn't this mean to ignore the Makefile when copying?... same woth go.mod and go.sum. Don't we need these to build the image?

The first line says ignore all files, except the following lines which start with !
So the entries in this file are the files which get added.

jcantrill · 2023-01-31T19:19:21Z

Design.md

+set of watch flags are used for directories and files.
+
+Watches for folders:
+ - `IN_CREATE`: watch is triggered when a file or direcory is created in this directory.


jcantrill · 2023-01-31T19:22:40Z

ISSUES.md

+as one returned in step 1. Once this happens, there are no more events received for file changes. the loop essentially halts.
+
+#### Workaround
+Save the watch desctiptor for files, and compare the new watch descriptor with earlier one, if same add watch again will the returned watch descriptor is different.


...add until(?) the returned watch...

jcantrill · 2023-02-01T16:39:39Z

internal/inotify/inotify.go

+							// a new container directory got created, add a watch for this directory
+							glog.V(3).Infof("A new container got created. container: %q", newdir)
+						}
+						must(n.WatchDir(newdir))


This fails the entire process if the watch fails right? Is there no recovery from here from that which we know?

Best is to restart the process

jcantrill · 2023-02-01T16:40:24Z

internal/inotify/inotify.go

+						// ignore files which are not log files
+						if strings.HasSuffix(logfile, ".log") {
+							glog.V(3).Infof("a new log file got created: %q\n", logfile)
+							must(n.WatchLogFile(logfile))


Same question here

Best is to restart the process

jcantrill · 2023-02-01T16:40:46Z

internal/inotify/inotify.go

+			case e.IsDelete():
+				if e.path != "" {
+					if e.IsDir() {
+						// a directory got created in a directory


update or remove comment

jcantrill · 2023-02-01T16:43:36Z

internal/inotify/inotify.go

+		case <-tickerCh:
+			/**/
+			sizemtx.Lock()
+			glog.V(0).Infof("sizes(%d): %s\nEventsSent: %d, EventsHandled: %d\n", len(Sizes), func(m map[string]int64) string {


This looks like we should bump the verbosity since Marshalling is likely an expensive operation. Should the marshal actually have a if block around the actual work so it only happens when v>X

This is for debugging purpose only, will remove this once metrics are added.

jcantrill · 2023-02-01T16:44:01Z

internal/inotify/inotify.go

+			n.mtx.RUnlock()
+
+			l := strings.Join(wl, ",\n")
+			glog.V(0).Infof("Watching paths: \n%s\nTotal watches: %d\n", l, len(wl))


same here. seems expensive

This is like a running log of paths being watched and is also for debugging purpose. It is getting printed once every tick, which can be made configurable, and log level changed to 3.

alanconway · 2023-02-01T20:12:09Z

@vimalk78 I've been poking around. You don't need to create a separate watch for every file. A watch on a directory will also be notified about events to files in that directory, with the file name in the event. That may solve your mystery with running into the same watch descriptor and hanging - perhaps you are running out of watch descriptors. Watching files only will reduce the number greatly.

That does create a problem with ONESHOT - while you're not watching the directory you could miss file create/delete events. Possibly you can solve that by having 2 watches per directory. One is only for create/delete and is not ONESHOT.
The other is for all other file change events, and is ONEHSOT. That means ONESHOT lets you ignore some WRITE events, but the continuous create/delete watch won't miss any create or delete events.

Here's a demos:

package main

import (
	"bytes"
	"fmt"
	"os"
	"time"
	"unsafe"

	"golang.org/x/sys/unix"
)

func must[T any](v T, err error) T {
	if err != nil {
		panic(err)
	}
	return v
}

func main() {
	fd := must(unix.InotifyInit1(unix.IN_CLOEXEC | unix.IN_NONBLOCK))
	wf := os.NewFile(uintptr(fd), "")
	_ = os.MkdirAll("/tmp/test", 0777)
	_ = must(unix.InotifyAddWatch(fd, "/tmp/test", unix.IN_ALL_EVENTS))
	go func() {
		const size = unix.SizeofInotifyEvent
		buf := make([]byte, size*1000)
		for {
			n := must(wf.Read(buf[:]))
			for i := 0; i+size < n; {
				e := *(*unix.InotifyEvent)(unsafe.Pointer(&buf[i]))
				i += size      // After the event struct
				if e.Len > 0 { // Event has a name
					end := bytes.IndexByte(buf[i:], 0)
					name := string(buf[i : i+end]) // NULL terminated string
					fmt.Println(name)
				}
				fmt.Println(e)
				i += int(e.Len) // Move past name
			}
		}
	}()
	f := must(os.Create("/tmp/test/x"))
	fmt.Fprintln(f, "hello")
	fmt.Fprintln(f, "goodbye")
	f.Close()
	f = must(os.Create("/tmp/test/y"))
	f.Close()
	time.Sleep(time.Minute)
}

openshift-ci · 2023-02-09T19:36:31Z

@vimalk78: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	`6b211e2`	link	true	`/test images`
ci/prow/test	`6b211e2`	link	true	`/test test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

vimalk78 added 2 commits January 31, 2023 21:48

Deleted the source to prepare for new design

28a5cec

Updated source with new design

dafd190

openshift-ci bot requested review from alanconway and jcantrill January 31, 2023 16:29

openshift-ci bot added do-not-merge/work-in-progress do-not-merge/hold labels Jan 31, 2023

openshift-ci bot added the approved label Jan 31, 2023

openshift-merge-robot added the needs-rebase label Jan 31, 2023

jcantrill reviewed Feb 1, 2023

View reviewed changes

Metric design

6b211e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: New design #27

WIP: New design #27

vimalk78 commented Jan 31, 2023

openshift-ci bot commented Jan 31, 2023

openshift-merge-robot commented Jan 31, 2023

jcantrill Jan 31, 2023

vimalk78 Feb 9, 2023

jcantrill Jan 31, 2023

vimalk78 Feb 9, 2023

jcantrill Jan 31, 2023

vimalk78 Feb 9, 2023

jcantrill Jan 31, 2023

vimalk78 Feb 9, 2023

jcantrill Jan 31, 2023

vimalk78 Feb 9, 2023

jcantrill Feb 1, 2023

vimalk78 Feb 9, 2023

jcantrill Feb 1, 2023

vimalk78 Feb 9, 2023

jcantrill Feb 1, 2023

vimalk78 Feb 9, 2023

jcantrill Feb 1, 2023

vimalk78 Feb 9, 2023

jcantrill Feb 1, 2023

vimalk78 Feb 9, 2023

alanconway commented Feb 1, 2023

openshift-ci bot commented Feb 9, 2023

WIP: New design #27

Are you sure you want to change the base?

WIP: New design #27

Conversation

vimalk78 commented Jan 31, 2023

openshift-ci bot commented Jan 31, 2023

openshift-merge-robot commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanconway commented Feb 1, 2023

openshift-ci bot commented Feb 9, 2023