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

feat: enable migration testing #1583

Merged
merged 4 commits into from
Oct 21, 2022
Merged

feat: enable migration testing #1583

merged 4 commits into from
Oct 21, 2022

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Oct 10, 2022

  1. I noticed that _pauseOnMigration does not stop the autoscaled pool, keeping the event loop alive. Is this expected?
    Google Meet summary: probably, let's consult with Ondra
    Resolution: it should probably not block the event loop, but there's bunch of other things that are blocking the event loop, such as snapshotter so probably not worth the change
  2. Do we want to E2E test this? Done ✅

Flaky tests encountered (should be a separate issue?):

https://github.com/apify/crawlee/actions/runs/3218900877/jobs/5263614933#step:9:70

FAIL test/core/session_pool/session_pool.test.ts (8.869 s)
  ● SessionPool - testing session pool › should initialize with default values for first time

    ENOENT: no such file or directory, stat '/home/runner/work/crawlee/crawlee/storage/datasets/default/000000001.json.lock'

https://github.com/apify/crawlee/actions/runs/3218900877/jobs/5263615026#step:9:70

FAIL test/core/crawlers/puppeteer_crawler.test.ts (40.046 s)
  ● PuppeteerCrawler › should work

    ENOENT: no such file or directory, stat '/home/runner/work/crawlee/crawlee/storage/datasets/default/000000001.json.lock'

https://github.com/apify/crawlee/actions/runs/3263113405/jobs/5361354845#step:9:82

FAIL test/core/autoscaling/snapshotter.test.ts (8.556 s)
  ● Snapshotter › correctly marks CPU overloaded using OS metrics

    expect(jest.fn()).toBeCalledTimes(expected)

    Expected number of calls: 5
    Received number of calls: 6

      168 |         expect(loopSnapshots[3].isOverloaded).toBe(false);
      169 |         expect(loopSnapshots[4].isOverloaded).toBe(true);
    > 170 |         expect(cpusMock).toBeCalledTimes(5);
          |                          ^
      171 |
      172 |         cpusMock.mockRestore();
      173 |         spy.mockRestore();

      at Object.<anonymous> (test/core/autoscaling/snapshotter.test.ts:170:26)

Closes #1531

@szmarczak szmarczak requested a review from B4nan October 10, 2022 11:18
@B4nan
Copy link
Member

B4nan commented Oct 11, 2022

I just realised, this solution will force users to press ctrl + c twice to shut down the crawler, right? We wanted to allow testing the migrations, not force all users to go through this process. Can we use a different signal/keyboard shortcut, to keep the ctrl + c working as it was?

@szmarczak
Copy link
Contributor Author

Unfortunately, no. Windows supports only SIGINT and SIGBREAK but POSIX supports only SIGINT and SIGTERM. I guess we could listen on both SIGBREAK and SIGTERM, but people would have to press different keyboard shortcuts. Also, SIGTERM by default doesn't have a keyboard shortcut.

@B4nan
Copy link
Member

B4nan commented Oct 12, 2022

but people would have to press different keyboard shortcuts

I want them to use different shortcut. This shouldnt be the default, rather an opt-in. Migrations are rather apify specific thing, so is their testing.

I was thinking about something similar to watch mode in some test runners like vitest, where it listens to your input on prompt (while printing the logs), so you can press anything and map any behavior this way. I hope it could be done easily, if not, I'd at least hide this behind a config toggle and keep it disabled by default.

@metalwarrior665
Copy link
Member

This would support #1531, right? That seems generally useful but maybe people did not yet hit a real need for it to be asking us :)

@B4nan
Copy link
Member

B4nan commented Oct 12, 2022

Yes, so you would be fine with doing this by default? Maybe we should add the same mechanism, with a 10s exit automatically triggered, right now it will wait for the second ctrl+c so you always need to do it twice to actually shut it down. At least that's how I understood it.

@szmarczak
Copy link
Contributor Author

After you press CTRL+C it will gracefully shut down (migration state). If you press CTRL+C again then it will force shutdown.

@szmarczak
Copy link
Contributor Author

This shouldnt be the default, rather an opt-in. Migrations are rather apify specific thing, so is their testing.

We would have to show them how to do this (stty command), I never had the need to add another interrupt signal keyboard shortcuts.

@B4nan
Copy link
Member

B4nan commented Oct 12, 2022

We would have to show them how to do this (stty command), I never had the need to add another interrupt signal keyboard shortcuts.

It woudn't be an interrupt signal, it would be a shortcut to trigger the pause code - it would be a different event, not sigint.

@szmarczak
Copy link
Contributor Author

szmarczak commented Oct 12, 2022

Yep, typo. All signals are interrupt signals. SIGINT means interrupt from keyboard.

@szmarczak
Copy link
Contributor Author

szmarczak commented Oct 17, 2022

Actually SIGTERM cannot be sent via keyboard :( (can be sent via kill though but that would be too complex)

Reverted back to handling just SIGINT

@B4nan B4nan merged commit ee3a68f into master Oct 21, 2022
@B4nan B4nan deleted the migration-testing branch October 21, 2022 19:47
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.

Graceful abort of the runtime process (emulation of Apify platform feature)
3 participants