feat: Better handling of Ctrl+C, log failure reason when sync ends abruptly #7756
Conversation
…udquery into send-stats-when-sync-ends
Co-authored-by: Kemal <[email protected]>
yevgenypats
left a comment
yevgenypats
left a comment
There was a problem hiding this comment.
Few questions on the context. still need to give it another review as it's tricky code section.
| opts = append(opts, source.WithNoSentry()) | ||
| } | ||
| sourceClient, err := source.NewClient(ctx, sourceSpec.Registry, sourceSpec.Path, sourceSpec.Version, opts...) | ||
| sourceCtx, sourceCancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
The idea here is that the CLI should manage the context for the plugins. If the CLI receives a signal and its context gets cancelled, we don't want that to bubble down to the plugins, because then we don't have time to fetch metrics first and send a shutdown signal of our own. Instead, the CLI should listen for when its context gets cancelled, do some clean-up, and then cancel the contexts of the plugins
There was a problem hiding this comment.
To add to this: now that cloudquery/plugin-sdk#664 has been merged, the plugins should run in a separate process group, so they won't receive an interrupt signal from the OS when ctrl+c is pressed. However, we still use a command with context which sends a kill signal as soon as the context is cancelled. To avoid this, we will either need to stop using CommandContext, or send a different context like the change in this PR is doing.
| defer log.Info().Str("source", sourceSpec.VersionString()).Strs("destinations", destinationStrings).Time("sync_time", syncTime).Msg("End sync") | ||
|
|
||
| destClients, err := newDestinationClientsV0(ctx, sourceSpec, destinationsSpecs, cqDir) | ||
| destCtx, destCancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Same here? why context.Background ?
|
Is there anything blocking us from merging and releasing this? |
|
@erezrokah It's been a few weeks, so I'd like to just do a quick round of manual testing again to ensure that it all still works as expected. Some of these scenarios, like ctrl+c behavior, are not currently being tested in an automated way (though maybe that would be a good test to add) |
|
I did some more testing and it still looks good, let's merge it |
🤖 I have created a release *beep* *boop* --- ## [2.4.0](cli-v2.3.10...cli-v2.4.0) (2023-02-28) ### Features * Better handling of Ctrl+C, log failure reason when sync ends abruptly ([#7756](#7756)) ([4fbaefe](4fbaefe)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-sdk to v1.39.0 ([#8344](#8344)) ([9c57544](9c57544)) * **deps:** Update module github.com/cloudquery/plugin-sdk to v1.39.1 ([#8371](#8371)) ([e3274c1](e3274c1)) * Update CLI SDK to sync sources in order they appear in config ([#8403](#8403)) ([e36e891](e36e891)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Before, when pressing ctrl+c, these messages would show in the CLI:
This PR fixes it so that these messages no longer appear. This is the main user-facing change.
Other changes include:
CQ_ANALYTICS_HOSTenvironment variable that can override the default host. This is mainly useful for testing, so no code changes are necessary to send analytics to a test server. For normal operation, users should still use--telemetry-levelorCQ_TELEMETRY_LEVELto control the telemetryThis change will be improved by cloudquery/plugin-sdk#664 but doesn't depend on it to work.