Add function logs support (#53) #131

life1347 · 2017-02-22T16:56:57Z

This PR add function logs support for issue #53.

Display function logs

  # Currently only influxDB is supported, use --help to see more options
  $ fission function logs --name hello --dbhost http://<ip>:8086

Display function logs from specific pod

  # Currently only influxDB is supported, use --help to see more options
  $ fission function pods --name hello --dbhost http://<ip>:8086
  $ fission function logs --name hello --dbhost http://<ip>:8086 --pod <pod>

Also, this PR implement the log database interface and use fluentd to pipe logs. Developer can implement their own log database solution for storing/searching function logs.

soamvasani · 2017-02-24T00:12:44Z

Awesome stuff, @life1347 !! Excited to have this feature and thanks for thinking it through and talking about it in the issue. I'll review and make comments.

soamvasani

Hey @life1347 ,
Sorry for the review delay. This is a pretty big PR :)

I've suggested lots of changes and most of them can be deferred to later PRs. However the suggestions for the code inside poolmgr/gp.go specializePod are more important, I think they should be addressed before merge.

Thanks again for doing this work. It's going to improve the experience of using fission for lots of people.

soamvasani · 2017-02-24T00:14:51Z

environments/fluentd/build.sh

@@ -0,0 +1,2 @@
+#!/bin/sh
+docker build -t fission-daemonset-fulentd:latest .


typo (fluentd)

soamvasani · 2017-02-24T22:28:34Z

environments/fluentd/fluent.conf

+
+<match fission.**>
+  type record_reformer
+  enable_ruby true


Looks like this can be set to false?

soamvasani · 2017-02-24T22:29:23Z

environments/fluentd/fluent.conf

+    namespace ${tag_parts[4]}
+    pod       ${tag_parts[5]}
+    container ${tag_parts[6]}
+    funcuid   ${tag_parts[7]}


Can we add the function name as well? Seems like it would be useful.

soamvasani · 2017-02-25T00:28:20Z

environments/fluentd/fluent.conf

+    sequence_tag _seq
+    buffer_type memory
+    buffer_chunk_limit 524288 # 512 * 1024
+    buffer_queue_limit 1024


Do lines 39-41 mean that in the worst case we can end up using 512*1024*1024 i.e 512M of memory? That's a lot. Maybe we should use buffer_type file and a smaller buffer_queue_limit?

soamvasani · 2017-02-25T00:32:38Z

environments/fluentd/run.sh

@@ -0,0 +1,23 @@
+#!/bin/sh


can you add a comment to this file saying where it's originally from?

soamvasani · 2017-03-02T00:29:51Z

poolmgr/gp.go

@@ -262,6 +262,18 @@ func (gp *GenericPool) specializePod(pod *v1.Pod, metadata *fission.Metadata) er
 		return errors.New(fmt.Sprintf("Error from fetcher: %v", resp.Status))
 	}

+	logRequest := fmt.Sprintf("{\"namespace\":\"%s\",\"pod\":\"%s\",\"container\":\"%s\",\"funcuid\":\"%s\"}", pod.Namespace, pod.Name, gp.env.Metadata.Name, metadata.Uid)


can we have a struct and then marshal it with json.Marshal?

soamvasani · 2017-03-02T00:32:23Z

poolmgr/gp.go

+		// Failed
+		resp.Body.Close()
+		if err == nil {
+			return errors.New(fmt.Sprintf("Error from fetcher: %v", resp.Status))


error from logging helper, not fetcher

we should make this error not fatal

do we need to do this synchronously? anything in specializePod adds to cold start time -- can we do this http post in a goroutine?

soamvasani · 2017-03-02T00:33:52Z

poolmgr/gp.go

+	loggerUrl := fmt.Sprintf("http://%s:1234/v1/log/%s", pod.Spec.NodeName, pod.Name)
+	req, err := http.NewRequest("DELETE", loggerUrl, nil)
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil || resp.StatusCode != 200 {


this error should not be fatal; we should still continue trying to clean up

soamvasani · 2017-03-02T00:35:29Z

fission/logdb/logdb.go

+	log.WithFields(log.Fields{
+		"FISSION_LOGDB_URL": cnf.Endpoint,
+	}).Fatalf("FISSION_LOGDB_URL is incorrect, now only support %s", INFLUXDB)
+	return dummyDB{}, nil


Why have a dummyDB? Why not just return nil and an error?

soamvasani · 2017-03-02T00:43:58Z

fission/function.go

+	}(ctx, requestChan, responseChan)
+	for {
+		select {
+		case <-time.After(1 * time.Second):


Hmm, we shouldn't wait a second for the first batch -- how about this: immediately do the first batch, and then either (a) quit if no -f (b) time.sleep if -f.

soamvasani · 2017-03-02T20:36:01Z

Oh I forgot to say: could you add the diagram from the issue into the Documentation/ directory in this PR? It's helpful to anyone trying to understand the architecture.

Also, we're trying to keep the environments/ directory for languages (I know fetcher is there, it should be moved out too, but later). Could you move logger to the top level?

atiwari · 2017-03-03T18:36:13Z

Wondering when this feature will be merged?

life1347 · 2017-03-04T11:20:25Z

Hi @soamvasani, I've pushed the commit with following modification.

Move logger to top-level
Integrate logger with fission-bundle
Add influxdb deployment
Fix minor issues

And thanks for the code review. :D

life1347 · 2017-03-04T15:06:37Z

Don't merge it right now, seems the logger failed to create symlink with latest commit. I am investigating it.

life1347 · 2017-03-04T16:20:43Z

With minikube, pod may fail to resolve node name (kubernetes/minikube#757). In the latest commit, poolmgr use host ip instead of node name to prevent dns problem.
And thanks for @yqf3139's help/test. :)

soamvasani

Thanks @life1347 , this is in pretty good shape. Just a few small requests below. If you choose not to address some of the feedback in this PR, you can file issues for them.

soamvasani · 2017-03-10T21:32:56Z

logger/types.go

+		ContainerID string `json:"-"`
+	}
+
+	logRequestTacker struct {


typo: logRequestTracker (missing 'r')

soamvasani · 2017-03-10T23:08:06Z

fission-logger.yaml

+apiVersion: v1
+kind: Service
+metadata:
+  name: controller


Is this here intentionally? it seems to just be a duplicate of the controller svc definition.

soamvasani · 2017-03-11T00:11:25Z

fission-logger.yaml

+              value: "8086"
+            - name: INFLUXDB_DBNAME
+              value: "fissionFunctionLog"
+            - name: INFLUXDB_USERNAME


So to be clear, this username and password is the only thing in this file the user needs to edit, right? Can you just add a comment here?

soamvasani · 2017-03-11T01:09:04Z

README.md

+
+```
+  # Currently only influxDB is supported, use --help to see more options
+  $ export FISSION_LOGDB=http://$(kubectl --namespace fission get svc influxdb -o=jsonpath='{..clusterIP}'):8086


I don't understand why clusterIP would work here? The clusterIP of a service is usually not accessible from outside the cluster.

soamvasani · 2017-03-11T01:10:36Z

fission/function.go

+		fatal("Need name of function, use --name")
+	}
+
+	dbHost := c.String("dbhost")


We should also all users to use env var here, instead of specifying this every time (also ok to do in a later change)

soamvasani · 2017-03-11T01:18:26Z

poolmgr/gp.go

+		return err
+	}
+
+	loggerUrl := fmt.Sprintf("http://%s:1234/v1/log/%s", pod.Spec.NodeName, pod.Name)


This port should be configurable (as an env var); but we can do that in a later change.

I'll add the env support in later PR with changes that cli gets log stream from controller instead of connecting to database directly.

soamvasani · 2017-03-11T01:19:51Z

poolmgr/gp.go

@@ -262,6 +264,31 @@ func (gp *GenericPool) specializePod(pod *v1.Pod, metadata *fission.Metadata) er
 		return errors.New(fmt.Sprintf("Error from fetcher: %v", resp.Status))
 	}

+	logReq := logger.LogRequest{


can you move this whole thing into a separate function? So we can just have gp.setupLogging(pod) here

Sorry I should have been more clear -- I meant the whole functionality of setting up the logger, not just the one statement (upto line 285)

soamvasani · 2017-03-15T06:47:17Z

So I'm trying to set this up locally to test it out, and I'm not sure I understand the instructions -- after the influxdb username and password are created, how does one retrieve them?

life1347 · 2017-03-16T13:15:01Z

Yes, that's the drawback of the current design. A user needs to know the username/password in advance when tries to access function logs.
I'm working on the feature that controller helps to aggregate logs and return log stream to the user, so that users do not need to know the sensitive info when they try to access logs.

soamvasani · 2017-03-16T14:46:21Z

Yes, I understand the drawback and I'm ok with it, I'm just trying to understand how to get it working on my setup :) Once I run the db, where do I get the credentials from?

1. Move logger to top-level 2. Integrate logger with fission-bundle 3. Add influxdb deployment 4. Fix minor issues

1. Modify README.md for logger usage 2. Retrieve logdb host from env 3. Fix minor issues

soamvasani · 2017-03-22T18:45:31Z

(I rebased life1347/function-logs-support to fission/master)

Also make the instructions a bit clearer.

soamvasani reviewed Mar 2, 2017

View reviewed changes

life1347 force-pushed the function-logs-support branch 2 times, most recently from 51f1e68 to 13dd5ec Compare March 4, 2017 11:10

soamvasani requested changes Mar 11, 2017

View reviewed changes

soamvasani approved these changes Mar 21, 2017

View reviewed changes

life1347 added 4 commits March 22, 2017 11:40

Add function logs support (fission#53)

bd98401

Fixed issues

57e5196

1. Move logger to top-level 2. Integrate logger with fission-bundle 3. Add influxdb deployment 4. Fix minor issues

Fixed minikube failed to resolve nodename

a0ca99a

Fixed issues

fd5b99c

1. Modify README.md for logger usage 2. Retrieve logdb host from env 3. Fix minor issues

soamvasani force-pushed the function-logs-support branch from 4ea06fc to fd5b99c Compare March 22, 2017 18:41

soamvasani added 4 commits March 22, 2017 13:22

Remove unused import

e4ab160

Update fluentd image location

bc1720c

Also make the instructions a bit clearer.

Move install instructions into INSTALL.md

1cec70f

Move logging setup code into gp.setupLogging()

e8b6f89

soamvasani merged commit a13015e into fission:master Mar 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function logs support (#53) #131

Add function logs support (#53) #131

life1347 commented Feb 22, 2017

soamvasani commented Feb 24, 2017

soamvasani left a comment

soamvasani Feb 24, 2017

soamvasani Feb 24, 2017

soamvasani Feb 24, 2017

soamvasani Feb 25, 2017

soamvasani Feb 25, 2017

soamvasani Mar 2, 2017

soamvasani Mar 2, 2017

soamvasani Mar 2, 2017

soamvasani Mar 2, 2017

soamvasani Mar 2, 2017

soamvasani commented Mar 2, 2017

atiwari commented Mar 3, 2017

life1347 commented Mar 4, 2017 •

edited

life1347 commented Mar 4, 2017 •

edited

life1347 commented Mar 4, 2017

soamvasani left a comment

soamvasani Mar 10, 2017

soamvasani Mar 10, 2017

soamvasani Mar 11, 2017

soamvasani Mar 11, 2017

soamvasani Mar 11, 2017

soamvasani Mar 11, 2017

life1347 Mar 11, 2017 •

edited

soamvasani Mar 11, 2017

soamvasani Mar 15, 2017

soamvasani commented Mar 15, 2017

life1347 commented Mar 16, 2017

soamvasani commented Mar 16, 2017

soamvasani commented Mar 22, 2017

		@@ -0,0 +1,2 @@
		#!/bin/sh
		docker build -t fission-daemonset-fulentd:latest .

Add function logs support (#53) #131

Add function logs support (#53) #131

Conversation

life1347 commented Feb 22, 2017

soamvasani commented Feb 24, 2017

soamvasani left a 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

soamvasani commented Mar 2, 2017

atiwari commented Mar 3, 2017

life1347 commented Mar 4, 2017 • edited

life1347 commented Mar 4, 2017 • edited

life1347 commented Mar 4, 2017

soamvasani left a 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

life1347 Mar 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soamvasani commented Mar 15, 2017

life1347 commented Mar 16, 2017

soamvasani commented Mar 16, 2017

soamvasani commented Mar 22, 2017

life1347 commented Mar 4, 2017 •

edited

life1347 commented Mar 4, 2017 •

edited

life1347 Mar 11, 2017 •

edited