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

Jump to bottom

Fixed cockpit process starting #20077

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Fixed cockpit process starting #20077

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bb0735
changes:
yrudman Apr 16, 2020
6e67f1f
fixed path to cockpit.conf
yrudman Apr 17, 2020
ddec1e3
close streams before killing process
yrudman Apr 20, 2020
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: 14 additions & 7 deletions app/models/miq_cockpit_ws_worker/runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,15 @@ def stop_cockpit_ws
def stop_cockpit_ws_process
return unless @pid
Process.kill("TERM", @pid)
@stdout&.close
@stderr&.close
yrudman marked this conversation as resolved.
Show resolved Hide resolved
wait_on_cockpit_ws
end

# Waits for a cockpit-ws process to stop. The process is expected to be
# in the act of shutting down, and thus it will wait 5 minutes
# before issuing a kill.
def wait_on_cockpit_ws(pid)
def wait_on_cockpit_ws(pid = nil)
pid ||= @pid
# TODO: Use Process.waitpid or one of its async variants
begin
Expand Down Expand Up @@ -148,13 +150,18 @@ def cockpit_ws_run
"XDG_CONFIG_DIRS" => cockpit_ws.config_dir,
"DRB_URI" => @drb_uri
}
Bundler.with_clean_env do
stdin, stdout, stderr, wait_thr = Open3.popen3(env, *cockpit_ws.command(BINDING_ADDRESS), :unsetenv_others => true)
_log.info("Starting cockpit-ws process with command: #{cockpit_ws.command(BINDING_ADDRESS)} ")
_log.info("Cockpit environment #{env} ")
stdin, stdout, stderr, wait_thr = Bundler.with_clean_env do
Open3.popen3(env, cockpit_ws.command(BINDING_ADDRESS), :unsetenv_others => true)
end
stdin&.close
Copy link
Member

Choose a reason for hiding this comment

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

I believe the intention was to have stdin.close in the scope of the Open3 command and was accidentally moved to a different scope, outside the Bundler.with_clean_env scope, in commit: 63b44ed

I believe checking for stdin here is not right. We should either not close stdin at all or move the stdin.close in the with_clean_env block.

cc @carbonin

Copy link
Member

Choose a reason for hiding this comment

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

move the stdin.close in the with_clean_env block.

☝️ This I think.

if wait_thr
_log.info("#{log_prefix} cockpit-ws process started - pid=#{wait_thr.pid}")
return wait_thr.pid, stdout, stderr
else
raise "Cockpit-ws process failed to start"
end
stdin.close

_log.info("#{log_prefix} cockpit-ws process started - pid=#{@pid}")
return wait_thr.pid, stdout, stderr
end

def check_drb_service
Expand Down
2 changes: 1 addition & 1 deletion lib/miq_cockpit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def self.url(miq_server, opts, address)

def initialize(opts = {})
@opts = opts || {}
@config_dir = File.join(__dir__, "..", "config")
@config_dir = Rails.root.join("config").to_s
@cockpit_conf_dir = File.join(@config_dir, "cockpit")
FileUtils.mkdir_p(@cockpit_conf_dir)
end
Expand Down