From 721c0f1255ea873c75ad4bdee3b358f001c880fd Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Fri, 28 Jul 2017 12:52:51 +0300 Subject: [PATCH 1/9] Improving erro handling while trying to reserve tasks at async runner Each time when MQ becomes unreachable HTTP GET /tasks returned HTTP 500 and code was not handling this case except expecting networking errors. After that it tried to unmarshal empty response body that caused another sort of an error. This patch triggers error based on http response code, explicitly checking if response code is something unexpected (not HTTP 200 OK). Response status code for /tasks for changed from 202 Accepted to 200 OK according to swagger doc. --- api/runner/async_runner.go | 21 ++++++++++++++------- api/server/server.go | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 0cf0d1f0b..8dcde6c14 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -14,30 +14,37 @@ import ( "sync" "time" + "fmt" "github.com/Sirupsen/logrus" - "github.com/opentracing/opentracing-go" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/common" "github.com/fnproject/fn/api/runner/task" + "github.com/opentracing/opentracing-go" ) func getTask(ctx context.Context, url string) (*models.Task, error) { - // TODO shove this ctx into the request? + ctx, log := common.LoggerWithFields(ctx, logrus.Fields{"runner": "async"}) span, _ := opentracing.StartSpanFromContext(ctx, "get_task") defer span.Finish() - // TODO uh, make a better http client :facepalm: - resp, err := http.Get(url) - if err != nil { - return nil, err - } + req, _ := http.NewRequest("GET", url, nil) + resp, err := http.DefaultClient.Do(req.WithContext(ctx)) defer func() { io.Copy(ioutil.Discard, resp.Body) resp.Body.Close() }() + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusOK { + return nil, errors.New(fmt.Sprintf("Unable to get task. Reason %v", resp.Status)) + } var task models.Task err = json.NewDecoder(resp.Body).Decode(&task) + if err != nil { + log.WithError(err).Error("Unable to decode task from response object") + } if err != nil { return nil, err } diff --git a/api/server/server.go b/api/server/server.go index 5b10e86dc..57827b42e 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -267,7 +267,7 @@ func (s *Server) handleTaskRequest(c *gin.Context) { handleErrorResponse(c, err) return } - c.JSON(http.StatusAccepted, task) + c.JSON(http.StatusOK, task) case "DELETE": body, err := ioutil.ReadAll(c.Request.Body) if err != nil { From 49fe3eb11aa6836302fd2fb5d210c8f263a57515 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Fri, 28 Jul 2017 13:03:47 +0300 Subject: [PATCH 2/9] Fixing FMT errors Do we run go-fmt in CI? --- api/datastore/internal/datastoreutil/metrics.go | 2 +- api/logs/mock.go | 2 +- api/logs/testing/test.go | 2 +- api/mqs/ironmq.go | 2 +- api/mqs/memory.go | 2 +- api/mqs/new.go | 2 +- api/mqs/redis.go | 2 +- api/runner/async_runner_test.go | 2 +- api/runner/drivers/docker/docker.go | 4 ++-- api/runner/drivers/docker/docker_client.go | 2 +- api/runner/drivers/docker/docker_test.go | 2 +- api/runner/runner.go | 4 ++-- api/runner/task.go | 2 +- api/runner/worker.go | 4 ++-- api/server/apps_create.go | 2 +- api/server/apps_delete.go | 2 +- api/server/apps_get.go | 2 +- api/server/apps_list.go | 2 +- api/server/apps_test.go | 2 +- api/server/apps_update.go | 2 +- api/server/call_get.go | 2 +- api/server/call_logs.go | 2 +- api/server/extension_points.go | 2 +- api/server/routes_create_update.go | 2 +- api/server/routes_delete.go | 2 +- api/server/routes_get.go | 2 +- api/server/routes_list.go | 2 +- api/server/runner.go | 6 +++--- api/server/runner_async_test.go | 4 ++-- api/server/server_test.go | 4 ++-- api/server/version.go | 2 +- cli/deploy.go | 2 +- cli/init.go | 2 +- cli/routes.go | 2 +- cli/testfn.go | 2 +- test/fn-api-tests/routes_test.go | 2 +- 36 files changed, 43 insertions(+), 43 deletions(-) diff --git a/api/datastore/internal/datastoreutil/metrics.go b/api/datastore/internal/datastoreutil/metrics.go index 877053c73..89d9df7bc 100644 --- a/api/datastore/internal/datastoreutil/metrics.go +++ b/api/datastore/internal/datastoreutil/metrics.go @@ -3,9 +3,9 @@ package datastoreutil import ( "context" + "github.com/fnproject/fn/api/models" "github.com/jmoiron/sqlx" "github.com/opentracing/opentracing-go" - "github.com/fnproject/fn/api/models" ) func MetricDS(ds models.Datastore) models.Datastore { diff --git a/api/logs/mock.go b/api/logs/mock.go index 3b649b3cb..74b8fd7c4 100644 --- a/api/logs/mock.go +++ b/api/logs/mock.go @@ -2,8 +2,8 @@ package logs import ( "context" - "github.com/pkg/errors" "github.com/fnproject/fn/api/models" + "github.com/pkg/errors" ) type mock struct { diff --git a/api/logs/testing/test.go b/api/logs/testing/test.go index 6f61a785e..4b8bfc0f5 100644 --- a/api/logs/testing/test.go +++ b/api/logs/testing/test.go @@ -6,9 +6,9 @@ import ( "testing" "time" - "github.com/go-openapi/strfmt" "github.com/fnproject/fn/api/id" "github.com/fnproject/fn/api/models" + "github.com/go-openapi/strfmt" ) var testApp = &models.App{ diff --git a/api/mqs/ironmq.go b/api/mqs/ironmq.go index 8f4afc3ac..6a260be82 100644 --- a/api/mqs/ironmq.go +++ b/api/mqs/ironmq.go @@ -10,9 +10,9 @@ import ( "sync" "github.com/Sirupsen/logrus" + "github.com/fnproject/fn/api/models" mq_config "github.com/iron-io/iron_go3/config" ironmq "github.com/iron-io/iron_go3/mq" - "github.com/fnproject/fn/api/models" ) type assoc struct { diff --git a/api/mqs/memory.go b/api/mqs/memory.go index 64b5e5d30..8a5d6d0d8 100644 --- a/api/mqs/memory.go +++ b/api/mqs/memory.go @@ -8,9 +8,9 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/google/btree" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/common" + "github.com/google/btree" ) type MemoryMQ struct { diff --git a/api/mqs/new.go b/api/mqs/new.go index ba2bcc08b..fcf8bfad5 100644 --- a/api/mqs/new.go +++ b/api/mqs/new.go @@ -7,8 +7,8 @@ import ( "strings" "github.com/Sirupsen/logrus" - "github.com/opentracing/opentracing-go" "github.com/fnproject/fn/api/models" + "github.com/opentracing/opentracing-go" ) // New will parse the URL and return the correct MQ implementation. diff --git a/api/mqs/redis.go b/api/mqs/redis.go index 9bb2d674b..5d93f6824 100644 --- a/api/mqs/redis.go +++ b/api/mqs/redis.go @@ -10,9 +10,9 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/garyburd/redigo/redis" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/common" + "github.com/garyburd/redigo/redis" ) type RedisMQ struct { diff --git a/api/runner/async_runner_test.go b/api/runner/async_runner_test.go index 14d1907e3..fa57443e6 100644 --- a/api/runner/async_runner_test.go +++ b/api/runner/async_runner_test.go @@ -14,13 +14,13 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api/datastore" "github.com/fnproject/fn/api/logs" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/mqs" "github.com/fnproject/fn/api/runner/drivers" "github.com/fnproject/fn/api/runner/task" + "github.com/gin-gonic/gin" ) func setLogBuffer() *bytes.Buffer { diff --git a/api/runner/drivers/docker/docker.go b/api/runner/drivers/docker/docker.go index 98342b543..2b6b02cb4 100644 --- a/api/runner/drivers/docker/docker.go +++ b/api/runner/drivers/docker/docker.go @@ -15,11 +15,11 @@ import ( "github.com/Sirupsen/logrus" manifest "github.com/docker/distribution/manifest/schema1" + "github.com/fnproject/fn/api/runner/common" + "github.com/fnproject/fn/api/runner/drivers" "github.com/fsouza/go-dockerclient" "github.com/heroku/docker-registry-client/registry" "github.com/opentracing/opentracing-go" - "github.com/fnproject/fn/api/runner/common" - "github.com/fnproject/fn/api/runner/drivers" ) const hubURL = "https://registry.hub.docker.com" diff --git a/api/runner/drivers/docker/docker_client.go b/api/runner/drivers/docker/docker_client.go index f7a0ddb24..9a0ee9d0f 100644 --- a/api/runner/drivers/docker/docker_client.go +++ b/api/runner/drivers/docker/docker_client.go @@ -13,10 +13,10 @@ import ( "time" "github.com/Sirupsen/logrus" + "github.com/fnproject/fn/api/runner/common" "github.com/fsouza/go-dockerclient" "github.com/opentracing/opentracing-go" "github.com/opentracing/opentracing-go/log" - "github.com/fnproject/fn/api/runner/common" ) const ( diff --git a/api/runner/drivers/docker/docker_test.go b/api/runner/drivers/docker/docker_test.go index 4f9734635..f764bb560 100644 --- a/api/runner/drivers/docker/docker_test.go +++ b/api/runner/drivers/docker/docker_test.go @@ -9,9 +9,9 @@ import ( "testing" "time" - "github.com/vrischmann/envconfig" "github.com/fnproject/fn/api/runner/common" "github.com/fnproject/fn/api/runner/drivers" + "github.com/vrischmann/envconfig" ) type taskDockerTest struct { diff --git a/api/runner/runner.go b/api/runner/runner.go index 360a0d641..8da4ca0dd 100644 --- a/api/runner/runner.go +++ b/api/runner/runner.go @@ -14,14 +14,14 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/opentracing/opentracing-go" - "github.com/opentracing/opentracing-go/log" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/common" "github.com/fnproject/fn/api/runner/drivers" "github.com/fnproject/fn/api/runner/drivers/docker" "github.com/fnproject/fn/api/runner/drivers/mock" "github.com/fnproject/fn/api/runner/task" + "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/log" ) // TODO clean all of this up, the exposed API is huge and incohesive, diff --git a/api/runner/task.go b/api/runner/task.go index 5e347b2e4..e8619c997 100644 --- a/api/runner/task.go +++ b/api/runner/task.go @@ -10,9 +10,9 @@ import ( "time" "github.com/docker/cli/cli/config/configfile" - docker "github.com/fsouza/go-dockerclient" "github.com/fnproject/fn/api/runner/drivers" "github.com/fnproject/fn/api/runner/task" + docker "github.com/fsouza/go-dockerclient" ) var registries dockerRegistries diff --git a/api/runner/worker.go b/api/runner/worker.go index 0ad57b13b..70cfe5f24 100644 --- a/api/runner/worker.go +++ b/api/runner/worker.go @@ -9,13 +9,13 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/go-openapi/strfmt" - "github.com/opentracing/opentracing-go" "github.com/fnproject/fn/api/id" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/drivers" "github.com/fnproject/fn/api/runner/protocol" "github.com/fnproject/fn/api/runner/task" + "github.com/go-openapi/strfmt" + "github.com/opentracing/opentracing-go" ) // hot functions - theory of operation diff --git a/api/server/apps_create.go b/api/server/apps_create.go index 0f728e390..7ae46b78f 100644 --- a/api/server/apps_create.go +++ b/api/server/apps_create.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) func (s *Server) handleAppCreate(c *gin.Context) { diff --git a/api/server/apps_delete.go b/api/server/apps_delete.go index e6633451d..07c51bb7e 100644 --- a/api/server/apps_delete.go +++ b/api/server/apps_delete.go @@ -3,10 +3,10 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner/common" + "github.com/gin-gonic/gin" ) func (s *Server) handleAppDelete(c *gin.Context) { diff --git a/api/server/apps_get.go b/api/server/apps_get.go index f2ecab6d0..e1d02a055 100644 --- a/api/server/apps_get.go +++ b/api/server/apps_get.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" + "github.com/gin-gonic/gin" ) func (s *Server) handleAppGet(c *gin.Context) { diff --git a/api/server/apps_list.go b/api/server/apps_list.go index a5b3fadc2..cbf19d8a8 100644 --- a/api/server/apps_list.go +++ b/api/server/apps_list.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) func (s *Server) handleAppList(c *gin.Context) { diff --git a/api/server/apps_test.go b/api/server/apps_test.go index db2045806..6d5c655d7 100644 --- a/api/server/apps_test.go +++ b/api/server/apps_test.go @@ -8,11 +8,11 @@ import ( "testing" "github.com/Sirupsen/logrus" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api/datastore" "github.com/fnproject/fn/api/logs" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/mqs" + "github.com/gin-gonic/gin" ) func setLogBuffer() *bytes.Buffer { diff --git a/api/server/apps_update.go b/api/server/apps_update.go index 796d89da6..e1f7d4f4a 100644 --- a/api/server/apps_update.go +++ b/api/server/apps_update.go @@ -3,9 +3,9 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) func (s *Server) handleAppUpdate(c *gin.Context) { diff --git a/api/server/call_get.go b/api/server/call_get.go index c9a33ece4..cff14cc37 100644 --- a/api/server/call_get.go +++ b/api/server/call_get.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" + "github.com/gin-gonic/gin" ) func (s *Server) handleCallGet(c *gin.Context) { diff --git a/api/server/call_logs.go b/api/server/call_logs.go index 101cd2158..fdfa99522 100644 --- a/api/server/call_logs.go +++ b/api/server/call_logs.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" + "github.com/gin-gonic/gin" ) func (s *Server) handleCallLogGet(c *gin.Context) { diff --git a/api/server/extension_points.go b/api/server/extension_points.go index e329e62c7..9ed12c378 100644 --- a/api/server/extension_points.go +++ b/api/server/extension_points.go @@ -4,9 +4,9 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) type ApiHandlerFunc func(w http.ResponseWriter, r *http.Request) diff --git a/api/server/routes_create_update.go b/api/server/routes_create_update.go index 59c2c1c1a..c13898031 100644 --- a/api/server/routes_create_update.go +++ b/api/server/routes_create_update.go @@ -6,9 +6,9 @@ import ( "path" "strings" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) /* handleRouteCreateOrUpdate is used to handle POST PUT and PATCH for routes. diff --git a/api/server/routes_delete.go b/api/server/routes_delete.go index 057c42ffd..a125f5c6e 100644 --- a/api/server/routes_delete.go +++ b/api/server/routes_delete.go @@ -4,8 +4,8 @@ import ( "net/http" "path" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" + "github.com/gin-gonic/gin" ) func (s *Server) handleRouteDelete(c *gin.Context) { diff --git a/api/server/routes_get.go b/api/server/routes_get.go index f9dc4ab4e..87235fc58 100644 --- a/api/server/routes_get.go +++ b/api/server/routes_get.go @@ -4,8 +4,8 @@ import ( "net/http" "path" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" + "github.com/gin-gonic/gin" ) func (s *Server) handleRouteGet(c *gin.Context) { diff --git a/api/server/routes_list.go b/api/server/routes_list.go index c6ce593b0..4e2add242 100644 --- a/api/server/routes_list.go +++ b/api/server/routes_list.go @@ -3,9 +3,9 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/models" + "github.com/gin-gonic/gin" ) func (s *Server) handleRouteList(c *gin.Context) { diff --git a/api/server/runner.go b/api/server/runner.go index 747cc8c48..46ce41eab 100644 --- a/api/server/runner.go +++ b/api/server/runner.go @@ -12,15 +12,15 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/gin-gonic/gin" - "github.com/go-openapi/strfmt" - cache "github.com/patrickmn/go-cache" "github.com/fnproject/fn/api" "github.com/fnproject/fn/api/id" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/runner" "github.com/fnproject/fn/api/runner/common" "github.com/fnproject/fn/api/runner/task" + "github.com/gin-gonic/gin" + "github.com/go-openapi/strfmt" + cache "github.com/patrickmn/go-cache" ) type runnerResponse struct { diff --git a/api/server/runner_async_test.go b/api/server/runner_async_test.go index 2303c6d90..cc780dfac 100644 --- a/api/server/runner_async_test.go +++ b/api/server/runner_async_test.go @@ -9,12 +9,12 @@ import ( "testing" "time" - "github.com/gin-gonic/gin" - cache "github.com/patrickmn/go-cache" "github.com/fnproject/fn/api/datastore" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/mqs" "github.com/fnproject/fn/api/runner" + "github.com/gin-gonic/gin" + cache "github.com/patrickmn/go-cache" ) func testRouterAsync(ds models.Datastore, mq models.MessageQueue, rnr *runner.Runner, enqueue models.Enqueue) *gin.Engine { diff --git a/api/server/server_test.go b/api/server/server_test.go index a8ca70bbe..3b95e7fd9 100644 --- a/api/server/server_test.go +++ b/api/server/server_test.go @@ -12,12 +12,12 @@ import ( "testing" "time" - "github.com/gin-gonic/gin" - cache "github.com/patrickmn/go-cache" "github.com/fnproject/fn/api/datastore" "github.com/fnproject/fn/api/models" "github.com/fnproject/fn/api/mqs" "github.com/fnproject/fn/api/runner" + "github.com/gin-gonic/gin" + cache "github.com/patrickmn/go-cache" ) var tmpDatastoreTests = "/tmp/func_test_datastore.db" diff --git a/api/server/version.go b/api/server/version.go index 2d9a3d6af..56cff46d9 100644 --- a/api/server/version.go +++ b/api/server/version.go @@ -3,8 +3,8 @@ package server import ( "net/http" - "github.com/gin-gonic/gin" "github.com/fnproject/fn/api/version" + "github.com/gin-gonic/gin" ) func handleVersion(c *gin.Context) { diff --git a/cli/deploy.go b/cli/deploy.go index 833e82698..44c102d4a 100644 --- a/cli/deploy.go +++ b/cli/deploy.go @@ -10,10 +10,10 @@ import ( "path/filepath" "time" + client "github.com/fnproject/fn/cli/client" functions "github.com/funcy/functions_go" "github.com/funcy/functions_go/models" "github.com/urfave/cli" - client "github.com/fnproject/fn/cli/client" ) func deploy() cli.Command { diff --git a/cli/init.go b/cli/init.go index e041dc70f..adabe9c76 100644 --- a/cli/init.go +++ b/cli/init.go @@ -17,8 +17,8 @@ import ( "strings" - "github.com/urfave/cli" "github.com/fnproject/fn/cli/langs" + "github.com/urfave/cli" ) var ( diff --git a/cli/routes.go b/cli/routes.go index 8e1deb64c..9a713fb5e 100644 --- a/cli/routes.go +++ b/cli/routes.go @@ -11,12 +11,12 @@ import ( "strings" "text/tabwriter" + client "github.com/fnproject/fn/cli/client" fnclient "github.com/funcy/functions_go/client" apiroutes "github.com/funcy/functions_go/client/routes" fnmodels "github.com/funcy/functions_go/models" "github.com/jmoiron/jsonq" "github.com/urfave/cli" - client "github.com/fnproject/fn/cli/client" ) type routesCmd struct { diff --git a/cli/testfn.go b/cli/testfn.go index 797209c9e..81105706b 100644 --- a/cli/testfn.go +++ b/cli/testfn.go @@ -12,10 +12,10 @@ import ( "strings" "time" + "github.com/fnproject/fn/cli/client" functions "github.com/funcy/functions_go" "github.com/onsi/gomega" "github.com/urfave/cli" - "github.com/fnproject/fn/cli/client" ) type testStruct struct { diff --git a/test/fn-api-tests/routes_test.go b/test/fn-api-tests/routes_test.go index ea765a831..19b5b1446 100644 --- a/test/fn-api-tests/routes_test.go +++ b/test/fn-api-tests/routes_test.go @@ -4,8 +4,8 @@ package tests import ( "testing" - "github.com/funcy/functions_go/models" "github.com/fnproject/fn/api/id" + "github.com/funcy/functions_go/models" ) func TestRoutes(t *testing.T) { From bb8f12ece98f9eb5993829d8502edbbc40921566 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Fri, 28 Jul 2017 13:32:13 +0300 Subject: [PATCH 3/9] Fixing tests and CI file --- Makefile | 3 +++ api/runner/async_runner.go | 4 ++-- api/runner/async_runner_test.go | 4 ++-- circle.yml | 2 ++ 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 45890400b..777ae347e 100644 --- a/Makefile +++ b/Makefile @@ -13,6 +13,9 @@ build: test: ./test.sh +fmt: + ./go-fmt.sh + test-datastore: cd api/datastore && go test -v ./... diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 8dcde6c14..848a25228 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -27,7 +27,7 @@ func getTask(ctx context.Context, url string) (*models.Task, error) { span, _ := opentracing.StartSpanFromContext(ctx, "get_task") defer span.Finish() - req, _ := http.NewRequest("GET", url, nil) + req, _ := http.NewRequest(http.MethodGet, url, nil) resp, err := http.DefaultClient.Do(req.WithContext(ctx)) defer func() { io.Copy(ioutil.Discard, resp.Body) @@ -37,7 +37,7 @@ func getTask(ctx context.Context, url string) (*models.Task, error) { return nil, err } if resp.StatusCode != http.StatusOK { - return nil, errors.New(fmt.Sprintf("Unable to get task. Reason %v", resp.Status)) + return nil, errors.New(fmt.Sprintf("Unable to get task. Reason: %v", resp.Status)) } var task models.Task diff --git a/api/runner/async_runner_test.go b/api/runner/async_runner_test.go index fa57443e6..c0ad456ae 100644 --- a/api/runner/async_runner_test.go +++ b/api/runner/async_runner_test.go @@ -63,7 +63,7 @@ func getTestServer(mockTasks []*models.Task) *httptest.Server { c.JSON(http.StatusInternalServerError, err) return } - c.JSON(http.StatusAccepted, task) + c.JSON(http.StatusOK, task) } delHandler := func(c *gin.Context) { @@ -120,7 +120,7 @@ func TestGetTaskError(t *testing.T) { { "url": "/invalid", "task": getMockTask(), - "error": "json: cannot unmarshal number into Go value of type models.Task", // TODO WTF! + "error": "Unable to get task. Reason: 404 Not Found", }, } diff --git a/circle.yml b/circle.yml index 6eaf54796..22f2bf84e 100644 --- a/circle.yml +++ b/circle.yml @@ -27,6 +27,8 @@ dependencies: test: override: + - make fmt: + pwd: $GO_PROJECT - make test: pwd: $GO_PROJECT - make test-build-arm: From 2d7f54bffeafdaf65a1e8667544e8f2e5c0c92fa Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Fri, 28 Jul 2017 22:08:01 +0300 Subject: [PATCH 4/9] Addressing comments --- api/runner/async_runner.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 848a25228..6239eb7f3 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -23,7 +23,6 @@ import ( ) func getTask(ctx context.Context, url string) (*models.Task, error) { - ctx, log := common.LoggerWithFields(ctx, logrus.Fields{"runner": "async"}) span, _ := opentracing.StartSpanFromContext(ctx, "get_task") defer span.Finish() @@ -42,9 +41,6 @@ func getTask(ctx context.Context, url string) (*models.Task, error) { var task models.Task err = json.NewDecoder(resp.Body).Decode(&task) - if err != nil { - log.WithError(err).Error("Unable to decode task from response object") - } if err != nil { return nil, err } From f1e46ebfe3b981b932931c0203afa81e7d9cdc67 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Fri, 28 Jul 2017 22:12:41 +0300 Subject: [PATCH 5/9] Using better HTTP client --- api/runner/async_runner.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 6239eb7f3..68b60cb53 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "crypto/tls" "fmt" "github.com/Sirupsen/logrus" "github.com/fnproject/fn/api/models" @@ -23,11 +24,26 @@ import ( ) func getTask(ctx context.Context, url string) (*models.Task, error) { + // TODO shove this ctx into the request? span, _ := opentracing.StartSpanFromContext(ctx, "get_task") defer span.Finish() req, _ := http.NewRequest(http.MethodGet, url, nil) - resp, err := http.DefaultClient.Do(req.WithContext(ctx)) + var client = &http.Client{ + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + Dial: (&net.Dialer{ + Timeout: 10 * time.Second, + KeepAlive: 120 * time.Second, + }).Dial, + MaxIdleConnsPerHost: 512, + TLSHandshakeTimeout: 10 * time.Second, + TLSClientConfig: &tls.Config{ + ClientSessionCache: tls.NewLRUClientSessionCache(4096), + }, + }, + } + resp, err := client.Do(req.WithContext(ctx)) defer func() { io.Copy(ioutil.Discard, resp.Body) resp.Body.Close() From 10566ba1a8a61660ba3551c4cadf20fb7499a9ce Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Sat, 29 Jul 2017 13:52:34 +0300 Subject: [PATCH 6/9] Use better http client while deleting task --- api/runner/async_runner.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 68b60cb53..fac13f451 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -29,7 +29,7 @@ func getTask(ctx context.Context, url string) (*models.Task, error) { defer span.Finish() req, _ := http.NewRequest(http.MethodGet, url, nil) - var client = &http.Client{ + client := &http.Client{ Transport: &http.Transport{ Proxy: http.ProxyFromEnvironment, Dial: (&net.Dialer{ @@ -105,8 +105,22 @@ func deleteTask(ctx context.Context, url string, task *models.Task) error { if err != nil { return err } - c := &http.Client{} - if resp, err := c.Do(req); err != nil { + client := &http.Client{ + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + Dial: (&net.Dialer{ + Timeout: 10 * time.Second, + KeepAlive: 120 * time.Second, + }).Dial, + MaxIdleConnsPerHost: 512, + TLSHandshakeTimeout: 10 * time.Second, + TLSClientConfig: &tls.Config{ + ClientSessionCache: tls.NewLRUClientSessionCache(4096), + }, + }, + } + + if resp, err := client.Do(req); err != nil { return err } else if resp.StatusCode != http.StatusAccepted { body, err := ioutil.ReadAll(resp.Body) From 7c22468b811b88e1ece1f85f855d03e823aded2b Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Mon, 31 Jul 2017 21:15:08 +0300 Subject: [PATCH 7/9] Addressing comments --- api/runner/async_runner.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index fac13f451..7475cd670 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -44,13 +44,13 @@ func getTask(ctx context.Context, url string) (*models.Task, error) { }, } resp, err := client.Do(req.WithContext(ctx)) + if err != nil { + return nil, err + } defer func() { io.Copy(ioutil.Discard, resp.Body) resp.Body.Close() }() - if err != nil { - return nil, err - } if resp.StatusCode != http.StatusOK { return nil, errors.New(fmt.Sprintf("Unable to get task. Reason: %v", resp.Status)) } From 333fc66906b618ddbf62ef7d1cd074d2a2bba6fd Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Mon, 31 Jul 2017 21:22:12 +0300 Subject: [PATCH 8/9] Making http client global to async runner --- api/runner/async_runner.go | 43 +++++++++++++------------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index 7475cd670..e0dc79b09 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -23,26 +23,27 @@ import ( "github.com/opentracing/opentracing-go" ) +var client = &http.Client{ + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + Dial: (&net.Dialer{ + Timeout: 10 * time.Second, + KeepAlive: 120 * time.Second, + }).Dial, + MaxIdleConnsPerHost: 512, + TLSHandshakeTimeout: 10 * time.Second, + TLSClientConfig: &tls.Config{ + ClientSessionCache: tls.NewLRUClientSessionCache(4096), + }, + }, +} + func getTask(ctx context.Context, url string) (*models.Task, error) { // TODO shove this ctx into the request? span, _ := opentracing.StartSpanFromContext(ctx, "get_task") defer span.Finish() req, _ := http.NewRequest(http.MethodGet, url, nil) - client := &http.Client{ - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - Dial: (&net.Dialer{ - Timeout: 10 * time.Second, - KeepAlive: 120 * time.Second, - }).Dial, - MaxIdleConnsPerHost: 512, - TLSHandshakeTimeout: 10 * time.Second, - TLSClientConfig: &tls.Config{ - ClientSessionCache: tls.NewLRUClientSessionCache(4096), - }, - }, - } resp, err := client.Do(req.WithContext(ctx)) if err != nil { return nil, err @@ -105,20 +106,6 @@ func deleteTask(ctx context.Context, url string, task *models.Task) error { if err != nil { return err } - client := &http.Client{ - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - Dial: (&net.Dialer{ - Timeout: 10 * time.Second, - KeepAlive: 120 * time.Second, - }).Dial, - MaxIdleConnsPerHost: 512, - TLSHandshakeTimeout: 10 * time.Second, - TLSClientConfig: &tls.Config{ - ClientSessionCache: tls.NewLRUClientSessionCache(4096), - }, - }, - } if resp, err := client.Do(req); err != nil { return err From edb1279d57351946ee5b4344284b5e48ae78a884 Mon Sep 17 00:00:00 2001 From: Denis Makogon Date: Mon, 31 Jul 2017 21:37:29 +0300 Subject: [PATCH 9/9] Fixing task deleting to be similar to task getter --- api/runner/async_runner.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/api/runner/async_runner.go b/api/runner/async_runner.go index e0dc79b09..9760164b8 100644 --- a/api/runner/async_runner.go +++ b/api/runner/async_runner.go @@ -107,9 +107,16 @@ func deleteTask(ctx context.Context, url string, task *models.Task) error { return err } - if resp, err := client.Do(req); err != nil { + resp, err := client.Do(req) + if err != nil { return err - } else if resp.StatusCode != http.StatusAccepted { + } + defer func() { + io.Copy(ioutil.Discard, resp.Body) + resp.Body.Close() + }() + + if resp.StatusCode != http.StatusAccepted { body, err := ioutil.ReadAll(resp.Body) if err != nil { return err