From afde8223d63affc5023c363e1bb56a1e8b2f627e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20S=CC=8Ctibrany=CC=81?= Date: Wed, 25 Mar 2020 11:56:15 +0100 Subject: [PATCH 1/2] /ready now returns 200, not 204 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Peter Štibraný --- CHANGELOG.md | 1 + integration/backward_compatibility_test.go | 33 ++++++++++++++++------ integration/e2e/service.go | 2 +- integration/e2ecortex/services.go | 14 ++++----- pkg/cortex/cortex.go | 4 ++- 5 files changed, 36 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a24fa290185..c8986520550 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ * [CHANGE] Utilize separate protos for rule state and storage. Experimental ruler API will not be functional until the rollout is complete. #2226 * [CHANGE] Frontend worker in querier now starts after all Querier module dependencies are started. This fixes issue where frontend worker started to send queries to querier before it was ready to serve them (mostly visible when using experimental blocks storage). #2246 * [CHANGE] Lifecycler component now enters Failed state on errors, and doesn't exit the process. (Important if you're vendoring Cortex and use Lifecycler) #2251 +* [CHANGE] /ready handler now returns 200 instead of 204. * [FEATURE] Added experimental storage API to the ruler service that is enabled when the `-experimental.ruler.enable-api` is set to true #2269 * `-ruler.storage.type` flag now allows `s3`,`gcs`, and `azure` values * `-ruler.storage.(s3|gcs|azure)` flags exist to allow the configuration of object clients set for rule storage diff --git a/integration/backward_compatibility_test.go b/integration/backward_compatibility_test.go index 619ff293f7d..7f1f5ad8aff 100644 --- a/integration/backward_compatibility_test.go +++ b/integration/backward_compatibility_test.go @@ -16,24 +16,34 @@ import ( "github.com/cortexproject/cortex/integration/e2ecortex" ) +type serviceSetupFn func(name string, s *e2ecortex.CortexService) + var ( // If you change the image tag, remember to update it in the preloading done // by CircleCI too (see .circleci/config.yml). - previousVersionImages = []string{ - "quay.io/cortexproject/cortex:v0.6.0", - "quay.io/cortexproject/cortex:v0.7.0", + previousVersionImages = map[string]serviceSetupFn{ + "quay.io/cortexproject/cortex:v0.6.0": func(name string, s *e2ecortex.CortexService) { + // 0.6.0 used 204 status code for querier and ingester + // distributor didn't have /ready page, and we used check on the /ring page instead + s.SetReadinessProbe(e2e.NewHTTPReadinessProbe(s.HTTPPort(), "/ready", 204)) + }, + + "quay.io/cortexproject/cortex:v0.7.0": func(name string, s *e2ecortex.CortexService) { + // 0.7.0 used 204 status code for all components + s.SetReadinessProbe(e2e.NewHTTPReadinessProbe(s.HTTPPort(), "/ready", 204)) + }, } ) func TestBackwardCompatibilityWithChunksStorage(t *testing.T) { - for _, previousImage := range previousVersionImages { + for previousImage, setupFn := range previousVersionImages { t.Run(fmt.Sprintf("Backward compatibility upgrading from %s", previousImage), func(t *testing.T) { - runBackwardCompatibilityTestWithChunksStorage(t, previousImage) + runBackwardCompatibilityTestWithChunksStorage(t, previousImage, setupFn) }) } } -func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage string) { +func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage string, setupFn serviceSetupFn) { s, err := e2e.NewScenario(networkName) require.NoError(t, err) defer s.Close() @@ -61,6 +71,7 @@ func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage s // Start other Cortex components (ingester running on previous version). ingester1 := e2ecortex.NewIngester("ingester-1", consul.NetworkHTTPEndpoint(), flagsForOldImage, previousImage) + setupFn("ingester", ingester1) distributor := e2ecortex.NewDistributor("distributor", consul.NetworkHTTPEndpoint(), flagsForOldImage, "") require.NoError(t, s.StartAndWaitReady(distributor, ingester1)) @@ -90,11 +101,15 @@ func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage s // Query the new ingester both with the old and the new querier. for _, image := range []string{previousImage, ""} { - flags := ChunksStorageFlags + var querier *e2ecortex.CortexService + if image == previousImage { - flags = flagsForOldImage + querier = e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), flagsForOldImage, image) + setupFn("querier", querier) + } else { + querier = e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), ChunksStorageFlags, image) } - querier := e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), flags, image) + require.NoError(t, s.StartAndWaitReady(querier)) // Wait until the querier has updated the ring. diff --git a/integration/e2e/service.go b/integration/e2e/service.go index 8bccf508ba2..661215b9414 100644 --- a/integration/e2e/service.go +++ b/integration/e2e/service.go @@ -393,7 +393,7 @@ func (p *HTTPReadinessProbe) Ready(service *ConcreteService) (err error) { return nil } - return fmt.Errorf("got no expected status code: %v, expected: %v", res.StatusCode, p.expectedStatus) + return fmt.Errorf("got status code: %v, expected: %v", res.StatusCode, p.expectedStatus) } // TCPReadinessProbe checks readiness by ensure a TCP connection can be established. diff --git a/integration/e2ecortex/services.go b/integration/e2ecortex/services.go index 6ab7c866800..503221242d5 100644 --- a/integration/e2ecortex/services.go +++ b/integration/e2ecortex/services.go @@ -48,7 +48,7 @@ func NewDistributorWithConfigFile(name, consulAddress, configFile string, flags "-ring.store": "consul", "-consul.hostname": consulAddress, }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) @@ -83,7 +83,7 @@ func NewQuerierWithConfigFile(name, consulAddress, configFile string, flags map[ "-querier.frontend-client.backoff-retries": "1", "-querier.worker-parallelism": "1", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) @@ -117,7 +117,7 @@ func NewIngesterWithConfigFile(name, consulAddress, configFile string, flags map "-ring.store": "consul", "-consul.hostname": consulAddress, }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) @@ -143,7 +143,7 @@ func NewTableManagerWithConfigFile(name, configFile string, flags map[string]str "-target": "table-manager", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) @@ -169,7 +169,7 @@ func NewQueryFrontendWithConfigFile(name, configFile string, flags map[string]st "-target": "query-frontend", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) @@ -186,7 +186,7 @@ func NewSingleBinary(name string, flags map[string]string, image string, httpPor e2e.NewCommandWithoutEntrypoint("cortex", e2e.BuildArgs(e2e.MergeFlags(map[string]string{ "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, otherPorts..., @@ -205,7 +205,7 @@ func NewAlertmanager(name string, flags map[string]string, image string) *Cortex "-target": "alertmanager", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 204), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), httpPort, grpcPort, ) diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go index c2372fac852..3eb3762c0c2 100644 --- a/pkg/cortex/cortex.go +++ b/pkg/cortex/cortex.go @@ -401,9 +401,11 @@ func (t *Cortex) readyHandler(sm *services.Manager) http.HandlerFunc { if t.ingester != nil { if err := t.ingester.CheckReady(r.Context()); err != nil { http.Error(w, "Ingester not ready: "+err.Error(), http.StatusServiceUnavailable) + return } } - w.WriteHeader(http.StatusNoContent) + + http.Error(w, "ready", http.StatusOK) } } From 9e10056ed83338003656bb75589eaa386e930f23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20S=CC=8Ctibrany=CC=81?= Date: Wed, 25 Mar 2020 13:00:56 +0100 Subject: [PATCH 2/2] HTTP service now takes a range of expected status codes. Updated CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Peter Štibraný --- CHANGELOG.md | 2 +- integration/backward_compatibility_test.go | 28 ++++++++-------------- integration/e2e/db/db.go | 4 ++-- integration/e2e/service.go | 20 +++++++++------- integration/e2ecortex/services.go | 17 +++++++------ 5 files changed, 32 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8986520550..49178f1ad22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,7 @@ * [CHANGE] Utilize separate protos for rule state and storage. Experimental ruler API will not be functional until the rollout is complete. #2226 * [CHANGE] Frontend worker in querier now starts after all Querier module dependencies are started. This fixes issue where frontend worker started to send queries to querier before it was ready to serve them (mostly visible when using experimental blocks storage). #2246 * [CHANGE] Lifecycler component now enters Failed state on errors, and doesn't exit the process. (Important if you're vendoring Cortex and use Lifecycler) #2251 -* [CHANGE] /ready handler now returns 200 instead of 204. +* [CHANGE] `/ready` handler now returns 200 instead of 204. #2330 * [FEATURE] Added experimental storage API to the ruler service that is enabled when the `-experimental.ruler.enable-api` is set to true #2269 * `-ruler.storage.type` flag now allows `s3`,`gcs`, and `azure` values * `-ruler.storage.(s3|gcs|azure)` flags exist to allow the configuration of object clients set for rule storage diff --git a/integration/backward_compatibility_test.go b/integration/backward_compatibility_test.go index 7f1f5ad8aff..58d5f7781b9 100644 --- a/integration/backward_compatibility_test.go +++ b/integration/backward_compatibility_test.go @@ -16,34 +16,28 @@ import ( "github.com/cortexproject/cortex/integration/e2ecortex" ) -type serviceSetupFn func(name string, s *e2ecortex.CortexService) - var ( // If you change the image tag, remember to update it in the preloading done // by CircleCI too (see .circleci/config.yml). - previousVersionImages = map[string]serviceSetupFn{ - "quay.io/cortexproject/cortex:v0.6.0": func(name string, s *e2ecortex.CortexService) { - // 0.6.0 used 204 status code for querier and ingester - // distributor didn't have /ready page, and we used check on the /ring page instead - s.SetReadinessProbe(e2e.NewHTTPReadinessProbe(s.HTTPPort(), "/ready", 204)) - }, - - "quay.io/cortexproject/cortex:v0.7.0": func(name string, s *e2ecortex.CortexService) { - // 0.7.0 used 204 status code for all components - s.SetReadinessProbe(e2e.NewHTTPReadinessProbe(s.HTTPPort(), "/ready", 204)) - }, + previousVersionImages = []string{ + // 0.6.0 used 204 status code for querier and ingester + // distributor didn't have /ready page, and we used check on the /ring page instead + "quay.io/cortexproject/cortex:v0.6.0", + + // 0.7.0 used 204 status code for all components + "quay.io/cortexproject/cortex:v0.7.0", } ) func TestBackwardCompatibilityWithChunksStorage(t *testing.T) { - for previousImage, setupFn := range previousVersionImages { + for _, previousImage := range previousVersionImages { t.Run(fmt.Sprintf("Backward compatibility upgrading from %s", previousImage), func(t *testing.T) { - runBackwardCompatibilityTestWithChunksStorage(t, previousImage, setupFn) + runBackwardCompatibilityTestWithChunksStorage(t, previousImage) }) } } -func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage string, setupFn serviceSetupFn) { +func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage string) { s, err := e2e.NewScenario(networkName) require.NoError(t, err) defer s.Close() @@ -71,7 +65,6 @@ func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage s // Start other Cortex components (ingester running on previous version). ingester1 := e2ecortex.NewIngester("ingester-1", consul.NetworkHTTPEndpoint(), flagsForOldImage, previousImage) - setupFn("ingester", ingester1) distributor := e2ecortex.NewDistributor("distributor", consul.NetworkHTTPEndpoint(), flagsForOldImage, "") require.NoError(t, s.StartAndWaitReady(distributor, ingester1)) @@ -105,7 +98,6 @@ func runBackwardCompatibilityTestWithChunksStorage(t *testing.T, previousImage s if image == previousImage { querier = e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), flagsForOldImage, image) - setupFn("querier", querier) } else { querier = e2ecortex.NewQuerier("querier", consul.NetworkHTTPEndpoint(), ChunksStorageFlags, image) } diff --git a/integration/e2e/db/db.go b/integration/e2e/db/db.go index efc40610faa..de758cddf70 100644 --- a/integration/e2e/db/db.go +++ b/integration/e2e/db/db.go @@ -25,7 +25,7 @@ func NewMinio(port int, bktName string) *e2e.HTTPService { "minio/minio:RELEASE.2019-12-30T05-45-39Z", // Create the "cortex" bucket before starting minio e2e.NewCommandWithoutEntrypoint("sh", "-c", fmt.Sprintf("mkdir -p /data/%s && minio server --address :%v --quiet /data", bktName, port)), - e2e.NewHTTPReadinessProbe(port, "/minio/health/ready", 200), + e2e.NewHTTPReadinessProbe(port, "/minio/health/ready", 200, 200), port, ) m.SetEnvVars(map[string]string{ @@ -78,7 +78,7 @@ func NewDynamoDB() *e2e.HTTPService { "amazon/dynamodb-local:1.11.477", e2e.NewCommand("-jar", "DynamoDBLocal.jar", "-inMemory", "-sharedDb"), // DynamoDB doesn't have a readiness probe, so we check if the / works even if returns 400 - e2e.NewHTTPReadinessProbe(8000, "/", 400), + e2e.NewHTTPReadinessProbe(8000, "/", 400, 400), 8000, ) } diff --git a/integration/e2e/service.go b/integration/e2e/service.go index 661215b9414..27db1ee4f53 100644 --- a/integration/e2e/service.go +++ b/integration/e2e/service.go @@ -361,16 +361,18 @@ type ReadinessProbe interface { // HTTPReadinessProbe checks readiness by making HTTP call and checking for expected HTTP status code type HTTPReadinessProbe struct { - port int - path string - expectedStatus int + port int + path string + expectedStatusRangeStart int + expectedStatusRangeEnd int } -func NewHTTPReadinessProbe(port int, path string, expectedStatus int) *HTTPReadinessProbe { +func NewHTTPReadinessProbe(port int, path string, expectedStatusRangeStart, expectedStatusRangeEnd int) *HTTPReadinessProbe { return &HTTPReadinessProbe{ - port: port, - path: path, - expectedStatus: expectedStatus, + port: port, + path: path, + expectedStatusRangeStart: expectedStatusRangeStart, + expectedStatusRangeEnd: expectedStatusRangeEnd, } } @@ -389,11 +391,11 @@ func (p *HTTPReadinessProbe) Ready(service *ConcreteService) (err error) { defer runutil.ExhaustCloseWithErrCapture(&err, res.Body, "response readiness") - if res.StatusCode == p.expectedStatus { + if p.expectedStatusRangeStart <= res.StatusCode && res.StatusCode <= p.expectedStatusRangeEnd { return nil } - return fmt.Errorf("got status code: %v, expected: %v", res.StatusCode, p.expectedStatus) + return fmt.Errorf("got status code: %v, expected code in range: [%v, %v]", res.StatusCode, p.expectedStatusRangeStart, p.expectedStatusRangeEnd) } // TCPReadinessProbe checks readiness by ensure a TCP connection can be established. diff --git a/integration/e2ecortex/services.go b/integration/e2ecortex/services.go index 503221242d5..1f57d30dfcb 100644 --- a/integration/e2ecortex/services.go +++ b/integration/e2ecortex/services.go @@ -48,7 +48,7 @@ func NewDistributorWithConfigFile(name, consulAddress, configFile string, flags "-ring.store": "consul", "-consul.hostname": consulAddress, }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -83,7 +83,7 @@ func NewQuerierWithConfigFile(name, consulAddress, configFile string, flags map[ "-querier.frontend-client.backoff-retries": "1", "-querier.worker-parallelism": "1", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -117,7 +117,7 @@ func NewIngesterWithConfigFile(name, consulAddress, configFile string, flags map "-ring.store": "consul", "-consul.hostname": consulAddress, }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -143,7 +143,7 @@ func NewTableManagerWithConfigFile(name, configFile string, flags map[string]str "-target": "table-manager", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -169,7 +169,7 @@ func NewQueryFrontendWithConfigFile(name, configFile string, flags map[string]st "-target": "query-frontend", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -186,7 +186,7 @@ func NewSingleBinary(name string, flags map[string]string, image string, httpPor e2e.NewCommandWithoutEntrypoint("cortex", e2e.BuildArgs(e2e.MergeFlags(map[string]string{ "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, otherPorts..., @@ -205,7 +205,7 @@ func NewAlertmanager(name string, flags map[string]string, image string) *Cortex "-target": "alertmanager", "-log.level": "warn", }, flags))...), - e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, ) @@ -223,8 +223,7 @@ func NewRuler(name string, flags map[string]string, image string) *CortexService "-target": "ruler", "-log.level": "warn", }, flags))...), - // The alertmanager doesn't expose a readiness probe, so we just check if the / returns 200 - e2e.NewHTTPReadinessProbe(httpPort, "/", 200), + e2e.NewHTTPReadinessProbe(httpPort, "/ready", 200, 299), httpPort, grpcPort, )