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

Do not use a sandbox for Run #1886

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Do not use a sandbox for Run #1886

merged 2 commits into from
Oct 9, 2024

Conversation

lovromazgon
Copy link
Member

@lovromazgon lovromazgon commented Oct 8, 2024

Description

We run builtin connector functions in a sandbox, making it possible to "detach" from the plugin. In this PR we remove this sandbox for calls to Run, as it prevents us from doing a graceful shutdown.

Shutdown logs before the change
2024-10-08T19:37:19+00:00 ERR stopping source connector error="graceful shutdown" component=SourceNode node_id=file-to-file:example.in pipeline_id=file-to-file stack=[{"file":"/Users/lovro/dev/conduitio/conduit/pkg/pipeline/errors.go","func":"github.com/conduitio/conduit/pkg/pipeline.init","line":21}]
2024-10-08T19:37:19+00:00 INF source connector plugin successfully responded to stop signal component=connector.Source connector_id=file-to-file:example.in record_position=
2024-10-08T19:37:19+00:00 ERR stopping source node error="graceful shutdown" component=SourceNode node_id=file-to-file:example.in pipeline_id=file-to-file record_position=<nil> stack=[{"file":"/Users/lovro/dev/conduitio/conduit/pkg/pipeline/errors.go","func":"github.com/conduitio/conduit/pkg/pipeline.init","line":21}]
2024-10-08T19:37:19+00:00 INF source connector plugin successfully torn down component=connector.Source connector_id=file-to-file:example.in
2024-10-08T19:37:19+00:00 ERR context cancelled while waiting for builtin connector plugin to respond, detaching from plugin component=builtin.sourcePluginAdapter connector_id=file-to-file:example.in plugin_name=file plugin_type=source
2024-10-08T19:37:19+00:00 ERR context cancelled when trying to return response from builtin connector plugin (this message comes from a detached plugin) error="ack stream error: context canceled" component=builtin.sourcePluginAdapter connector_id=file-to-file:example.in plugin_name=file plugin_type=source response=null stack=null
2024-10-08T19:37:19+00:00 INF destination connector plugin successfully torn down component=connector.Destination connector_id=file-to-file-dlq
2024-10-08T19:37:19+00:00 ERR context cancelled while waiting for builtin connector plugin to respond, detaching from plugin component=builtin.destinationPluginAdapter connector_id=file-to-file-dlq plugin_name=builtin:log plugin_type=destination
2024-10-08T19:37:19+00:00 INF destination connector plugin successfully torn down component=connector.Destination connector_id=file-to-file:example.out
2024-10-08T19:37:19+00:00 ERR context cancelled while waiting for builtin connector plugin to respond, detaching from plugin component=builtin.destinationPluginAdapter connector_id=file-to-file:example.out plugin_name=builtin:file plugin_type=destination
2024-10-08T19:37:19+00:00 ERR context cancelled when trying to return response from builtin connector plugin (this message comes from a detached plugin) error="write stream error: context canceled" component=builtin.destinationPluginAdapter connector_id=file-to-file-dlq plugin_name=builtin:log plugin_type=destination response=null stack=null
2024-10-08T19:37:19+00:00 ERR context cancelled when trying to return response from builtin connector plugin (this message comes from a detached plugin) error="write stream error: context canceled" component=builtin.destinationPluginAdapter connector_id=file-to-file:example.out plugin_name=builtin:file plugin_type=destination response=null stack=null
2024-10-08T19:37:19+00:00 INF pipeline stopped component=lifecycle.Service pipeline_id=file-to-file
Shutdown logs after the change
2024-10-08T19:38:44+00:00 INF gracefully stopping pipeline component=lifecycle.Service pipeline_id=file-to-file pipeline_status=1
2024-10-08T19:38:44+00:00 ERR stopping source connector error="graceful shutdown" component=SourceNode node_id=file-to-file:example.in pipeline_id=file-to-file stack=[{"file":"/Users/lovro/dev/conduitio/conduit/pkg/pipeline/errors.go","func":"github.com/conduitio/conduit/pkg/pipeline.init","line":21}]
2024-10-08T19:38:44+00:00 INF source connector plugin successfully responded to stop signal component=connector.Source connector_id=file-to-file:example.in record_position=
2024-10-08T19:38:44+00:00 ERR stopping source node error="graceful shutdown" component=SourceNode node_id=file-to-file:example.in pipeline_id=file-to-file record_position=<nil> stack=[{"file":"/Users/lovro/dev/conduitio/conduit/pkg/pipeline/errors.go","func":"github.com/conduitio/conduit/pkg/pipeline.init","line":21}]
2024-10-08T19:38:44+00:00 INF source connector plugin successfully torn down component=connector.Source connector_id=file-to-file:example.in
2024-10-08T19:38:44+00:00 INF destination connector plugin successfully torn down component=connector.Destination connector_id=file-to-file-dlq
2024-10-08T19:38:44+00:00 INF destination connector plugin successfully torn down component=connector.Destination connector_id=file-to-file:example.out
2024-10-08T19:38:44+00:00 INF pipeline stopped component=lifecycle.Service pipeline_id=file-to-file

When the pipeline stops we cancel the context passed to Run with the intention to close the bi-directional stream opened between the connector and Conduit. However, the sandbox interprets the closed context as the intention that we want to detach from that function and keep it running in the background. In case of Run this doesn't make sense, because it is already running in the background, so we will never want to detach from it.

The second reason for the sandbox is to catch any panics the connector might produce (well at least it's a best effort). Again, in Run this won't make a difference, because the SDK will spawn two separate goroutines for the incoming and outgoing stream (acks and records), so panics that happen in those goroutines won't be caught by the sandbox anyway.

Quick checks

  • I have followed the Code Guidelines.
  • There is no other pull request for the same update/change.
  • I have written unit tests.
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

@lovromazgon lovromazgon requested a review from a team as a code owner October 8, 2024 17:49
@lovromazgon lovromazgon merged commit b429365 into main Oct 9, 2024
3 checks passed
@lovromazgon lovromazgon deleted the lovro/graceful-shutdown branch October 9, 2024 09:40
lovromazgon added a commit that referenced this pull request Oct 10, 2024
Co-authored-by: Haris Osmanagić <haris@meroxa.io>
raulb added a commit that referenced this pull request Nov 7, 2024
* proof of concept for funnel (new stream engine)

* tie code into the lifecycle service to test end-to-end

* expand the implementation

* improve performance

* minimize allocations

* start implementing processor

* ensure graceful stop

* implement processor task

* build worker tasks correctly

* do not use a sandbox for Run (#1886)

Co-authored-by: Haris Osmanagić <haris@meroxa.io>

* make graceful shutdown work

* fix example

* update tests

* fix linter warnings

* rename introduced lifecycle package

* restore original lifecycle package

* fix code after merge

* add feature flag

* go mod tidy

* make acknowledgment fetching from destination stricter

* documentation

* make generate

---------

Co-authored-by: Haris Osmanagić <haris@meroxa.io>
Co-authored-by: Raúl Barroso <ra.barroso@gmail.com>
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.

2 participants