Liveness probe with subservices #481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zolug
added
kind/enhancement
-NSP, IPAM, Proxy and LB register Liveness probe service at custom health server. -FE no longer checks Target Registry Client as part of Readiness. -IPAM, NSP Readiness no longer includes their server status.
Liveness probes in IPAM, NSP, Proxy and LB containers rely on service name "Liveness" to check status.
LionelJouin
reviewed
Nov 20, 2023
docs/components/tapa.md
Outdated
| @@ -56,6 +56,9 @@ MERIDIO_TIMEOUT | time.Duration | timeout of NSM request/close, NSP register/unr | |||
| MERIDIO_DIAL_TIMEOUT | time.Duration | timeout to dial NSMgr | 5s | |||
| MERIDIO_MAX_TOKEN_LIFETIME | time.Duration | maximum lifetime of tokens | 24h | |||
| MERIDIO_LOG_LEVEL | string | Log level | DEBUG | |||
| MERIDIO_NSP_ETRY_TIMEOUT | time.Duration | Timeout of the entries registered in NSP | 30senvconfig:"nsp_entry_timeout"` | |||
There was a problem hiding this comment.
Suggested change
| MERIDIO_NSP_ETRY_TIMEOUT | time.Duration | Timeout of the entries registered in NSP | 30senvconfig:"nsp_entry_timeout"` | |
| MERIDIO_NSP_ETRY_TIMEOUT | time.Duration | Timeout of the entries registered in NSP | 30s"` |
docs/components/tapa.md
Outdated
Comment on lines
87
to
92
| Service | Description | ||
| --- | --- | ||
| Liveness | A unique service to be used by liveness probe to return status, can aggregate other lesser services | ||
| Startup | A unique service to be used by startup probe to return status, can aggregate other lesser services | ||
| Readiness | A unique service to be used by readiness probe to return status, can aggregate other lesser services | ||
| AmbassadorSvc | Monitor status of the Ambassador server |
There was a problem hiding this comment.
Suggested change
| Service | Description | |
| --- | --- | |
| Liveness | A unique service to be used by liveness probe to return status, can aggregate other lesser services | |
| Startup | A unique service to be used by startup probe to return status, can aggregate other lesser services | |
| Readiness | A unique service to be used by readiness probe to return status, can aggregate other lesser services | |
| AmbassadorSvc | Monitor status of the Ambassador server | |
| Service | Probe | Description | |
| --- | --- | --- | |
| AmbassadorSvc | Liveness | Monitor status of the Ambassador server |
pkg/health/const.go
Outdated
| var ProxyLivenessServices []string = []string{NSMEndpointSvc} | ||
| var IPAMReadinessServices []string = []string{NSPCliSvc} | ||
| var IPAMLivenessServices []string = []string{IPAMSvc} | ||
| var NSPReadinessServices []string = []string{} |
There was a problem hiding this comment.
Why keeping the empty sub-services? (NSPReadinessServices, TAPAStartupServices and TAPAReadinessServices)
There was a problem hiding this comment.
Hmm, could be that these are just confusing, so I will remove them.
LionelJouin
reviewed
Nov 20, 2023
TAPA registers health services Liveness, Startup and Readiness, which can be used by kubernetes probes to check the respective status. The evaluation criteria for these services can be changed behind the secenes, without the need of updating the kubernetes probe configuration of the TAPA container. In accordance with the recent changes the example-target checks Ambassador server availability as part of its liveness status.
LionelJouin
reviewed
Nov 21, 2023
Comment on lines
+87
to
+91
| Service | Description | ||
| --- | --- | ||
| Liveness | A unique service to be used by liveness probe to return status, can aggregate other lesser services | ||
| Startup | A unique service to be used by startup probe to return status, can aggregate other lesser services | ||
| Readiness | A unique service to be used by readiness probe to return status, can aggregate other lesser services |
There was a problem hiding this comment.
Not sure why this table is needed
| @@ -115,6 +117,16 @@ func main() { | |||
|
|
|||
| // create and start health server | |||
| sigCtx = health.CreateChecker(sigCtx) | |||
| // add health server services Startup, Readiness and Liveness representing probes | |||
| if err := health.RegisterStartupSubservices(sigCtx); err != nil { | |||
There was a problem hiding this comment.
Why keeping empty probe services?
There was a problem hiding this comment.
Their purpose is to hide implementation, thus not requiring kubernetes probe config changes in the TAPA container (avoiding/limiting user impact).
LionelJouin
approved these changes
Nov 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Note: There's a DR that does not recommend putting evaluation criteria inside startup probe which are related to service operation readiness which later can fail readiness. Thus startup probes do not check for server availability. (Although in case of IPAM and NSP this limitation would no longer stand.)
Issue link
#480
Checklist