Skip to content

Conversation

jrussett
Copy link
Contributor

@jrussett jrussett commented May 15, 2023

What is this change about?

Add interval_ms param for health check definitions
This change allows the interval for the health checks to be defined and
configured in the check definition of the LRP. Previously, this value was
always filled in with a default when the health checks were created, now
the checks can define their own interval that can be used when actually
creating the checks (in the executor).

Checks are now of the form:

"check_definition": {
    "checks": [
      {
        "http_check": {
          "port": 8080,
          "path": "/health",
-         "request_timeout_ms": 10000
+         "request_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "readiness_checks": [
      {
        "tcp_check": {
          "port": 8080,
-         "connect_timeout_ms": 10000
+         "connect_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "log_source": ""
  },

Please provide any contextual information.

Associated executor PR: cloudfoundry/executor#80

Check json example adapted from our design document (Publicly Accessible):
https://docs.google.com/document/d/1AS8GzKg3pdAv56nySQTgF4NaqqzrBwmztkL2_8ahKa4/edit?usp=sharing

#185036553

Tag your pair, your PM, and/or team!

@MarcPaquette

jrussett added 2 commits May 9, 2023 22:10
This change allows the interval for the health checks to be defined and
configured in the check definiton of the LRP. Previously, this value was
always filled in with a default when the health checks were created, now
the checks can define their own interval that can be used when actually
creating the checks (in the executor).

Checks are now of the form:
```diff
"check_definition": {
    "checks": [
      {
        "http_check": {
          "port": 8080,
          "path": "/health",
-         "request_timeout_ms": 10000
+         "request_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "readiness_checks": [
      {
        "tcp_check": {
          "port": 8080,
-         "connect_timeout_ms": 10000
+         "connect_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "log_source": ""
  },
```

Check json example adapted from our design document (Publicly Accessible):
https://docs.google.com/document/d/1AS8GzKg3pdAv56nySQTgF4NaqqzrBwmztkL2_8ahKa4/edit?usp=sharing

Signed-off-by: Marc Paquette <[email protected]>

[#185036553](https://www.pivotaltracker.com/story/show/185036553)

Signed-off-by: Josh Russett <[email protected]>
Signed-off-by: Marc Paquette <[email protected]>
We don't need to touch the actual_lrp at all for the interval_ms changes. This
file got updated when we were generating new protobuf stuff for the
changes to the desired_lrp.

This will eventually get updated by the dynamic tracing PRs, so we
aren't missing anything.

[#185036553](https://www.pivotaltracker.com/story/show/185036553)
@jrussett jrussett merged commit 08ff19f into main May 15, 2023
mariash pushed a commit that referenced this pull request May 23, 2023
* Add interval_ms param for health check definitions

This change allows the interval for the health checks to be defined and
configured in the check definiton of the LRP. Previously, this value was
always filled in with a default when the health checks were created, now
the checks can define their own interval that can be used when actually
creating the checks (in the executor).

Checks are now of the form:
```diff
"check_definition": {
    "checks": [
      {
        "http_check": {
          "port": 8080,
          "path": "/health",
-         "request_timeout_ms": 10000
+         "request_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "readiness_checks": [
      {
        "tcp_check": {
          "port": 8080,
-         "connect_timeout_ms": 10000
+         "connect_timeout_ms": 10000,
+         "interval_ms": 123
        },
      }
    ],
    "log_source": ""
  },
```

Check json example adapted from our design document (Publicly Accessible):
https://docs.google.com/document/d/1AS8GzKg3pdAv56nySQTgF4NaqqzrBwmztkL2_8ahKa4/edit?usp=sharing

Signed-off-by: Marc Paquette <[email protected]>

[#185036553](https://www.pivotaltracker.com/story/show/185036553)

Signed-off-by: Josh Russett <[email protected]>
Signed-off-by: Marc Paquette <[email protected]>

* Do not modify actual_lrp

We don't need to touch the actual_lrp at all for the interval_ms changes. This
file got updated when we were generating new protobuf stuff for the
changes to the desired_lrp.

This will eventually get updated by the dynamic tracing PRs, so we
aren't missing anything.

[#185036553](https://www.pivotaltracker.com/story/show/185036553)

---------

Signed-off-by: Josh Russett <[email protected]>
Signed-off-by: Marc Paquette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant