PR#3007: better error messages for Task.lock() - koji

koji

#3007 better error messages for Task.lock()

Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.

tkopecek/koji issue2834 into master

better error messages for Task.lock()

Tomas Kopecek • 2 years ago

2f336d0

hub/kojihub.py

file modified

+22 -9

		`@@ -194,22 +194,35 @@`
		`state, otherhost = r`
		`if state == koji.TASK_STATES['FREE']:`
		`if otherhost is not None:`
		`- log_error("Error: task %i is both free and locked (host %i)"`
		`- % (task_id, otherhost))`
		`+ log_error(f"Error: task {task_id} is both free "`
		`+ f"and handled by host {otherhost}")`
		`return False`
		`elif state == koji.TASK_STATES['ASSIGNED']:`
		`if otherhost is None:`
		`- log_error("Error: task %i is assigned, but has no assignee"`
		`- % (task_id))`
		`+ log_error(f"Error: task {task_id} is assigned, but no host is really assigned")`
		`return False`
		`elif otherhost != host_id:`
		`- # task is assigned to someone else`
		`+ # task is assigned to someone else, no error just return`
		`return False`
		`+ elif newstate == 'ASSIGNED':`
		`+ # double assign is a weird situation but we can return True as state doesn't`
		`+ # really change`
		`+ log_error(f"Error: double assign of task {task_id} and host {host_id}")`
		`+ return True`
		`# otherwise the task is assigned to host_id, so keep going`
		`- else:`
		`+ elif state == koji.TASK_STATES['CANCELED']:`
		`+ # it is ok that task was canceled meanwhile`
		`+ return False`
		`+ elif state == koji.TASK_STATES['OPEN']:`
		`if otherhost is None:`
		`- log_error("Error: task %i is non-free but unlocked (state %i)"`
		`- % (task_id, state))`
		`+ log_error(f"Error: task {task_id} is opened but not handled by any host")`
		`+ elif otherhost == host_id:`
		`+ log_error(f"Error: task {task_id} is already open and handled by "`
		`+ f"{host_id} (double open/assign)")`
		`+ return False`
		`+ elif otherhost is None: # CLOSED, FAILED`
		`+ log_error(f"Error: task {task_id} is non-free but not handled by any host "`
		`+ f"(state {koji.TASK_STATES[state]})")`
		`return False`
		`# if we reach here, task is either`
		`# - free and unlocked`
		`@@ -6321,7 +6334,7 @@`
		`"""`

		`if state not in (koji.BUILD_STATES['FAILED'], koji.BUILD_STATES['CANCELED']):`
		`- raise koji.GenericError("Only FAILED/CANCELLED build states are allowed")`
		`+ raise koji.GenericError("Only FAILED/CANCELED build states are allowed")`

		`assert_cg(cg)`
		`binfo = get_build(build_id, strict=True)`

tkopecek commented 2 years ago

@mikem I'm not sure about ASSIGNED case - it seems to me that it also shouldn't log an error?

mikem commented 2 years ago

elif state == koji.TASK_STATES['OPEN'] and otherhost is None:
    log_error(f"Error: task {task_id} is opened but not locked by any host")
    return False

This probably needs to open with simply elif state == koji.TASK_STATES['OPEN']: and cover some different sanity rules for open tasks.

if the open task has no host, then that is an error that needs logging
if the open task has a different host, then that is a normal, non-error case (another host won the race) and we can simply return False.
if the open task has the same host, then that is probably worth logging. It either means there was a double open, or an admin trying to assign a task to a host that already has it.

mikem commented 2 years ago

I'm not sure about ASSIGNED case

It depends on the particulars. I think the case where the state is assigned but there is no host_id is unexpected and worth an error message.

For assigned in particular, the sanity checks might need to depend on newstate as well. I.e. it might be worth noting a double assign (same host id, state=newstate=assigned), but perfectly normal and expected for that host to open the assigned task.

tkopecek commented 2 years ago

makes sense, updated

rebased onto 8dc2b5d2290f98def3b7afac717b5e6801741dc9

2 years ago

mikem commented 2 years ago

:thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

rebased onto 2f336d0

2 years ago