From b03fd08c48c1f848f8a1f0777c1f3a90288a7fed Mon Sep 17 00:00:00 2001 From: Jhonas Date: Fri, 6 Dec 2024 10:27:30 -0300 Subject: [PATCH] fix: capture errors that shouldnt fail the transaction We introduced a ResolutionError to mark errors during the new plan network resolution shouldnt be raised, which causes the whole transation to fail. Instead, we capture them and fail the deployment of the specific tasks that caused them. --- lib/syskit/cli/doc/gen.rb | 4 +- lib/syskit/gui/component_network_view.rb | 2 +- lib/syskit/network_generation/async.rb | 2 +- lib/syskit/network_generation/engine.rb | 28 ++++++-- lib/syskit/network_generation/exceptions.rb | 23 +++++++ .../system_network_generator.rb | 59 ++++++++++++---- lib/syskit/test/network_manipulation.rb | 2 +- test/network_generation/test_async.rb | 21 ++++++ test/network_generation/test_engine.rb | 2 +- .../test_system_network_generator.rb | 67 ++++++++++++++----- 10 files changed, 168 insertions(+), 42 deletions(-) create mode 100644 lib/syskit/network_generation/exceptions.rb diff --git a/lib/syskit/cli/doc/gen.rb b/lib/syskit/cli/doc/gen.rb index 71c8914d5..235a49548 100644 --- a/lib/syskit/cli/doc/gen.rb +++ b/lib/syskit/cli/doc/gen.rb @@ -72,7 +72,7 @@ def self.save_profile_model(target_path, profile_m) # @param [Actions::Profile::Definition] profile_def def self.save_profile_definition(target_path, profile_def) begin - task = compute_system_network( + task, = compute_system_network( profile_def, validate_abstract_network: false, validate_generated_network: false @@ -266,7 +266,7 @@ def self.compute_system_network(model, main_plan = Roby::Plan.new, **options) main_plan.add(original_task = model.as_plan) engine = Syskit::NetworkGeneration::Engine.new(main_plan) planning_task = original_task.planning_task - mapping = engine.compute_system_network([planning_task], **options) + mapping, = engine.compute_system_network([planning_task], **options) if engine.work_plan.respond_to?(:commit_transaction) engine.work_plan.commit_transaction diff --git a/lib/syskit/gui/component_network_view.rb b/lib/syskit/gui/component_network_view.rb index fdb0ff68b..02a7567d1 100644 --- a/lib/syskit/gui/component_network_view.rb +++ b/lib/syskit/gui/component_network_view.rb @@ -150,7 +150,7 @@ def render(model, begin if method == :compute_system_network tic = Time.now - @task = compute_system_network(model, plan) + @task, = compute_system_network(model, plan) timing = Time.now - tic elsif method == :compute_deployed_network tic = Time.now diff --git a/lib/syskit/network_generation/async.rb b/lib/syskit/network_generation/async.rb index 106f5fbf1..9325b7c3f 100644 --- a/lib/syskit/network_generation/async.rb +++ b/lib/syskit/network_generation/async.rb @@ -135,7 +135,7 @@ def apply engine.discard_work_plan false elsif future.fulfilled? - required_instances = future.value + required_instances, resolution_errors = future.value begin engine.apply_system_network_to_plan( required_instances, **@apply_system_network_options diff --git a/lib/syskit/network_generation/engine.rb b/lib/syskit/network_generation/engine.rb index 4d294e535..0e0febabd 100644 --- a/lib/syskit/network_generation/engine.rb +++ b/lib/syskit/network_generation/engine.rb @@ -710,14 +710,14 @@ def compute_system_network( system_network_generator = SystemNetworkGenerator.new( work_plan, event_logger: event_logger, merge_solver: merge_solver ) - toplevel_tasks = system_network_generator.generate( + toplevel_tasks, resolution_errors = system_network_generator.generate( instance_requirements, garbage_collect: garbage_collect, validate_abstract_network: validate_abstract_network, validate_generated_network: validate_generated_network ) - Hash[requirement_tasks.zip(toplevel_tasks)] + [Hash[requirement_tasks.zip(toplevel_tasks)], resolution_errors] end # Computes the system network, that is the network that fullfills @@ -752,7 +752,7 @@ def resolve_system_network( compute_policies: true ) - required_instances = compute_system_network( + required_instances, resolution_errors = compute_system_network( requirement_tasks, garbage_collect: garbage_collect, validate_abstract_network: validate_abstract_network, @@ -768,7 +768,7 @@ def resolve_system_network( ) end end - required_instances + [required_instances, resolution_errors] end # Generate the deployment according to the current requirements, and @@ -806,7 +806,7 @@ def resolve( validate_deployed_network: true, validate_final_network: true ) - required_instances = resolve_system_network( + required_instances, resolution_errors = resolve_system_network( requirement_tasks, garbage_collect: garbage_collect, validate_abstract_network: validate_abstract_network, @@ -823,11 +823,29 @@ def resolve( garbage_collect: garbage_collect, validate_final_network: validate_final_network ) + + resolution_errors = replace_resolution_errors_tasks(resolution_errors) + resolution_errors.each do |error| + plan.execution_engine.add_error(error) + end rescue Exception => e # rubocop:disable Lint/RescueException handle_resolution_exception(e, on_error: on_error) raise end + # Replace the failed tasks in resolution errors for the tasks found in the + # real plan + # + # @param [Array] resolution_errors all the resolution errors that should have + # their failed tasks replaced + # @return [Array] the newly generated resolution errors + def replace_resolution_errors_tasks(resolution_errors) + (resolution_errors || []).map do |original| + replacement = merge_solver.replacement_for(original.failed_task) + original.replace_failed_task(replacement) + end + end + def apply_system_network_to_plan( required_instances, compute_deployments: true, garbage_collect: true, validate_final_network: true diff --git a/lib/syskit/network_generation/exceptions.rb b/lib/syskit/network_generation/exceptions.rb new file mode 100644 index 000000000..b4aa1d501 --- /dev/null +++ b/lib/syskit/network_generation/exceptions.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require "roby/standard_errors" + +module Syskit + module NetworkGeneration + class ResolutionError < Roby::LocalizedError + def replace_failed_task(replacement) + ResolutionError.new(replacement, original_exceptions, message) + end + + def initialize(failed_task, original_exceptions = [], message = "") + super(failed_task) + @message = message + report_exceptions_from(original_exceptions) + end + + def pretty_print(pp) + each_original_exception { |exception| exception.pretty_print(pp) } + end + end + end +end diff --git a/lib/syskit/network_generation/system_network_generator.rb b/lib/syskit/network_generation/system_network_generator.rb index 77b15e946..04258957e 100644 --- a/lib/syskit/network_generation/system_network_generator.rb +++ b/lib/syskit/network_generation/system_network_generator.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "syskit/network_generation/exceptions" + module Syskit module NetworkGeneration # Generate a plan from a set of {InstanceRequirement} objects @@ -241,17 +243,26 @@ def compute_system_network(instance_requirements, garbage_collect: true, end log_timepoint "postprocessing" + resolution_errors = validate_network( + validate_abstract_network: validate_abstract_network, + validate_generated_network: validate_generated_network + ) + remove_tasks_from_plan_due_to_resolution_errors(resolution_errors) + [@toplevel_tasks, resolution_errors] + end + + def validate_network(validate_abstract_network: true, + validate_generated_network: true) if validate_abstract_network self.validate_abstract_network log_timepoint "validate_abstract_network" end if validate_generated_network - self.validate_generated_network + resolution_errors = self.validate_generated_network log_timepoint "validate_generated_network" end - - @toplevel_tasks + resolution_errors || [] end def toplevel_tasks_to_requirements @@ -261,6 +272,14 @@ def toplevel_tasks_to_requirements .each_with_object({}) { |(t, ir), h| (h[t] ||= []) << ir } end + def remove_tasks_from_plan_due_to_resolution_errors(resolution_errors) + resolution_errors.each do |resolution_error| + failed_task = resolution_error.failed_task + @toplevel_tasks.delete(failed_task) + plan.remove_task(failed_task) + end + end + # Verifies that the task allocation is complete # # @param [Roby::Plan] plan the plan on which we are working @@ -270,11 +289,12 @@ def self.verify_task_allocation( plan, components: plan.find_local_tasks(AbstractComponent) ) still_abstract = components.find_all(&:abstract?) - return if still_abstract.empty? - - raise TaskAllocationFailed.new(self, still_abstract), + still_abstract.map do |task| + exception = TaskAllocationFailed.new(self, [task]) + ResolutionError.new(task, exception, "could not find implementation for the following abstract " \ - "tasks: #{still_abstract}" + "tasks: #{still_abstract}") + end end # Verifies that there are no multiple output - single input @@ -322,10 +342,11 @@ def self.verify_device_allocation(plan, toplevel_tasks_to_requirements = {}) t.model.each_master_driver_service .any? { |srv| !t.find_device_attached_to(srv) } end - unless missing_devices.empty? - raise DeviceAllocationFailed.new(plan, missing_devices), - "could not allocate devices for the following tasks: " \ - "#{missing_devices}" + resolution_errors = missing_devices.map do |dev| + allocation_error = DeviceAllocationFailed.new(plan, dev) + resolution_errors << + ResolutionError.new(dev, allocation_error, + "could not allocate a device for the task: #{dev}") end devices = {} @@ -333,14 +354,19 @@ def self.verify_device_allocation(plan, toplevel_tasks_to_requirements = {}) task.each_master_device do |dev| device_name = dev.full_name if (old_task = devices[device_name]) - raise ConflictingDeviceAllocation.new( + allocation_error = ConflictingDeviceAllocation.new( dev, task, old_task, toplevel_tasks_to_requirements ) + resolution_errors << + ResolutionError.new(old_task, allocation_error) + resolution_errors << + ResolutionError.new(task, allocation_error) else devices[device_name] = task end end end + resolution_errors end # Validates the network generated by {#compute_system_network} @@ -354,9 +380,14 @@ def validate_abstract_network # Validates the network generated by {#compute_system_network} def validate_generated_network - self.class.verify_task_allocation(plan) - self.class.verify_device_allocation(plan, toplevel_tasks_to_requirements) + resolution_errors = self.class.verify_task_allocation(plan) + device_allocation_errors = self.class.verify_device_allocation( + plan, toplevel_tasks_to_requirements + ) + resolution_errors.concat(device_allocation_errors) super if defined? super + + resolution_errors.flatten end end end diff --git a/lib/syskit/test/network_manipulation.rb b/lib/syskit/test/network_manipulation.rb index caf9814c1..f070b04cf 100644 --- a/lib/syskit/test/network_manipulation.rb +++ b/lib/syskit/test/network_manipulation.rb @@ -160,7 +160,7 @@ def syskit_generate_network(*to_instanciate, add_missions: true) end task_mapping = plan.in_transaction do |trsc| engine = NetworkGeneration::Engine.new(plan, work_plan: trsc) - mapping = engine.compute_system_network( + mapping, = engine.compute_system_network( tasks_to_instanciate.map(&:planning_task), validate_generated_network: false ) diff --git a/test/network_generation/test_async.rb b/test/network_generation/test_async.rb index 833217fbd..da2dd844a 100644 --- a/test/network_generation/test_async.rb +++ b/test/network_generation/test_async.rb @@ -124,6 +124,27 @@ def assert_future_fulfilled(future) assert subject.finished? assert_raises(error_t) { subject.apply } end + + it "goes through with the plan even if some tasks could not be resolved" do + srv_m = Syskit::DataService.new_submodel + plan.add(task = Models::Placeholder.create_for([srv_m]).new) + + resolution_errors = [ResolutionError.new(task, RuntimeError)] + requirements = Set[flexmock] + resolution = subject.prepare(requirements) + engine = flexmock(resolution.engine, :strict) + engine.should_receive(:resolve_system_network) + .and_return([[], resolution_errors]) + engine.should_receive(:apply_system_network_to_plan).once + flexmock(engine).should_receive(:replace_resolution_errors_tasks) + .once.pass_thru + flexmock(subject.plan.execution_engine).should_receive(:add_error) + .with(*resolution_errors) + resolution.execute + subject.join + assert subject.finished? + subject.apply + end end end end diff --git a/test/network_generation/test_engine.rb b/test/network_generation/test_engine.rb index ef9ff6ada..440b6f12d 100644 --- a/test/network_generation/test_engine.rb +++ b/test/network_generation/test_engine.rb @@ -104,7 +104,7 @@ def work_plan it "saves the mapping from requirement task in real_plan to instanciated task in work_plan" do flexmock(requirements).should_receive(:instanciate) .and_return(instanciated_task = simple_component_model.new) - mapping = syskit_engine.compute_system_network([planning_task]) + mapping, = syskit_engine.compute_system_network([planning_task]) assert_equal instanciated_task, mapping[planning_task] end end diff --git a/test/network_generation/test_system_network_generator.rb b/test/network_generation/test_system_network_generator.rb index d0f672254..1d3b36caf 100644 --- a/test/network_generation/test_system_network_generator.rb +++ b/test/network_generation/test_system_network_generator.rb @@ -103,14 +103,40 @@ def arg=(value) describe "#compute_system_network" do it "runs the validate_abstract_network handler if asked to" do generator = SystemNetworkGenerator.new(plan) - flexmock(generator).should_receive(:validate_abstract_network).once + flexmock(generator).should_receive(:validate_abstract_network).and_return([]).once generator.compute_system_network([], validate_abstract_network: true) end it "runs the validate_generated_network handler if asked to" do generator = SystemNetworkGenerator.new(plan) - flexmock(generator).should_receive(:validate_generated_network).once + flexmock(generator).should_receive(:validate_generated_network).and_return([]).once generator.compute_system_network([], validate_generated_network: true) end + + it "removes tasks with errors and their dependencies on the generated network" do + srv_m = Syskit::DataService.new_submodel + cmp_m = Syskit::Composition.new_submodel do + add srv_m, as: "test" + end + task = cmp_m.instanciate(plan).to_task + requirements = cmp_m.to_instance_requirements + plan.add(task) + + generator = SystemNetworkGenerator.new(plan) + error = ResolutionError.new(task) + flexmock(generator).should_receive(:validate_network) + .and_return([error]) + flexmock(generator).should_receive(:instanciate) + .pass_thru { [task] } + # Turn off garbage collect because the mocked resolution error task + # would be garbage collected. + toplevel_tasks, = + generator.compute_system_network([requirements], + garbage_collect: false, + validate_generated_network: true, + validate_abstract_network: false) + refute plan.has_task? task + assert toplevel_tasks.empty? + end end describe "#generate" do @@ -136,23 +162,23 @@ def compute_system_network(*requirements) end it "keeps the compositions' optional dependencies that are not abstract" do - cmp = compute_system_network(cmp_m.use("test" => task_m)) + cmp, = compute_system_network(cmp_m.use("test" => task_m)) assert cmp.has_role?("test") end it "keeps the compositions' non-optional dependencies that are abstract" do cmp_m.add srv_m, as: "non_optional" - cmp = compute_system_network(cmp_m) + cmp, = compute_system_network(cmp_m) assert cmp.has_role?("non_optional") end it "removes the compositions' optional dependencies that are still abstract" do - cmp = compute_system_network(cmp_m) + cmp, = compute_system_network(cmp_m) assert !cmp.has_role?("test") end it "enables the use of the abstract flag in InstanceRequirements to use an optional dep only if it is instanciated by other means" do - cmp = compute_system_network(cmp_m.use("test" => task_m.to_instance_requirements.abstract)) + cmp, = compute_system_network(cmp_m.use("test" => task_m.to_instance_requirements.abstract)) refute cmp.has_role?("test") execute { plan.remove_task(cmp) } - cmp = compute_system_network(cmp_m.use("test" => task_m.to_instance_requirements.abstract), task_m) + cmp, = compute_system_network(cmp_m.use("test" => task_m.to_instance_requirements.abstract), task_m) assert cmp.has_role?("test") end end @@ -162,9 +188,13 @@ def compute_system_network(*requirements) it "validates that there are no placeholder tasks left in the plan" do srv_m = Syskit::DataService.new_submodel plan.add(task = Models::Placeholder.create_for([srv_m]).new) - assert_raises(Syskit::TaskAllocationFailed) do - SystemNetworkGenerator.new(plan).validate_generated_network - end + generator = SystemNetworkGenerator.new(plan) + actual = generator.validate_generated_network + expected = [ + ResolutionError.new(task, + TaskAllocationFailed.new(generator, [task])) + ] + assert_equal expected, actual end it "validates that devices are allocated at most once" do @@ -177,13 +207,11 @@ def compute_system_network(*requirements) plan.add(task1 = task_m.new(arg: 1, test_dev: robot.test_dev)) plan.add(task2 = task_m.new(arg: 2, test_dev: robot.test_dev)) - e = assert_raises(ConflictingDeviceAllocation) do - SystemNetworkGenerator.new(plan).validate_generated_network - end + generator = SystemNetworkGenerator.new(plan) + actual = generator.validate_generated_network - assert_equal Set[task1, task2], e.tasks.to_set - formatted = PP.pp(e, +"") - expected = <<~MSG + expected_failed_tasks = [task1, task2] + expected_message = <<~MSG device 'test' of type D is assigned to two tasks that cannot be merged Chain 1 cannot be merged in chain 2: Chain 1: @@ -203,7 +231,12 @@ def compute_system_network(*requirements) conf: default(["default"]), read_only: default(false) MSG - assert_equal expected, formatted.gsub(//, "") + actual.each_with_index do |e, i| + formatted = PP.pp(e, +"") + assert_equal expected_failed_tasks[i], e.failed_task + assert_equal expected_message, + formatted.gsub(//, "") + end end end