Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/buildstream/_scheduler/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,25 +353,26 @@ def _sched_queue_jobs(self):

if self._queue_jobs:
# Scheduler.stop() was not called, consider all queues.
queues = self.queues
queues_to_process = self.queues
else:
# Limit processing to post-imperative queues
for queue in self.queues:
if queue.imperative:
# Here the `queues` list will consists of the imperative queue along with
# any subsequent queues, this means elements will be carried only from the
# imerative queue onwards, and only post-imperative jobs will be processed.
queues = self.queues[self.queues.index(queue) :]
# Here the `queues_to_process` list will consists of all the queues after
# the imperative queue. This means only post-imperative jobs will be processed.
queues_to_process = self.queues[self.queues.index(queue) + 1 :]
break
else:
# No imperative queue was marked, stop queueing any jobs
queues = []
queues_to_process = []

while process_queues:

# Pull elements forward through queues
# Pull elements forward through all queues, regardless of whether we're processing those
# queues. The main reason to do this is to ensure we propagate finished jobs from the
# imperative queue.
elements = []
for queue in queues:
for queue in self.queues:
Copy link
Contributor

Choose a reason for hiding this comment

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

This line looks very dubious.

The first part of the function's job is to select the queues on which to process in the latter half of the function, that seems to make sense. And except for this line you've changed, the rest of the function does only operate on the selected queues.

I wonder if this change is really needed, or if only the previous change is needed.

Given this is such a sensitive code block, I think this change needs further clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first part of the function's job is to select the queues on which to process in the latter half of the function, that seems to make sense. And except for this line you've changed, the rest of the function does only operate on the selected queues.

Correct, and that's the root cause of the issue I'm trying to fix.

There are two operations in this function ("pull elements forward through queues", and "kickoff whatever processes can be processed at this time") and they shouldn't operate on the same list of queues.

Let's consider a simple example where we have three queues: fetch, build and push. Once we receive the quit signal, we only want to:

  • pull elements forward from build to push
  • kickoff new jobs in push

What the current code does is:

  • pull elements forward from build to push
  • kickoff new jobs in build (<- not desired, and is the issue I'm trying to fix)
  • kickoff new jobs in push

What the new code does is:

  • pull elements forward from fetch to build (<- undesired but harmless, this is the issue you're pointing)
  • pull elements through from build to push
  • kickoff new jobs in push

I hope this explains it. So while this line is indeed "dubious", it needs to be different from the other one. And since it's harmless to pull jobs into next queues even when asked to quit, I think it's fine to do that unconditionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok... I've done some mental gymnastics and I agree with your statement :)

What I would like to see here is:

  • Rename the local queues variable to something more explicit, perhaps queues_to_process
  • At least slightly elaborate on the comment "Pull elements forward through queues"
    • Perhaps something like "Pull elements forward through all queues, regardless of whether we are processing those queues"
    • Maybe even a little explanation, like "We need to propagate elements through all queues even if we are not processing post-imperative queues, so that we do end up processing the jobs we need to"

queue.enqueue(elements)
elements = list(queue.dequeue())

Expand All @@ -386,13 +387,13 @@ def _sched_queue_jobs(self):
# to fetch tasks for elements which failed to pull, and
# thus need all the pulls to complete before ever starting
# a build
ready.extend(chain.from_iterable(q.harvest_jobs() for q in reversed(queues)))
ready.extend(chain.from_iterable(q.harvest_jobs() for q in reversed(queues_to_process)))

# harvest_jobs() may have decided to skip some jobs, making
# them eligible for promotion to the next queue as a side effect.
#
# If that happens, do another round.
process_queues = any(q.dequeue_ready() for q in queues)
process_queues = any(q.dequeue_ready() for q in queues_to_process)

# Start the jobs
#
Expand Down
9 changes: 8 additions & 1 deletion src/buildstream/_testing/runcli.py
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,14 @@ def remove_artifact_from_cache(self, cache_dir, element_name):

normal_name = element_name.replace(os.sep, "-")
cache_dir = os.path.splitext(os.path.join(cache_dir, "test", normal_name))[0]
shutil.rmtree(cache_dir)

# Don't cause a failure here if the artifact is not there, we use this in integration
# tests where the cache is shared and we just want to clean up some artifacts
# regardless of whether they have already been built or not.
try:
shutil.rmtree(cache_dir)
except FileNotFoundError:
pass

# is_cached():
#
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/cached-fail/elements/base-also-fail.bst
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
kind: script

build-depends:
- base.bst

config:
commands:
- touch %{install-root}/foo
- false
9 changes: 9 additions & 0 deletions tests/integration/cached-fail/elements/base-fail.bst
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
kind: script

build-depends:
- base.bst

config:
commands:
- touch %{install-root}/foo
- false
8 changes: 8 additions & 0 deletions tests/integration/cached-fail/elements/base-success.bst
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
kind: script

build-depends:
- base.bst

config:
commands:
- touch %{install-root}/foo
17 changes: 17 additions & 0 deletions tests/integration/cached-fail/elements/base.bst
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
kind: import

description: |
Alpine Linux base for tests

Generated using the `tests/integration-tests/base/generate-base.sh` script.

sources:
- kind: tar
base-dir: ''
(?):
- arch == "x86-64":
ref: 3eb559250ba82b64a68d86d0636a6b127aa5f6d25d3601a79f79214dc9703639
url: "alpine:integration-tests-base.v1.x86_64.tar.xz"
- arch == "aarch64":
ref: 431fb5362032ede6f172e70a3258354a8fd71fcbdeb1edebc0e20968c792329a
url: "alpine:integration-tests-base.v1.aarch64.tar.xz"
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
kind: script

build-depends:
- base.bst
- base-fail.bst

config:
commands:
- test -e /foo
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
kind: script

build-depends:
- base-fail.bst
- base-also-fail.bst

config:
commands:
- test -e /foo
37 changes: 37 additions & 0 deletions tests/integration/cached-fail/project.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Project config for frontend build test
name: test
min-version: 2.0
element-path: elements

aliases:
alpine: https://bst-integration-test-images.ams3.cdn.digitaloceanspaces.com/
project_dir: file://{project_dir}

plugins:
- origin: pip
package-name: sample-plugins
elements:
- autotools

options:
linux:
type: bool
description: Whether to expect a linux platform
default: True
arch:
type: arch
description: Current architecture
variable: build_arch
values:
- x86-64
- aarch64

environment:
TEST_VAR: pony

split-rules:
test:
- |
/tests
- |
/tests/*
Loading
Loading