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

healthcheckextension: Add an ability to delay collector shutdown #34454

Open
yuri-rs opened this issue Aug 7, 2024 · 5 comments
Open

healthcheckextension: Add an ability to delay collector shutdown #34454

yuri-rs opened this issue Aug 7, 2024 · 5 comments
Labels

Comments

@yuri-rs
Copy link
Contributor

yuri-rs commented Aug 7, 2024

Component(s)

extension/healthcheck, extension/healthcheckv2

Is your feature request related to a problem? Please describe.

We have a problem with 503 errors in the K8s environment for the otel endpoint.
These errors occur during the distribution/change of the collector configuration.
We have a process to restart the collector inside the pod with the new configuration.
During the configuration rollout, the collector shuts down too quickly, so K8s doesn't update the pod's healthy state and continues sending requests to this pod, leading to 503 errors.

Describe the solution you'd like

I'd like to have a way to delay the collector/endpoint shutdown after the HC state changes to NotReady.
This way, pod readiness would catch the NotReady status and move the traffic to the other pods before the actual collector shutdown.

It could look like: yuri-rs@55e0c5a

Describe alternatives you've considered

An alternative could be a delay in the collector service itself, like (added sleep line):

// Shutdown the service. Shutdown will do the following steps in order:
// 1. Notify extensions that the pipeline is shutting down.
// 2. Shutdown all pipelines.
// 3. Shutdown all extensions.
// 4. Shutdown telemetry.
func (srv *Service) Shutdown(ctx context.Context) error {
	// Accumulate errors and proceed with shutting down remaining components.
	var errs error

	// Begin shutdown sequence.
	srv.telemetrySettings.Logger.Info("Starting shutdown...")

	if err := srv.host.serviceExtensions.NotifyPipelineNotReady(); err != nil {
		errs = multierr.Append(errs, fmt.Errorf("failed to notify that pipeline is not ready: %w", err))
	}
	
	time.Sleep(time.Second)

	if err := srv.host.pipelines.ShutdownAll(ctx, srv.reporter); err != nil {
		errs = multierr.Append(errs, fmt.Errorf("failed to shutdown pipelines: %w", err))
	}

However, doing it inside the healthcheck extension seems better to me.

Additional context

What do you think about this feature?
Would you accept a PR with this functionality?

@yuri-rs yuri-rs added enhancement New feature or request needs triage New item requiring triage labels Aug 7, 2024
@github-actions github-actions bot added the extension/healthcheck Health Check Extension label Aug 7, 2024
Copy link
Contributor

github-actions bot commented Aug 7, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member

The current health check extension will give way to the health check v2 extension, which should be ready soon. @mwear, do you remember if this use-case is part of the scope for v2 already?

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Oct 14, 2024
@atoulme
Copy link
Contributor

atoulme commented Oct 14, 2024

I would recommend you write an extension that waits a number of seconds on shutdown. That's an easy thing to maintain and will do better for this use case, no need to tack this on the health check extension imo.

@github-actions github-actions bot removed the Stale label Oct 15, 2024
@atoulme atoulme removed the needs triage New item requiring triage label Oct 15, 2024
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants