New issue
Advanced search Search tips

Issue 921842 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

swarming: task_runner fails to handle timeout

Project Member Reported by mar...@chromium.org, Jan 15

Issue description

kill_and_wait() never worked. While fixing is trivial, we should add a proper test to ensure this works.

https://chromium.googlesource.com/infra/luci/luci-py.git/+/fbd748ec330193cfbee4baa2c28345c3588388d0/appengine/swarming/swarming_bot/bot_code/task_runner.py#353
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 15

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/8c73c4599f067823097e2d33f5c8f3d8afaba27b

commit 8c73c4599f067823097e2d33f5c8f3d8afaba27b
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Tue Jan 15 20:01:06 2019

client: rename mock to fake; move swarming fake into its own file

Test only change; no functional change.

These (CIPD, Isolate, Swarming) are fakes, not mocks.

This is purely a cosmetical change, but I want to expand on the swarming fake to
fix task_runner_test, and the incorrect wording was confusing.

Change-Id: I84b48cb417980af5ae0cdc4038cae6e055cc2478
Bug:  921842 
Reviewed-on: https://chromium-review.googlesource.com/c/1412254
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Auto-Submit: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/appengine/swarming/swarming_bot/bot_code/task_runner_test.py
[rename] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/cipdserver_fake.py
[rename] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/httpserver.py
[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/isolated_format_test.py
[rename] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/isolateserver_fake.py
[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/isolateserver_test.py
[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/run_isolated_smoke_test.py
[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/run_isolated_test.py
[modify] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/swarming_test.py
[add] https://crrev.com/8c73c4599f067823097e2d33f5c8f3d8afaba27b/client/tests/swarmingserver_fake.py

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 15

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/55f70ce5ad7eec605742927318868e99245aff2e

commit 55f70ce5ad7eec605742927318868e99245aff2e
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Tue Jan 15 20:11:16 2019

client: lower symlink message to warning

When it is logged as an error on Windows, it shows up in the task's stdout and
breaks many task_runner_test.py test cases on Windows.

These failures make it hard to add more test cases.

Bug:  921842 
Change-Id: I2304ab8bf4955b9332f8bb3c0a1e737a9c039b4a
Reviewed-on: https://chromium-review.googlesource.com/c/1412256
Auto-Submit: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/55f70ce5ad7eec605742927318868e99245aff2e/client/isolateserver.py
[modify] https://crrev.com/55f70ce5ad7eec605742927318868e99245aff2e/client/run_isolated.py

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 15

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/e2326f7d43117dc43524a002e5f9d44f949e1fe2

commit e2326f7d43117dc43524a002e5f9d44f949e1fe2
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Tue Jan 15 21:29:39 2019

swarming_bot: rename fake_swarming, migrate to httpserver

fake_swarming was written a long time ago. Update it to reuse
httpserver, which works well. This enables reducing the amount of
duplicate code. Rename it to use the same naming convention than in
client/tests/.

Reorder the imports in task_runner_test.py to make it clearer what comes from
where.

This work is split out as this separate change, as the next one is to
update task_runner_test.py to use this, instead of mocked HTTP request
expectations, and this one will be fairly involved.

Bug:  921842 
Change-Id: I85086428627287d2ddeb8587f59a541009d10d73
Reviewed-on: https://chromium-review.googlesource.com/c/1412258
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/e2326f7d43117dc43524a002e5f9d44f949e1fe2/appengine/swarming/swarming_bot/bot_code/task_runner_test.py
[delete] https://crrev.com/55f70ce5ad7eec605742927318868e99245aff2e/appengine/swarming/swarming_bot/fake_swarming.py
[modify] https://crrev.com/e2326f7d43117dc43524a002e5f9d44f949e1fe2/appengine/swarming/swarming_bot/main_test.py
[add] https://crrev.com/e2326f7d43117dc43524a002e5f9d44f949e1fe2/appengine/swarming/swarming_bot/swarmingserver_bot_fake.py

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 17 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/fac63cee56e1659cc3e8360f77603aaaac27c28b

commit fac63cee56e1659cc3e8360f77603aaaac27c28b
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Thu Jan 17 00:41:29 2019

swarming_bot: rewrite task_runner_test, task_runner send SIGTERM only once

- task_runner: Do not send SIGTERM more than once, it's unnecessary.
- task_runner_test.py previously used HTTP request level expectations via
  mocking, which had implicit timing assumptions. These timing assumptions
  (especially w.r.t. stdout buffering) didn't hold on Windows, which has a
  different clock resolution. Switching to a fake makes the tests work on
  Windows too. Manually verified.
- Improve the fake Swarming bot API server implementation.
- Skip IO timeout tests on Windows, since the currently flow doesn't guarantee
  cleanup.

This also enables a more seamless migration to a pRPC based Bot API, so it's not
all wasted time.

Manually verified to pass on Windows and macOS.

Bug:  921842 
Change-Id: Ibcaad8b29d7f4c4fb8d16f9fe5d1a1e2ef4f9c86
Reviewed-on: https://chromium-review.googlesource.com/c/1412259
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Auto-Submit: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/fac63cee56e1659cc3e8360f77603aaaac27c28b/appengine/swarming/swarming_bot/bot_code/task_runner.py
[modify] https://crrev.com/fac63cee56e1659cc3e8360f77603aaaac27c28b/appengine/swarming/swarming_bot/bot_code/task_runner_test.py
[modify] https://crrev.com/fac63cee56e1659cc3e8360f77603aaaac27c28b/appengine/swarming/swarming_bot/main_test.py
[modify] https://crrev.com/fac63cee56e1659cc3e8360f77603aaaac27c28b/appengine/swarming/swarming_bot/swarmingserver_bot_fake.py

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 17 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/0c298134234fe93391db406a5a0d70f351248685

commit 0c298134234fe93391db406a5a0d70f351248685
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Thu Jan 17 01:11:05 2019

swarming_bot: disable buffering, fix kill_and_wait()

- Disable buffering when calling run_isolated, this way it reduces the risk of
  latency when sending it to the bot.
- Fix kill_and_wait() to work. It never worked. :(
- Add a unit test to make sure this is handled properly, and manually
  tested on macOS and Windows.

Bug:  921842 
Change-Id: Ieaf3f4cd3346aebc4e3c9ff317d394d7e0a4a165
Reviewed-on: https://chromium-review.googlesource.com/c/1412253
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Auto-Submit: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/0c298134234fe93391db406a5a0d70f351248685/appengine/swarming/swarming_bot/bot_code/task_runner.py
[modify] https://crrev.com/0c298134234fe93391db406a5a0d70f351248685/appengine/swarming/swarming_bot/bot_code/task_runner_test.py

Comment 6 by mar...@chromium.org, Jan 17 (6 days ago)

Status: Fixed (was: Assigned)
Took a bit more work than expected for it's finally fixed! The work done as part of this issue also helped with issue 913953 because the new server API fake will be easier to replace.

Sign in to add a comment