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

Single-mode exceptions in isImpassable / Limit launched workers to half of remaining capacity #848

Merged
merged 4 commits into from
Dec 28, 2022

Conversation

abyrd
Copy link
Member

@abyrd abyrd commented Dec 28, 2022

See discussion in comments on #841 and in code comments. Even when access=no|private, there are frequently exceptions for single modes. We want to default to the preexisting behavior of allowing passage (and rely only on edge traversal permissions) whenever we see exception tags that aren't clearly blocking access.

Allows config.maxWorkers() and MAX_WORKERS_PER_CATEGORY to be more
freely set on staging.
@abyrd
Copy link
Member Author

abyrd commented Dec 28, 2022

I have added another unrelated change to this PR to apply different limits when starting large numbers of spot instances. This gives us a version of R5 with both patches applied for immediate testing.

@abyrd abyrd changed the title Handle single-mode exceptions in isImpassable Single-mode exceptions in isImpassable / Limit launched workers to half of remaining capacity Dec 28, 2022
@abyrd
Copy link
Member Author

abyrd commented Dec 28, 2022

Trying out the worker limiting on staging: on running a first analysis I see in the logs:

2022-12-28 12:12:16,549 [qtp1775504837-33] WARN  c.c.a.c.b.Broker - Request for 249 workers is more than half the remaining worker pool capacity.
2022-12-28 12:12:16,549 [qtp1775504837-33] WARN  c.c.a.c.b.Broker - Lowered to 0 on-demand and 123 spot workers.
2022-12-28 12:12:16,551 [qtp1775504837-33] INFO  c.c.c.EC2WorkerLauncher - Requesting 123 spot workers on graph ID '63aa9bc9c84e4152d58fbab2', worker version 'v6.7' for abyrd@conveyal.com (conveyal)

Then, on running a second analysis:

2022-12-28 12:13:33,006 [qtp1775504837-30] WARN  c.c.a.c.b.Broker - Request for 249 workers is more than half the remaining worker pool capacity.
2022-12-28 12:13:33,006 [qtp1775504837-30] WARN  c.c.a.c.b.Broker - Lowered to 0 on-demand and 67 spot workers.
2022-12-28 12:13:33,008 [qtp1775504837-30] INFO  c.c.c.EC2WorkerLauncher - Requesting 67 spot workers on graph ID '63ac10c2ac64097d90768f6f', worker version 'v6.7-15-g7a3689e' for abyrd@conveyal.com (conveyal)

Note that 67 is more than half of the 123 capacity that should have been remaining after the first run. This is probably because there's a time lag in the backend becoming aware of the newly started workers - not all of them had reported in yet when I ran the second job, so workerCatalog.totalWorkerCount() was artificially low. This is a pre-existing problem though, and will only be resolved by a bigger overhaul of the worker pool management.

ansoncfit
ansoncfit previously approved these changes Dec 28, 2022
Copy link
Member

@ansoncfit ansoncfit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revised worker startup logic and node tag checks.

Co-authored-by: Anson Stewart <astewart@conveyal.com>
Copy link
Member

@ansoncfit ansoncfit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@abyrd abyrd merged commit 9cdd402 into dev Dec 28, 2022
@abyrd abyrd deleted the access-mode-exceptions branch December 28, 2022 15:18
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