New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Platforms connection in health-check #325

Merged

dotchev merged 36 commits into master from platform-health

Sep 12, 2019

Member

DimitarPetrov commented Aug 27, 2019 •

edited

Loading

Motivation

Connection with platforms should be included in the health-check so it is easy to figure out if any state is propagated to a platform.

Approach

A new indicator for all platforms registered in SM is introduced.

...
health:
  indicators:
    storage:
      fatal: true
      failures_threshold: 3
      interval: 30s
    platforms:
      fatal: true
      failures_threshold: 3
      interval: 30s
...

And health-check response for such a configuration looks like this:

{
  "details": {
    "platforms": {
      "check_time": "2019-09-11T16:17:54.121789+03:00",
      "details": {
        "asd": {
          "details": {
            "since": "0001-01-01T00:00:00Z",
            "type": "cloudfoundry"
          },
          "status": "DOWN"
        },
        "asdasd": {
          "details": {
            "since": "0001-01-01T00:00:00Z",
            "type": "cloudfoundry"
          },
          "status": "DOWN"
        },
        "asdf": {
          "details": {
            "since": "0001-01-01T00:00:00Z",
            "type": "kubernetes"
          },
          "status": "DOWN"
        }
      },
      "error": "there are 3 inactive platforms",
      "fatal": true,
      "first_failure_at": "2019-09-11T16:17:54.121889+03:00",
      "name": "platforms",
      "num_failures": 1,
      "status": "DOWN"
    },
    "storage": {
      "check_time": "2019-09-11T16:17:54.117632+03:00",
      "fatal": true,
      "first_failure_at": "0001-01-01T00:00:00Z",
      "name": "storage",
      "num_failures": 0,
      "status": "UP"
    }
  },
  "status": "UP"
}

DimitarPetrov added 24 commits

August 21, 2019 09:13


          Async health check

4139ea3


          Tests adaptation

144ea91


          Make health configurable externally

61157b4


          Install health later

9f62a81


          Export ConvertStatus function

81ff665


          Fix health settings validation

115e2a4


          Attach logger to health

37f9bc4


          Address PR comments

4f21ed9


          Configuration per indicator and refactoring

421d88f


          Add health status listener

30ab52e


          Minor tweaks

d25a87d


          Fix indicator interval type

794c3b0


          Fix tests

40fb02b


          Remove unused import

37c03dd


          Extract indicator configuration and address PR comments

f138807


          Add error to panic

ce4d4bb


          Address PR comments

3a34381


          minor fix

91eb2a9


          Add active status to platform

31f86b7


          Platforms connection in health

0ac6fb5


          Merge branch 'master' into platform-health

17a1bf7


          Add tests

7ffd051


          Fix formatting

9621fb0


          Merge branch 'master' into platform-health

743afcf

DimitarPetrov added the 👋request review label

coveralls commented Aug 27, 2019 •

edited

Loading

Coverage increased (+0.2%) to 89.626% when pulling cfd8a90 on platform-health into 7b05398 on master.

DimitarPetrov added 4 commits

August 27, 2019 18:00


          Hide platform details on unauthorized healthcheck

a2ba4fe


          Rename migrations

89ab1ff


          Fix test last migration

b04b750


          Add default value to migration

dfe900f

DimitarPetrov added 2 commits

August 29, 2019 12:21


          Last Active default value

3e864fd


          Initial fix health configuration

46bc9c5

DimitarPetrov added 🚧 WIP 🚧 and removed 👋request review labels


          fix failing tests

aace011

KirilKabakchiev added 👋request review and removed 🚧 WIP 🚧 labels

dpanayotov reviewed

View reviewed changes

pkg/health/types.go Outdated

+              // but later not registered nothing will happen.
+              var indicatorNames = [...]string{
+              	StorageIndicatorName,
+              	"kubernetes" + PlatformIndicatorSuffix,

Member

dpanayotov Sep 4, 2019

SM must not care about different platform types in the code. These should be a configuration

Member

KirilKabakchiev Sep 4, 2019 •

edited

Loading

If we don't mention the indicator names here somehow then they cannot be configured via the configuration

Contributor

dotchev commented Sep 10, 2019

Describe in the Approach section the meaning of failures_threshold and interval properties

Contributor

dotchev commented Sep 10, 2019

Any documentation for this feature?

Member Author

DimitarPetrov commented Sep 10, 2019

Describe in the Approach section the meaning of failures_threshold and interval properties

This properties are part of different change which introduces async healthcheck.
This is the PR #301 which introduces them.

dotchev reviewed

View reviewed changes

Contributor

dotchev left a comment

Instead of using separate health indicators for each platform type, it would be simpler to have one indicator for all platforms and in its details list the state of each platform together with its type.
Leave SM wrappers decide which platform status is fatal. This decision may not be based on the platform type.

api/healthcheck/healthcheck_controller.go Outdated

@@ @@ -17,10 +17,12 @@ @@
               package healthcheck
               import (
+              	"context"
               	h "github.com/InVisionApp/go-health"

Contributor

dotchev Sep 10, 2019

h alias is too short for such a wide scope

api/healthcheck/healthcheck_controller.go Outdated

               	return util.NewJSONResponse(status, healthResult)
               }
-              func (c *controller) aggregate(overallState map[string]h.State) *health.Health {
+              func (c *controller) aggregate(ctx context.Context, overallState map[string]h.State) *health.Health {

Contributor

dotchev Sep 10, 2019

"overall" suggests the state is already aggregated, better rename it

api/healthcheck/healthcheck_controller.go Outdated

               	details := make(map[string]interface{})
               	for name, state := range overallState {
               		state.Status = convertStatus(state.Status)
+              		if strings.Contains(name, health.PlatformIndicatorSuffix) && !web.IsAuthorized(ctx) {

Contributor

dotchev Sep 10, 2019

avoid parsing strings as it is error prone, better store this data in a structured way

Contributor

dotchev Sep 10, 2019

why not remove the details for all the health indicators in case the request is not authorised?

api/healthcheck/healthcheck_controller.go

               	details := make(map[string]interface{})
               	for name, state := range overallState {
               		state.Status = convertStatus(state.Status)

Contributor

dotchev Sep 10, 2019

why remap the status?

Member Author

DimitarPetrov Sep 11, 2019

If we decide to adopt library’s status “ok” and “failed” this change should be propagated everywhere since all the components check UP to determine if it is started correctly.

api/healthcheck/healthcheck_controller.go Outdated

		@@ -70,6 +72,9 @@ func (c controller) aggregate(overallState map[string]h.State) health.Health {
		details := make(map[string]interface{})
		for name, state := range overallState {

Contributor

dotchev Sep 10, 2019

why loop a second time over overallState? can't we do it in one loop?

api/healthcheck/platform_indicator.go Outdated

+              		return nil, fmt.Errorf("could not fetch platforms health from storage: %v", err)
+              	}
+              	details := make(map[string]interface{})

Contributor

dotchev Sep 10, 2019

map[string]*health.Health

api/healthcheck/platform_indicator.go Outdated

+              			details[platform.Name] = health.New().WithStatus(health.StatusUp)
+              		} else {
+              			details[platform.Name] = health.New().WithStatus(health.StatusDown).WithDetail("since", platform.LastActive)
+              			err = fmt.Errorf("there is inactive %s platforms", pi.platformType)

Contributor

dotchev Sep 10, 2019

Better include the total number of inactive platforms, e.g. "there are %d inactive %s platforms"

pkg/health/types.go

+              // default settings again, but this defaults could be overridden only via application.yml,
+              // env variables and pflags won't have any effect. If an indicator is specified in this list
+              // but later not registered nothing will happen.
+              var indicatorNames = [...]string{

Contributor

dotchev Sep 10, 2019

what about indicators outside Peripli? who will add them here?

Member Author

DimitarPetrov Sep 11, 2019

They will maintain separate list of indicator names and extend default settings with them while configuring their settings. Other approach is to export this array lets discuss which is better.

storage/postgres/migrations/20190829101500_add_platforms_status.up.sql Outdated

+              BEGIN;
+              ALTER TABLE platforms ADD COLUMN active boolean NOT NULL DEFAULT '0';
+              ALTER TABLE platforms ADD COLUMN last_active TIMESTAMP NOT NULL DEFAULT '1970-01-01 00:00:00+00';

Contributor

dotchev Sep 10, 2019

that's strange, better use NULL is the platform was never active

Member Author

DimitarPetrov Sep 11, 2019

If we use NULL then in Platform struct we need to use sql.NullString instead of time.Time so rows scanning can be done properly for null values and deal with casting, which for me seems like unnecessary complication but i am okay with both solutions.

storage/postgres/notificator.go Outdated

+              				if err := c.updatePlatform(platform.ID, func(p *types.Platform) {
+              					p.Active = false
+              					p.LastActive = time.Now()
+              				}); err != nil {

Contributor

dotchev Sep 10, 2019

this if is hard to read, break into multiple statements

DimitarPetrov added 5 commits

September 11, 2019 11:30


          Address PR comments

c9db83e


          Refactor platform indicator

11fb13f


          Strip indicator error on unauthorized request

bf23318


          Fix formatting

cfd8a90


          Fix squash tag handling

d2fd944

dotchev approved these changes

View reviewed changes

dotchev merged commit 63c53a2 into master

dotchev deleted the platform-health branch

September 12, 2019 11:50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

👋request review