Fix virt-v2v kill #433
Fix virt-v2v kill #433
Conversation
|
@miq-bot add-label transformation, bug, technical debt, hammer/yes |
ghost
mentioned this pull request
ghost
force-pushed
the
v2v_fix_virtv2v_kill
branch
from
17d18c3
to
48a4b49
Compare
ghost
force-pushed
the
v2v_fix_virtv2v_kill
branch
from
48a4b49
to
137858c
Compare
| end | ||
|
|
||
| it "when virtv2v is finished" do | ||
| allow(svc_model_task).to receive(:get_option).with(:virtv2v_started_on).and_return(Time.now.utc - 1) |
There was a problem hiding this comment.
@fdupont-redhat There is a lot of shared setup between these tests. I recommend creating context so you can share the setup between tests.
For example,
The allow(svc_model_task).to receive(:get_option).with(:virtv2v_started_on).and_return(Time.now.utc - 1) could define one context. with several tests.
Inside of that context would be another that does a before block for allow(svc_model_task).to receive(:get_option).with(:virtv2v_finished_on).and_return(nil) and list the tests under it.
Can you take a shot at reorganizing these and let me know if you need help.
| end | ||
|
|
||
| before(:each) do | ||
| ManageIQ::Automate::Transformation::TransformationHosts::Common::KillVirtV2V.instance_variable_set(:@task, nil) |
There was a problem hiding this comment.
before(:each) is the default action so it is the same as the before block above. This line can just move up in to the previous block.
| before { allow(svc_model_task).to receive(:get_option).with(:virtv2v_started_on).and_return(Time.now.utc - 1) } | ||
|
|
||
| context "when virtv2v is finished" do | ||
| before { allow(svc_model_task).to receive(:get_option).with(:virtv2v_finished_on).and_return(Time.now.utc) } |
There was a problem hiding this comment.
Same, move the allow to the test block.
|
|
||
| context "#task_virtv2v_state" do | ||
| context "when virtv2v is not started" do | ||
| before { allow(svc_model_task).to receive(:get_option).with(:virtv2v_started_on).and_return(nil) } |
There was a problem hiding this comment.
You don't need the before block if there is only one test. Just include the allow line in the test block.
| @handle.root['ae_result'] = 'retry' | ||
| @handle.root['ae_retry_interval'] = '30.seconds' | ||
| end | ||
| ManageIQ::Automate::Transformation::TransformationHosts::Common::Utils.remote_command(@task, transformation_host, "kill -s #{signal} #{pid}") |
There was a problem hiding this comment.
I was thinking about how to write this without using unless since we normally only use that for simple one-line conditions.
My suggestion would be to move the kill_signal logic into a separate method.
def kill_virtv2v(transformation_host, pid)
ManageIQ::Automate::Transformation::TransformationHosts::Common::Utils.remote_command(@task, transformation_host, "kill -s #{kill_signal} #{pid}")
end
def kill_signal
if @handle.get_state_var('virtv2v_graceful_kill')
'KILL'
else
@handle.set_state_var('virtv2v_graceful_kill', true)
@handle.root['ae_result'] = 'retry'
@handle.root['ae_retry_interval'] = '30.seconds'
'TERM'
end
end
|
Checked commits fabiendupont/manageiq-content@137858c~...03be4ae with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@gmcculloug Do you want more changes ? |
|
This pull request is not mergeable. Please rebase and repush. |
|
This change has been implemented in #441. Closing this PR. |
ghost
closed this
In the course of a discussion with Richard W.M. Jones, he said that:
This PR aims at fixing this by first using SIGTERM to kill virt-v2v with a grace period of 30 seconds, using retry. After that grace period, virt-v2v is sent SIGKILL, as it was the case before.
While I was at it, I added the specs for this method.
Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1633526