Skip to content
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.

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

implement registered service healthchecks #238

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

sadlerap
Copy link
Member

@sadlerap sadlerap commented Jul 17, 2023

This implements healthchecks for registered services. A few important notes:

  • The state of registered services now defaults in the CRD to the new state "Unknown", rather than "Available". If a healthcheck is undefined or removed, then the service immediately becomes available, which is in line with old behavior. However, if a healthcheck is defined, we don't want to transition it to "Available", since we haven't run the healthcheck yet.
  • A new field, "minutes", has been added to the healthcheck's container field. It represents how many minutes should be between each check. This is largely introduced for testing purposes, since the planned default of 5 minutes makes acceptance testing a pain. Its value is restricted from 1 to 60.
  • Healthchecks time out after thirty seconds. We need to have some kind of timeout mechanism, since providing users a way to run processes (even if their unpriviledged) has a potential for abuse. Hard-coding it to 30 seconds is fairly reasonable default, but we might want to revisit how we decide this in the future.

@sadlerap sadlerap force-pushed the registered-service-healthchecks branch 5 times, most recently from ea2d231 to 06bb14e Compare July 18, 2023 20:21
api/v1alpha1/types.go Outdated Show resolved Hide resolved
controllers/registeredservice_controller.go Show resolved Hide resolved
controllers/registeredservice_controller.go Show resolved Hide resolved
@sadlerap sadlerap force-pushed the registered-service-healthchecks branch 2 times, most recently from bc4b028 to 55626e7 Compare July 19, 2023 19:41
Copy link
Member

@dperaza4dustbit dperaza4dustbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great approach, good implementation so far

@sadlerap sadlerap force-pushed the registered-service-healthchecks branch 3 times, most recently from 8c8aba5 to a66cb7a Compare July 21, 2023 13:59
Copy link
Member

@dperaza4dustbit dperaza4dustbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

controllers/registeredservice_controller.go Show resolved Hide resolved
This implements healthchecks for registered services.  A few important
notes:
- The state of registered services now defaults in the CRD to the new
  state "Unknown", rather than "Available".  If a healthcheck is
  undefined or removed, then the service immediately becomes available,
  which is in line with old behavior.  However, if a healthcheck is
  defined, we don't want to transition it to "Available", since we
  haven't run the healthcheck yet.
- A new field, "minutes", has been added to the healthcheck's container
  field.  It represents how many minutes should be between each check.
  This is largely introduced for testing purposes, since the planned
  default of 5 minutes makes acceptance testing a pain.  Its value is
  restricted from 1 to 60.
- Healthchecks time out after thirty seconds.  We need to have some kind
  of timeout mechanism, since providing users a way to run processes
  (even if their unpriviledged) has a potential for abuse.  Hard-coding
  it to 30 seconds is fairly reasonable default, but we might want to
  revisit how we decide this in the future.

Signed-off-by: Andy Sadler <ansadler@redhat.com>
Copy link
Member

@filariow filariow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@filariow filariow merged commit 6aedf75 into primaza:main Jul 21, 2023
47 checks passed
@sadlerap sadlerap deleted the registered-service-healthchecks branch July 21, 2023 15:59
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.

None yet

3 participants