fix(stop): store engine logs of stopped workflow (#563)

[mdonadoni]

Closes #560

Depends on #559

[mdonadoni]

Note that the method _update_workflow_status in consumer.py is now being called in rest/utils.py. I think a refactor is needed, in particular:

1. The logic on how to update the status/logs/job progress/job cache of workflows should not be in consumer.py, but in a dedicated module/package
2. There should only be one way (thus, one function) to stop a workflow (and to start/delete/... it), which both updates the database and interacts with k8s as needed. These functions would/should use the ones from (1), and should not be part of rest.

Proper design/planning is needed, though. Should we do it later or as part of this PR?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (817b019) 72.83% compared to head (199c163) 73.23%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   72.83%   73.23%   +0.40%     
==========================================
  Files          15       15              
  Lines        1362     1360       -2     
==========================================
+ Hits          992      996       +4     
+ Misses        370      364       -6     

Files	Coverage Δ
reana_workflow_controller/rest/utils.py	81.79% <100.00%> (-0.11%)	⬇️

... and 1 file with indirect coverage changes

[giuseppe-steduto]

I fully support the idea of a refactor, especially to create a single point from which to update the status of a workflow. However, maybe it's better if we create a ticket for it and work on it later, as part of the next minor release, so we can focus on finishing what's left for the patch release and fixing this bug without the risk of changing the code in a lot of places. WDYT?

[giuseppe-steduto]

I guess calling Session.commit() here is unnecessary, since it is already being called by _update_workflow_status (here the reana_db code), right?

[mdonadoni]

It is still needed because even though the status is commited by Workflow.update_workflow_status, the logs are fetched and saved to the database after that:

reana-workflow-controller/reana_workflow_controller/consumer.py

Lines 143 to 156 in 817b019

	def _update_workflow_status(workflow, status, logs):
	"""Update workflow status in DB."""
	if workflow.status != status:
	Workflow.update_workflow_status(Session, workflow.id_, status, logs, None)
	if workflow.git_ref:
	_update_commit_status(workflow, status)
	if status not in ALIVE_STATUSES:
	workflow.run_finished_at = datetime.now()
	workflow.logs = workflow.logs or ""
	try:
	workflow_engine_logs = _get_workflow_engine_pod_logs(workflow)
	workflow.logs += workflow_engine_logs + "\n"

[mdonadoni]

I fully support the idea of a refactor, especially to create a single point from which to update the status of a workflow. However, maybe it's better if we create a ticket for it and work on it later, as part of the next minor release, so we can focus on finishing what's left for the patch release and fixing this bug without the risk of changing the code in a lot of places. WDYT?

I agree! I have opened a new issue: #565