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.

Sign up for GitHub

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

Scanning::Job always cleanup all signals in cleanup #99

Merged
merged 1 commit into from
Sep 27, 2017
+12 −2
Merged

Scanning::Job always cleanup all signals in cleanup #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions app/models/manageiq/providers/kubernetes/container_manager/scanning/job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ def delete_pod
end

def cleanup(*args)
unqueue_all_signals
Copy link

Choose a reason for hiding this comment

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

cancel now calls this twice?

Copy link
Author

Choose a reason for hiding this comment

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

@moolitayer Yes, but if we remove this from cancel there will be a race between the next pod_wait and the cleanup signals.

Copy link

Choose a reason for hiding this comment

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

interesting, makes sense.
There should not be a race since in cancel you force the state to be cancel and a state change from cancel to pod_wait is not allowed - but now I see the bug you are fixing is the logging of an invalid state change so 👍

Copy link
Author

Choose a reason for hiding this comment

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

exactly, In this fix we are trying to avoid this mechanism that enforces the state machine.

image = target_entity
if image
# TODO: check job success / failure
Expand Down Expand Up @@ -251,8 +252,7 @@ def cancel(*_args)
if self.state != "canceling" # ensure change of states
signal :cancel
else
unqueue_all_signals
queue_signal(:cancel_job)
signal :cancel_job
end
end
alias_method :cancel_job, :cleanup
Expand Down
10 changes: 10 additions & 0 deletions spec/models/manageiq/providers/kubernetes/container_manager/scanning/job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,16 @@ def fetch_oscap_arf
end
end

context "when timeout occures during pod_wait state" do
it "should clean all signals and not fail in the state machine" do
initial_q_size = MiqQueue.all.count
MiqQueue.put(**@job.send(:queue_options), :args => [:pod_wait,])
expect(MiqQueue.all.count).to eq(initial_q_size + 1)
@job.cancel
expect(MiqQueue.all.count).to eq(initial_q_size)
end
end

context "#current_job_timeout" do
it "checks for timeout in Settings" do
stub_settings_merge(:container_scanning => {:scanning_job_timeout => '15.minutes'})
Expand Down