From f500cb3b7423c495d4cee8898ed965c7028bfa77 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 15:33:01 +0000 Subject: [PATCH 1/51] update getNextTask to use host.getTasks --- diff --git a/koji/daemon.py b/koji/daemon.py index fdd4be2..e343351 100644 --- a/koji/daemon.py +++ b/koji/daemon.py @@ -1030,130 +1030,42 @@ class TaskManager(object): self.logger.info("Lingering task %r (pid %r)" % (id, pid)) def getNextTask(self): + """Task the next task + + :returns: True if a task was taken, False otherwise + """ self.ready = self.readyForTask() self.session.host.updateHost(self.task_load, self.ready) if not self.ready: self.logger.info("Not ready for task") return False - hosts, tasks = self.session.host.getLoadData() - self.logger.debug("Load Data:") - self.logger.debug(" hosts: %r" % hosts) - self.logger.debug(" tasks: %r" % tasks) - # now we organize this data into channel-arch bins - bin_hosts = {} # hosts indexed by bin - bins = {} # bins for this host - our_avail = None - for host in hosts: - host['bins'] = [] - if host['id'] == self.host_id: - # note: task_load reported by server might differ from what we - # sent due to precision variation - our_avail = host['capacity'] - host['task_load'] - for chan in host['channels']: - for arch in host['arches'].split() + ['noarch']: - bin = "%s:%s" % (chan, arch) - bin_hosts.setdefault(bin, []).append(host) - if host['id'] == self.host_id: - bins[bin] = 1 - self.logger.debug("bins: %r" % bins) - if our_avail is None: - self.logger.info("Server did not report this host. Are we disabled?") - return False - elif not bins: - self.logger.info("No bins for this host. Missing channel/arch config?") - # Note: we may still take an assigned task below - # sort available capacities for each of our bins - avail = {} - for bin in bins: - avail[bin] = [host['capacity'] - host['task_load'] for host in bin_hosts[bin]] - avail[bin].sort() - avail[bin].reverse() - self.cleanDelayTimes() + + # get our assigned tasks + tasks = self.session.host.getTasks() + self.logger.debug("Got tasks: %r", tasks) for task in tasks: - # note: tasks are in priority order self.logger.debug("task: %r" % task) - if task['method'] not in self.handlers: - self.logger.warning("Skipping task %(id)i, no handler for method %(method)s", task) - self.session.host.refuseTask(task['id'], soft=False, msg="no handler for method") - continue if task['id'] in self.tasks: # we were running this task, but it apparently has been - # freed or reassigned. We can't do anything with it until + # reassigned. We can't do anything with it until # updateTasks notices this and cleans up. - self.logger.debug("Task %(id)s freed or reassigned", task) + self.logger.info("Task %(id)s freed or reassigned", task) + continue + if task['state'] != koji.TASK_STATES['ASSIGNED']: + # shouldn't happen + self.logger.error("Recieved task %(id)s is not assigned, state=%(state)s", task) + continue + if task['host_id'] != self.host_id: + # shouldn't happen + self.logger.error("Recieved task %(id)s is not ours, host=%(host_id)s", task) continue - if task['state'] == koji.TASK_STATES['ASSIGNED']: - self.logger.debug("task is assigned") - if self.host_id == task['host_id']: - # assigned to us, we can take it regardless - if self.takeTask(task): - return True - elif task['state'] == koji.TASK_STATES['FREE']: - bin = "%(channel_id)s:%(arch)s" % task - self.logger.debug("task is free, bin=%r" % bin) - if bin not in bins: - continue - # see where our available capacity is compared to other hosts for this bin - # (note: the hosts in this bin are exactly those that could - # accept this task) - bin_avail = avail.get(bin, [0]) - if self.checkAvailDelay(task, bin_avail, our_avail): - # decline for now and give the upper half a chance - continue - # otherwise, we attempt to open the task - if self.takeTask(task): - return True - else: - # should not happen - raise Exception("Invalid task state reported by server") - return False - - def checkAvailDelay(self, task, bin_avail, our_avail): - """Check to see if we should still delay taking a task - - Returns True if we are still in the delay period and should skip the - task. Otherwise False (delay has expired). - """ - - now = time.time() - ts = self.skipped_tasks.get(task['id']) - if not ts: - ts = self.skipped_tasks[task['id']] = now - - # determine our normalized bin rank - for pos, cap in enumerate(bin_avail): - if our_avail >= cap: - break - if len(bin_avail) > 1: - rank = float(pos) / (len(bin_avail) - 1) - else: - rank = 0.0 - # so, 0.0 for highest available capacity, 1.0 for lowest - delay = getattr(self.options, 'task_avail_delay', 180) - delay *= rank + # otherwise attempt to take it + if self.takeTask(task): + return True - # return True if we should delay - if now - ts < delay: - self.logger.debug("skipping task %i, age=%s rank=%s" - % (task['id'], int(now - ts), rank)) - return True - # otherwise - del self.skipped_tasks[task['id']] return False - def cleanDelayTimes(self): - """Remove old entries from skipped_tasks""" - now = time.time() - delay = getattr(self.options, 'task_avail_delay', 180) - cutoff = now - delay * 10 - # After 10x the delay, we've had plenty of opportunity to take the - # task, so either it has already been taken or we can't take it. - for task_id in list(self.skipped_tasks): - ts = self.skipped_tasks[task_id] - if ts < cutoff: - del self.skipped_tasks[task_id] - def _waitTask(self, task_id, pid=None): """Wait (nohang) on the task, return true if finished""" if pid is None: @@ -1415,7 +1327,9 @@ class TaskManager(object): if method in self.handlers: handlerClass = self.handlers[method] else: - raise koji.GenericError("No handler found for method '%s'" % method) + self.logger.warning("Refusing task %(id)s, no handler for %(method)s", task) + self.session.host.refuseTask(task['id'], soft=False, msg="no handler for method") + return False task_info = self.session.getTaskInfo(task['id'], request=True) if task_info.get('request') is None: self.logger.warning("Task '%s' has no request" % task['id']) From dcfe0243c37c8c9305d52a006a470e75721b7863 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 15:49:08 +0000 Subject: [PATCH 2/51] have the builders nudge workflows --- diff --git a/koji/daemon.py b/koji/daemon.py index e343351..97f3744 100644 --- a/koji/daemon.py +++ b/koji/daemon.py @@ -1064,6 +1064,9 @@ class TaskManager(object): if self.takeTask(task): return True + # if we get no tasks, nudge the workflows + self.session.host.nudgeWork() + return False def _waitTask(self, task_id, pid=None): diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 217d71d..ba2e356 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -14789,6 +14789,11 @@ class HostExports(object): host.verify() return scheduler.get_tasks_for_host(hostID=host.id, retry=True) + def nudgeWork(self): + host = Host() + host.verify() + return workflow.nudge_queue() + def refuseTask(self, task_id, soft=True, msg=''): soft = convert_value(soft, cast=bool) msg = convert_value(msg, cast=str) From d7b15d65f9a4ff4920fa13de509544df8a14bff3 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 19:07:19 +0000 Subject: [PATCH 3/51] working on slots --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index d6c1e11..463318a 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -567,72 +567,82 @@ class TaskWait(BaseWait): class SlotWait(BaseWait): def check(self): - # we also have triggers to update these, but this is a fallback - params = self.info['params'] - query = QueryProcessor( - tables=['workflow_slots'], - columns=['id'], + # handle_slots will mark us fulfilled, so no point in further checking here + return False + + +def request_slot(name, workflow_id): + data = { + 'name': name, + 'workflow_id': workflow_id, + } + upsert = UpsertProcessor(table='workflow_slots', data=data, skip_dup=True) + upsert.execute() + # table has: UNIQUE (name, workflow_id) + # so this is a no-op if we already have a request, or are already holding the slot + + +def free_slot(name, workflow_id): + values = { + 'name': name, + 'workflow_id': workflow_id, + } + delete = DeleteProcessor( + table='workflow_slots', clauses=['name = %(name)s', 'workflow_id = %(workflow_id)s'], - values={'name': params['name'], 'workflow_id': self.info['workflow_id']}, - ) - slot_id = query.singleValue() - return (slot_id is not None) + values=values) + delete.execute() def handle_slots(): - """Check slot waits and see if we can fulfill them""" + """Check slot requests and see if we can grant them""" - query = WaitsQuery( - clauses=[ - ['fulfilled', 'IS', False], - ['wait_type', '=', 'slot'], - ], - opts={'order': 'id'}, # oldest first + query = QueryProcessor( + tables=['workflow_slots'], + columns=['id', 'name', 'workflow_id', 'held'], + opts=['order': 'id'], ) - by_name = {} - for wait in query.execute(): - name = wait['params']['name'] - by_name.setdefault(name, []).append(wait) - - for name in sorted(by_name): - query = QueryProcessor( - tables=['workflow_slots'], - columns=['id', 'num'], - clauses=['name = %(name)s'], - values={'name': name}, - ) + # index needed and held by name + need_idx = {} + held_idx = {} + for slot in query.execute(): + if slot['held']: + held_idx.setdefault(slot['name'], []).append(slot) + else: + need_idx.setdefault(slot['name'], []).append(slot) + + grants = [] + for name in need_idx: + need = need_idx[name] + held = held_idx.get(name, []) limit = 10 # XXX CONFIG - held = query.execute() - if len(held) >= limit: - # all in use - continue - waits = by_name[name] - held = set(held) - for num in range(limit): - if num in held: - continue - if not waits: - break - # try to take it - wait = waits[0] - data = { - 'name': name, - 'workflow_id': wait['workflow_id'], - 'num': num, - } - insert = InsertProcessor(table='workflow_slots', data=data) - savepoint = Savepoint('pre_slot_insert') - try: - insert.execute() - except Exception: - # there must be a parallel call - savepoint.rollback() - logger.debug('Failed to acquire workflow slot') - # XXX how do we avoid duplicate fulfillments by parallel instances? - continue - # success! pop this wait so next pass can handle the next - waits.pop(0) + while need and len(held) > limit: + slot = need.pop(0) # first come, first served + held.append(slot) + grants.append(slot) + + # update the slots + update = UpdateProcessor(table='workflow_slots', + clauses=['id IN %(ids)s'], + values={'ids': [s['id'] for s in grants]}) + update.set(held=True) + update.rawset(grant_time='NOW()') + update.execute() + + # also mark waits fulfilled + for slot in grants: + update = UpdateProcessor( + 'workflow_wait', + clauses=[ + "wait_type = 'slot'", + 'fulfilled IS FALSE', + 'workflow_id = %(workflow_id)s', + "(params->>'name') = %(name)s", # note the ->> + ], + values=slot) + update.set(fulfilled=True) + update.execute() @workflows.add('test') @@ -679,7 +689,7 @@ class NewRepoWorkflow(BaseWorkflow): class WorkflowExports: # TODO: would be nice to mimic our registry approach in kojixmlrpc - #handleWorkQueue = staticmethod(handle_work_queue) + # XXX most of these need access controls getQueue = staticmethod(get_queue) nudge = staticmethod(nudge_queue) updateQueue = staticmethod(update_queue) diff --git a/schemas/schema.sql b/schemas/schema.sql index 56576f7..2e1c226 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1081,10 +1081,12 @@ CREATE TABLE workflow_wait ( CREATE TABLE workflow_slots ( id SERIAL NOT NULL PRIMARY KEY, name TEXT, - num INTEGER NOT NULL, - UNIQUE (name, num), workflow_id INTEGER REFERENCES workflow(id), - create_time TIMESTAMPTZ NOT NULL DEFAULT NOW() + UNIQUE (name, workflow_id), + held BOOLEAN NOT FULL DEFAULT FALSE, + -- if held is False, that means the slot is requested + create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), + grant_time TIMETSTAMPTZ ) WITHOUT OIDS; From f26a3a1dfa232dc0422ee454607f570190e3887a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 19:08:15 +0000 Subject: [PATCH 4/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 463318a..f622d09 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -600,7 +600,7 @@ def handle_slots(): query = QueryProcessor( tables=['workflow_slots'], columns=['id', 'name', 'workflow_id', 'held'], - opts=['order': 'id'], + opts={'order': 'id'}, ) # index needed and held by name From 6c7b9b987a3edde6925e882ae0fb2a49e44f19da Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 19:53:32 +0000 Subject: [PATCH 5/51] attempt to handle slots in workflow --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index f622d09..b3bb8cd 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -268,9 +268,22 @@ class BaseWorkflow: # TODO error handling step = self.data['steps'].pop(0) + handler = getattr(self, step) + slot = getattr(handler, 'slot', None) + if slot: + # a note about timing. We don't request a slot until we're otherwise ready to run + # We don't want to hold a slot if we're waiting on something else. + if not has_slot(slot, self.info['id']): + self.wait_slot(slot) + return + logger.debug('We have slot %s. Proceeding.', slot) + logger.debug('Running %s step for workflow %s', step, self.info['id']) - func = getattr(self, step) - func() + handler() + + if slot: + # we only hold the slot during the execution of the step + free_slot(slot, self.info['id']) # are we done? if not self.data['steps']: @@ -383,6 +396,13 @@ class BaseWorkflow: if n: logger.error('Dangling waits for %(method)s workflow %(id)i', self.info) + # and similarly for slots + delete = DeleteProcessor('workflow_slots', clauses=['workflow_id = %(id)s'], + values=self.info) + n = delete.execute() + if n: + logger.error('Dangling slots for %(method)s workflow %(id)i', self.info) + update = UpdateProcessor('workflow', clauses=['id=%(id)s'], values=self.info) update.set(data=json.dumps(self.data)) update.rawset(update_time='NOW()') @@ -396,6 +416,9 @@ class BaseWorkflow: # we shouldn't have any waits but... update = UpdateProcessor('task', clauses=['id = %(stub_id)s'], values=self.info) update.set(state=koji.TASK_STATES[stub_state]) + update.rawset(completion_time='NOW()') + # TODO set a result for stub + # TODO use kojihub.Task so we get the callbacks right # TODO handle failure update.execute() @@ -571,6 +594,15 @@ class SlotWait(BaseWait): return False +def slot(name): + """Decorator to indicate that a step handler requires a slot""" + def decorator(handler): + handler.slot = name + return handler + + return decorator + + def request_slot(name, workflow_id): data = { 'name': name, @@ -594,6 +626,20 @@ def free_slot(name, workflow_id): delete.execute() +def has_slot(name, workflow_id): + values = { + 'name': name, + 'workflow_id': workflow_id, + } + query = QueryProcessor( + tables=['workflow_slots'], + columns=['id'], + clauses=['name = %(name)s', 'workflow_id = %(workflow_id)s', 'held IS TRUE'], + values=values, + ) + return query.singleValue() is not None + + def handle_slots(): """Check slot requests and see if we can grant them""" From cf3fbdd035f74fdada645c57864294df67a9ec8a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 20:00:02 +0000 Subject: [PATCH 6/51] typos --- diff --git a/schemas/schema.sql b/schemas/schema.sql index 2e1c226..d549f7e 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1083,10 +1083,10 @@ CREATE TABLE workflow_slots ( name TEXT, workflow_id INTEGER REFERENCES workflow(id), UNIQUE (name, workflow_id), - held BOOLEAN NOT FULL DEFAULT FALSE, + held BOOLEAN NOT NULL DEFAULT FALSE, -- if held is False, that means the slot is requested create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), - grant_time TIMETSTAMPTZ + grant_time TIMESTAMPTZ ) WITHOUT OIDS; From 279fba6150d80fda2b3f6458b3049b6335a6e508 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 20:12:24 +0000 Subject: [PATCH 7/51] slot test code --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index b3bb8cd..5307f42 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -662,7 +662,7 @@ def handle_slots(): for name in need_idx: need = need_idx[name] held = held_idx.get(name, []) - limit = 10 # XXX CONFIG + limit = 2 # XXX CONFIG while need and len(held) > limit: slot = need.pop(0) # first come, first served held.append(slot) @@ -699,11 +699,13 @@ class TestWorkflow(BaseWorkflow): STEPS = ['start', 'finish'] PARAMS = {'a': int, 'b': (int, type(None)), 'c': str} + @slot('FOO') def start(self): # fire off a do-nothing task logger.info('TEST WORKFLOW START') task_id = self.task('sleep', {'n': 1}) + @slot('BAR') def finish(self): # XXX how do we propagate task_id? logger.info('TEST WORKFLOW FINISH') From 3c0919dcb48877d383a8e7f7488cdadc554b5d73 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 21:35:07 +0000 Subject: [PATCH 8/51] slot tinkering --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 5307f42..7c43ce7 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -56,6 +56,7 @@ def nudge_queue(): # if we handled a queue item, we're done return update_queue() + handle_slots() # TODO figure out what we should return @@ -76,7 +77,7 @@ def queue_next(): # either empty queue or all locked return False - logger.debug('Handling work queue id %(id)s', job) + logger.debug('Handling work queue id %(id)s, workflow %(workflow_id)s', job) handle_job(job) # mark it done @@ -273,8 +274,8 @@ class BaseWorkflow: if slot: # a note about timing. We don't request a slot until we're otherwise ready to run # We don't want to hold a slot if we're waiting on something else. - if not has_slot(slot, self.info['id']): - self.wait_slot(slot) + if not get_slot(slot, self.info['id']): + self.wait_slot(slot, request=False) # get_slot made the request for us return logger.debug('We have slot %s. Proceeding.', slot) @@ -305,6 +306,7 @@ class BaseWorkflow: # also open our stub task # we don't worry about checks here because the entry is just a stub update = UpdateProcessor('task', clauses=['id = %(stub_id)s'], values=self.info) + # TODO integrate with kojihub.Task update.set(state=koji.TASK_STATES['OPEN']) update.execute() @@ -383,6 +385,11 @@ class BaseWorkflow: if wait: self.wait_task(task_id) + def wait_slot(self, name, request=True): + self.wait('slot', {'name': name}) + if request: + request_slot(name, self.info['id']) + def start(self): raise NotImplementedError('start method not defined') @@ -604,6 +611,7 @@ def slot(name): def request_slot(name, workflow_id): + logger.info('Requesting %s slot for workflow %i', name, workflow_id) data = { 'name': name, 'workflow_id': workflow_id, @@ -615,6 +623,7 @@ def request_slot(name, workflow_id): def free_slot(name, workflow_id): + logger.info('Freeing %s slot for workflow %i', name, workflow_id) values = { 'name': name, 'workflow_id': workflow_id, @@ -626,23 +635,44 @@ def free_slot(name, workflow_id): delete.execute() -def has_slot(name, workflow_id): +def get_slot(name, workflow_id): + """Check for and/or attempt to acquire slot + + :returns: True if slot is held, False otherwise + + If False, then the slot is *requested* + """ values = { 'name': name, 'workflow_id': workflow_id, } query = QueryProcessor( tables=['workflow_slots'], - columns=['id'], - clauses=['name = %(name)s', 'workflow_id = %(workflow_id)s', 'held IS TRUE'], + columns=['id', 'held'], + clauses=['name = %(name)s', 'workflow_id = %(workflow_id)s'], values=values, ) - return query.singleValue() is not None + slot = query.executeOne() + if not slot: + request_slot(name, workflow_id) + elif slot['held']: + return True + + handle_slots() # XXX? + + # check again + slot = query.executeOne() + return slot and slot['held'] def handle_slots(): """Check slot requests and see if we can grant them""" + if not db_lock('workflow_slots', wait=False): + return + + logger.debug('Checking slots') + query = QueryProcessor( tables=['workflow_slots'], columns=['id', 'name', 'workflow_id', 'held'], @@ -652,7 +682,8 @@ def handle_slots(): # index needed and held by name need_idx = {} held_idx = {} - for slot in query.execute(): + slots = query.execute() + for slot in slots: if slot['held']: held_idx.setdefault(slot['name'], []).append(slot) else: @@ -662,21 +693,23 @@ def handle_slots(): for name in need_idx: need = need_idx[name] held = held_idx.get(name, []) - limit = 2 # XXX CONFIG - while need and len(held) > limit: + limit = 3 # XXX CONFIG + logger.debug('Slot %s: need %i, held %i', name, len(need), len(held)) + while need and len(held) < limit: slot = need.pop(0) # first come, first served held.append(slot) grants.append(slot) # update the slots - update = UpdateProcessor(table='workflow_slots', - clauses=['id IN %(ids)s'], - values={'ids': [s['id'] for s in grants]}) - update.set(held=True) - update.rawset(grant_time='NOW()') - update.execute() + if grants: + update = UpdateProcessor(table='workflow_slots', + clauses=['id IN %(ids)s'], + values={'ids': [s['id'] for s in grants]}) + update.set(held=True) + update.rawset(grant_time='NOW()') + update.execute() - # also mark waits fulfilled + # also mark any waits fulfilled for slot in grants: update = UpdateProcessor( 'workflow_wait', diff --git a/schemas/schema.sql b/schemas/schema.sql index d549f7e..fcd9269 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1107,5 +1107,6 @@ CREATE TABLE locks ( INSERT INTO locks(name) VALUES('protonmsg-plugin'); INSERT INTO locks(name) VALUES('scheduler'); INSERT INTO locks(name) VALUES('work_queue'); +INSERT INTO locks(name) VALUES('workflow_slots'); COMMIT WORK; From 0afd271d07e6f7e840085c74b9d74510c830274d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 21:42:41 +0000 Subject: [PATCH 9/51] sleeping tweaks --- diff --git a/builder/kojid b/builder/kojid index 1f72e77..e1c9354 100755 --- a/builder/kojid +++ b/builder/kojid @@ -181,8 +181,6 @@ def main(options, session): try: if not taken: # Only sleep if we didn't take a task, otherwise retry immediately. - # The load-balancing code in getNextTask() will prevent a single builder - # from getting overloaded. time.sleep(options.sleeptime) except (SystemExit, KeyboardInterrupt): logger.warning("Exiting") diff --git a/koji/daemon.py b/koji/daemon.py index 97f3744..78b5387 100644 --- a/koji/daemon.py +++ b/koji/daemon.py @@ -1065,7 +1065,9 @@ class TaskManager(object): return True # if we get no tasks, nudge the workflows - self.session.host.nudgeWork() + if self.session.host.nudgeWork(): + # if a workflow ran, tell main not to sleep + return True return False diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 7c43ce7..c1a8786 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -54,10 +54,11 @@ def nudge_queue(): """Run next queue entry, or attempt to refill queue""" if queue_next(): # if we handled a queue item, we're done - return + return True update_queue() handle_slots() - # TODO figure out what we should return + return False + # TODO should we return something more informative? def queue_next(): From 2d21df121744c84cedc8465de8cc64a26f5f26a0 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 22:13:34 +0000 Subject: [PATCH 10/51] hooking into scheduler logs --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index c1a8786..2c200fe 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -5,6 +5,7 @@ import time import koji from koji.context import context from . import kojihub +from .scheduler import log_db, log_both from .db import QueryProcessor, InsertProcessor, UpsertProcessor, UpdateProcessor, \ DeleteProcessor, QueryView, db_lock, nextval, Savepoint @@ -280,7 +281,7 @@ class BaseWorkflow: return logger.debug('We have slot %s. Proceeding.', slot) - logger.debug('Running %s step for workflow %s', step, self.info['id']) + self.log(f'Running workflow step {step}') handler() if slot: @@ -300,6 +301,9 @@ class BaseWorkflow: # update the db self.update() + def log(self, msg, level=logging.INFO): + log_both(msg, task_id=self.info['stub_id'], level=level) + def setup(self): """Called to set up the workflow run""" logger.debug('Setting up workflow: %r', self.info) @@ -365,6 +369,7 @@ class BaseWorkflow: self.wait('task', {'task_id': task_id}) def wait(self, wait_type, params): # TODO maybe **params? + self.log(f'Waiting for {wait_type}: {params}') data = { 'workflow_id': self.info['id'], 'wait_type': wait_type, @@ -396,7 +401,7 @@ class BaseWorkflow: def close(self, result='complete', stub_state='CLOSED'): # TODO - the result field needs to be handled better - logger.info('Closing %(method)s workflow %(id)i', self.info) + self.log(f'Closing {method} workflow') # we shouldn't have any waits but... delete = DeleteProcessor('workflow_wait', clauses=['workflow_id = %(id)s'], values=self.info) @@ -435,6 +440,7 @@ class BaseWorkflow: self.close(result='canceled', stub_state='CANCELED') def requeue(self): + self.log('Queuing %(method)s workflow') insert = InsertProcessor('work_queue', data={'workflow_id': self.info['id']}) insert.execute() @@ -500,6 +506,7 @@ def add_workflow(method, params, queue=True): } insert = InsertProcessor('workflow', data=data) insert.execute() + log_both(f'Adding {method} workflow', task_id=stub_id) if queue: # also add it to the work queue so it will start From 71708e65106bcc312c2ace9cbf15d0948b3fb7cd Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 22:16:36 +0000 Subject: [PATCH 11/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 2c200fe..bdc0b72 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -401,7 +401,7 @@ class BaseWorkflow: def close(self, result='complete', stub_state='CLOSED'): # TODO - the result field needs to be handled better - self.log(f'Closing {method} workflow') + self.log('Closing %(method)s workflow' % self.info) # we shouldn't have any waits but... delete = DeleteProcessor('workflow_wait', clauses=['workflow_id = %(id)s'], values=self.info) @@ -440,7 +440,7 @@ class BaseWorkflow: self.close(result='canceled', stub_state='CANCELED') def requeue(self): - self.log('Queuing %(method)s workflow') + self.log('Queuing %(method)s workflow' % self.info) insert = InsertProcessor('work_queue', data={'workflow_id': self.info['id']}) insert.execute() From 57c1fdb4588ba401e16b1e881d65401fe95fca2c Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 22:23:11 +0000 Subject: [PATCH 12/51] slot test tweaks --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index bdc0b72..876fa6d 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -740,15 +740,14 @@ class TestWorkflow(BaseWorkflow): STEPS = ['start', 'finish'] PARAMS = {'a': int, 'b': (int, type(None)), 'c': str} - @slot('FOO') def start(self): # fire off a do-nothing task logger.info('TEST WORKFLOW START') - task_id = self.task('sleep', {'n': 1}) + self.data['task_id'] = self.task('sleep', {'n': 1}) - @slot('BAR') + @slot('test-sleep') def finish(self): - # XXX how do we propagate task_id? + time.sleep(10) logger.info('TEST WORKFLOW FINISH') From 59610bd300400cf9fbeb80fb8c129926edffe1c7 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 29 2024 22:56:02 +0000 Subject: [PATCH 13/51] typo --- diff --git a/koji/__init__.py b/koji/__init__.py index d582af2..6113c4b 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -3753,7 +3753,7 @@ def _taskLabel(taskInfo): # at this place (e.g. client without knowledge of such signatures) # it should still display at least "method (arch)" return '%s (%s)' % (method, arch) - except koji.ParameterError: + except ParameterError: return '%s (invalid parameters)' % method From af2c762cf6c38641dff376a1ec353f0571472924 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 04:53:19 +0000 Subject: [PATCH 14/51] auto-fill step args --- diff --git a/builder/kojid b/builder/kojid index e1c9354..ace31a2 100755 --- a/builder/kojid +++ b/builder/kojid @@ -6698,7 +6698,7 @@ def get_options(): 'allow_noverifyssl': False, 'allow_password_in_scm_url': False, 'methods': None, - } + } if config.has_section('kojid'): for name, value in config.items('kojid'): if name in ['sleeptime', 'maxjobs', 'minspace', 'retry_interval', diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 876fa6d..0471def 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -1,3 +1,4 @@ +import inspect import json import logging import time @@ -5,9 +6,9 @@ import time import koji from koji.context import context from . import kojihub -from .scheduler import log_db, log_both +from .scheduler import log_both from .db import QueryProcessor, InsertProcessor, UpsertProcessor, UpdateProcessor, \ - DeleteProcessor, QueryView, db_lock, nextval, Savepoint + DeleteProcessor, QueryView, db_lock, nextval logger = logging.getLogger('koji.workflow') @@ -281,8 +282,23 @@ class BaseWorkflow: return logger.debug('We have slot %s. Proceeding.', slot) + # auto-fill handler params + kwargs = {} + params = inspect.signature(handler).parameters + for key in params: + param = params[key] + if param.kind in (param.VAR_POSITIONAL, param.VAR_KEYWORD): + # step handlers shouldn't use these, but we'll be nice + logger.warning('Ignoring variable args for %s', handler) + continue + if key in self.params: + kwargs[key] = self.params[key] + elif key in self.data: + kwargs[key] = self.data[key] + self.log(f'Running workflow step {step}') - handler() + logger.debug('Step args: %r', kwargs) + handler(**kwargs) if slot: # we only hold the slot during the execution of the step @@ -402,19 +418,11 @@ class BaseWorkflow: def close(self, result='complete', stub_state='CLOSED'): # TODO - the result field needs to be handled better self.log('Closing %(method)s workflow' % self.info) - # we shouldn't have any waits but... - delete = DeleteProcessor('workflow_wait', clauses=['workflow_id = %(id)s'], - values=self.info) - n = delete.execute() - if n: - logger.error('Dangling waits for %(method)s workflow %(id)i', self.info) - - # and similarly for slots - delete = DeleteProcessor('workflow_slots', clauses=['workflow_id = %(id)s'], - values=self.info) - n = delete.execute() - if n: - logger.error('Dangling slots for %(method)s workflow %(id)i', self.info) + + for table in ('workflow_wait', 'workflow_slots', 'work_queue'): + delete = DeleteProcessor(table, clauses=['workflow_id = %(id)s'], + values=self.info) + delete.execute() update = UpdateProcessor('workflow', clauses=['id=%(id)s'], values=self.info) update.set(data=json.dumps(self.data)) @@ -740,7 +748,7 @@ class TestWorkflow(BaseWorkflow): STEPS = ['start', 'finish'] PARAMS = {'a': int, 'b': (int, type(None)), 'c': str} - def start(self): + def start(self, a, b): # fire off a do-nothing task logger.info('TEST WORKFLOW START') self.data['task_id'] = self.task('sleep', {'n': 1}) @@ -754,13 +762,19 @@ class TestWorkflow(BaseWorkflow): @workflows.add('new-repo') class NewRepoWorkflow(BaseWorkflow): - STEPS = ['start', 'repos', 'finalize'] + STEPS = ['init', 'repos', 'finalize'] + PARAMS = { + 'tag': (int, str, dict), + 'event': (int,), + 'opts': (dict,), + } - def start(self): - # TODO validate params + @slot('repo-init') + def init(self, tag, event=None, opts=None): + tinfo = kojihub.get_tag(tag, strict=True, event=event) kw = self.params # ??? should we call repo_init ourselves? - task_id = self.task('initRepo', kw) + self.data['task_id'] = self.task('initRepo', kw) # TODO mechanism for task_id value to persist to next step def repos(self): From 38593826454feeccea9fbfbaa0cdb010e2ca5fbd Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 05:42:21 +0000 Subject: [PATCH 15/51] first stab at subtask steps --- diff --git a/koji/tasks.py b/koji/tasks.py index 1cbfb80..cc2a48b 100644 --- a/koji/tasks.py +++ b/koji/tasks.py @@ -286,6 +286,9 @@ LEGACY_SIGNATURES = { # these are stub tasks created by workflows [['method', 'params', 'workflow_id'], None, None, (None,)], ], + 'workflowStep': [ + [['workflow_id', 'step'], None, None, (None,)], + ], } @@ -931,3 +934,16 @@ class WaitrepoTask(BaseTaskHandler): time.sleep(self.PAUSE) last_repo = repo + + +class WorkflowStepTask(BaseTaskHandler): + + Methods = ['workflowStep'] + # all we do is make a hub call + _taskWeight = 0.1 + + def handler(self, workflow_id, step): + self.session.hub.workflowStep(workflow_id, step) + + +# the end diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index ba2e356..d3fa770 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -14794,6 +14794,11 @@ class HostExports(object): host.verify() return workflow.nudge_queue() + def workflowStep(self, workflow_id, step): + host = Host() + host.verify() + return workflow.run_subtask_step(workflow_id, step) + def refuseTask(self, task_id, soft=True, msg=''): soft = convert_value(soft, cast=bool) msg = convert_value(msg, cast=str) diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 0471def..ca3cfa1 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -208,6 +208,7 @@ class WorkflowQuery(QueryView): def handle_job(job): + # TODO row lock wf = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).executeOne(strict=True) if wf['completed']: logger.error('Ignoring completed %(method)s workflow in queue: %(id)i', wf) @@ -223,6 +224,16 @@ def handle_job(job): raise # XXX +def run_subtask_step(workflow_id, step): + # TODO row lock + wf = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).executeOne(strict=True) + if wf['completed']: + raise koji.GenericError('Workflow is completed') + cls = workflows.get(wf['method']) + handler = cls(wf) + handler.run(subtask_step=step) + + def handle_error(job, err): # for now we mark it completed but include the error # TODO retries? @@ -265,14 +276,28 @@ class BaseWorkflow: self.data = info['data'] self.waiting = False - def run(self): + def run(self, subtask_step=None): if self.data is None: self.setup() # TODO error handling step = self.data['steps'].pop(0) - handler = getattr(self, step) + + is_subtask = getattr(handler, 'subtask', False) + if subtask_step is not None: + # we've been called via a workflowStep task + if subtask_step != step: + raise koji.GenericError(f'Step mismatch {subtask_step} != {step}') + elif not is_subtask: + raise koji.GenericError(f'Not a subtask step: {step}') + # otherwise we're good + elif is_subtask: + # this step needs to run via a subtask + self.task('workflowStep', {'workflow_id': self.info['id'], 'step': step}) + return + + # TODO slots are a better idea for tasks than for workflows slot = getattr(handler, 'slot', None) if slot: # a note about timing. We don't request a slot until we're otherwise ready to run @@ -459,6 +484,15 @@ class BaseWorkflow: update.execute() +def subtask() + """Decorator to indicate that a step handler should run via a subtask""" + def decorator(handler): + handler.subtask = True + return handler + + return decorator + + class ParamSpec: def __init__(self, rule, required=False): From 888294136c420468a2b11d01678e2a726b5e595d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 05:44:04 +0000 Subject: [PATCH 16/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index ca3cfa1..3132a08 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -484,7 +484,7 @@ class BaseWorkflow: update.execute() -def subtask() +def subtask(): """Decorator to indicate that a step handler should run via a subtask""" def decorator(handler): handler.subtask = True From 0e56e66bb95613da103d89eeba74bd64fa4dd20f Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 20:22:13 +0000 Subject: [PATCH 17/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 3132a08..f15916d 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -485,6 +485,7 @@ class BaseWorkflow: def subtask(): + # TODO args? """Decorator to indicate that a step handler should run via a subtask""" def decorator(handler): handler.subtask = True @@ -787,7 +788,7 @@ class TestWorkflow(BaseWorkflow): logger.info('TEST WORKFLOW START') self.data['task_id'] = self.task('sleep', {'n': 1}) - @slot('test-sleep') + @subtask() def finish(self): time.sleep(10) logger.info('TEST WORKFLOW FINISH') From 54ad664ce7f815d4c5844afbdae2a07c1f10a51b Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 20:25:48 +0000 Subject: [PATCH 18/51] ... --- diff --git a/koji/tasks.py b/koji/tasks.py index cc2a48b..9eaa343 100644 --- a/koji/tasks.py +++ b/koji/tasks.py @@ -943,7 +943,7 @@ class WorkflowStepTask(BaseTaskHandler): _taskWeight = 0.1 def handler(self, workflow_id, step): - self.session.hub.workflowStep(workflow_id, step) + self.session.host.workflowStep(workflow_id, step) # the end From 50ed3ad5bbe0825c883c004d5d9cab3518d45234 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 01 2024 20:29:20 +0000 Subject: [PATCH 19/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index f15916d..13f7278 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -226,7 +226,7 @@ def handle_job(job): def run_subtask_step(workflow_id, step): # TODO row lock - wf = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).executeOne(strict=True) + wf = WorkflowQuery(clauses=[['id', '=', workflow_id]]).executeOne(strict=True) if wf['completed']: raise koji.GenericError('Workflow is completed') cls = workflows.get(wf['method']) From e42666e60d4f7082f942c5364e51a378583c4108 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 02 2024 23:09:52 +0000 Subject: [PATCH 20/51] step decorator experiment --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 13f7278..e9897c0 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -282,7 +282,7 @@ class BaseWorkflow: # TODO error handling step = self.data['steps'].pop(0) - handler = getattr(self, step) + handler = self.get_handler(step) is_subtask = getattr(handler, 'subtask', False) if subtask_step is not None: @@ -316,7 +316,9 @@ class BaseWorkflow: # step handlers shouldn't use these, but we'll be nice logger.warning('Ignoring variable args for %s', handler) continue - if key in self.params: + if key == 'workflow': + kwargs[key] = self + elif key in self.params: kwargs[key] = self.params[key] elif key in self.data: kwargs[key] = self.data[key] @@ -356,6 +358,35 @@ class BaseWorkflow: update.set(state=koji.TASK_STATES['OPEN']) update.execute() + @classmethod + def step(cls, name=None): + """Decorator to add steps outside of class""" + # note this can't be used IN the class definition + steps = getattr(cls, 'STEPS', None) + if steps is None: + steps = cls.STEPS = [] + handlers = getattr(cls, '_step_handlers', None) + if handlers is None: + handlers = cls._step_handlers = {} + + def decorator(func): + nonlocal name + # also updates nonlocal steps + if name is None: + name = func.__name__ + steps.append(name) + handlers[name] = func + return func + + return decorator + + def get_handler(self, step): + handlers = getattr(self, '_step_handlers', {}) + if handlers and step in handlers: + return handlers[step] + else: + return getattr(self, step) + def get_steps(self): """Get the initial list of steps @@ -780,18 +811,20 @@ class TestWorkflow(BaseWorkflow): # XXX remove this test code - STEPS = ['start', 'finish'] + # STEPS = ['start', 'finish'] PARAMS = {'a': int, 'b': (int, type(None)), 'c': str} - def start(self, a, b): - # fire off a do-nothing task - logger.info('TEST WORKFLOW START') - self.data['task_id'] = self.task('sleep', {'n': 1}) - - @subtask() - def finish(self): - time.sleep(10) - logger.info('TEST WORKFLOW FINISH') +@TestWorkflow.step() +def start(workflow, a, b): + # fire off a do-nothing task + logger.info('TEST WORKFLOW START') + workflow.data['task_id'] = workflow.task('sleep', {'n': 1}) + +@subtask() +@TestWorkflow.step() +def finish(): + time.sleep(10) + logger.info('TEST WORKFLOW FINISH') @workflows.add('new-repo') From f4f1898a2abbe60c0df178b98b006ba6119d6de5 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 04:40:50 +0000 Subject: [PATCH 21/51] get row lock on workflows --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index e9897c0..6da8a48 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -209,7 +209,9 @@ class WorkflowQuery(QueryView): def handle_job(job): # TODO row lock - wf = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).executeOne(strict=True) + query = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).query + query.lock = True # we must have a lock on the workflow before attempting to run it + wf = query.executeOne(strict=True) if wf['completed']: logger.error('Ignoring completed %(method)s workflow in queue: %(id)i', wf) logger.debug('Data: %r', wf) @@ -225,8 +227,9 @@ def handle_job(job): def run_subtask_step(workflow_id, step): - # TODO row lock - wf = WorkflowQuery(clauses=[['id', '=', workflow_id]]).executeOne(strict=True) + query = WorkflowQuery(clauses=[['id', '=', workflow_id]]) + query.lock = True # we must have a lock on the workflow before attempting to run it + wf = query.executeOne(strict=True) if wf['completed']: raise koji.GenericError('Workflow is completed') cls = workflows.get(wf['method']) From 8448cf132bcdc71544b5a0a8c7d160eaae5bebf2 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 06:01:46 +0000 Subject: [PATCH 22/51] use Task() for task changes --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index d3fa770..fabe777 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -226,26 +226,42 @@ class Task(object): if not self.verifyOwner(user_id): raise koji.ActionNotAllowed("user %d does not own task %d" % (user_id, self.id)) - def lock(self, host_id, newstate='OPEN', force=False): + def lock(self, host_id, newstate='OPEN', force=False, workflow_id=None): """Attempt to associate the task for host, either to assign or open + :param int host_id: id of host, can also be None + :param str newstate: task state to set, as a string, default: "OPEN" + :param bool force: force operation, default False + :param workflow_id: workflow id, default None + + If host_id is specfied as None, workflow_id must be given. + If workflow_id is given, it must match the task. + returns True if successful, False otherwise""" info = self.getInfo(request=True) self.runCallbacks('preTaskStateChange', info, 'state', koji.TASK_STATES[newstate]) - self.runCallbacks('preTaskStateChange', info, 'host_id', host_id) + if workflow_id is not None: + host_id = None + if host_id is not None: + self.runCallbacks('preTaskStateChange', info, 'host_id', host_id) # we use row-level locks to keep things sane # note the QueryProcessor...opts={'rowlock': True} task_id = self.id if not force: - query = QueryProcessor(columns=['state', 'host_id'], tables=['task'], + query = QueryProcessor(columns=['state', 'host_id', 'workflow_id'], tables=['task'], clauses=['id=%(task_id)s'], values={'task_id': task_id}, - opts={'rowlock': True}) + lock=True) r = query.executeOne() if not r: raise koji.GenericError("No such task: %i" % task_id) state = r['state'] otherhost = r['host_id'] - if state == koji.TASK_STATES['FREE']: + if workflow_id is not None: + if workflow_id != r['workflow_id']: + # should not happen + raise ValueError('workflow id mismatch') + # otherwise a workflow can manage its own stub + elif state == koji.TASK_STATES['FREE']: if otherhost is not None: log_error(f"Error: task {task_id} is both free " f"and handled by host {otherhost}") @@ -282,6 +298,7 @@ class Task(object): # if we reach here, task is either # - free and unlocked # - assigned to host_id + # - a workflow stub # - force option is enabled state = koji.TASK_STATES[newstate] update = UpdateProcessor('task', clauses=['id=%(task_id)i'], values=locals()) @@ -290,7 +307,8 @@ class Task(object): update.rawset(start_time='NOW()') update.execute() self.runCallbacks('postTaskStateChange', info, 'state', koji.TASK_STATES[newstate]) - self.runCallbacks('postTaskStateChange', info, 'host_id', host_id) + if host_id is not None: + self.runCallbacks('postTaskStateChange', info, 'host_id', host_id) return True def assign(self, host_id, force=False): @@ -299,11 +317,11 @@ class Task(object): returns True if successful, False otherwise""" return self.lock(host_id, 'ASSIGNED', force) - def open(self, host_id): + def open(self, host_id=None, workflow_id=None): """Attempt to open the task for host. returns task data if successful, None otherwise""" - if self.lock(host_id, 'OPEN'): + if self.lock(host_id, 'OPEN', workflow_id=workflow_id): # get more complete data to return fields = self.fields + (('task.request', 'request'),) query = QueryProcessor(tables=['task'], clauses=['id=%(id)i'], values=vars(self), diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 6da8a48..b140453 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -298,6 +298,7 @@ class BaseWorkflow: elif is_subtask: # this step needs to run via a subtask self.task('workflowStep', {'workflow_id': self.info['id'], 'step': step}) + # TODO handle task failure without looping return # TODO slots are a better idea for tasks than for workflows @@ -355,11 +356,8 @@ class BaseWorkflow: logger.debug('Setting up workflow: %r', self.info) self.data = {'steps': self.get_steps()} # also open our stub task - # we don't worry about checks here because the entry is just a stub - update = UpdateProcessor('task', clauses=['id = %(stub_id)s'], values=self.info) - # TODO integrate with kojihub.Task - update.set(state=koji.TASK_STATES['OPEN']) - update.execute() + stub = kojihub.Task(self.info['stub_id']) + stub.open(workflow_id=self.info['id']) @classmethod def step(cls, name=None): @@ -493,14 +491,9 @@ class BaseWorkflow: # also close our stub task # we don't worry about checks here because the entry is just a stub logger.info('Closing workflow task %(stub_id)i', self.info) - # we shouldn't have any waits but... - update = UpdateProcessor('task', clauses=['id = %(stub_id)s'], values=self.info) - update.set(state=koji.TASK_STATES[stub_state]) - update.rawset(completion_time='NOW()') - # TODO set a result for stub - # TODO use kojihub.Task so we get the callbacks right + stub = kojihub.Task(self.info['stub_id']) + stub._close(result, stub_state) # TODO handle failure - update.execute() def cancel(self): # TODO we need to do more here, but for now From 80ade4669c3eec8dbc3e7a6a29aa5d2967cc2d06 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 06:25:02 +0000 Subject: [PATCH 23/51] ... --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index fabe777..bbf698f 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -226,21 +226,21 @@ class Task(object): if not self.verifyOwner(user_id): raise koji.ActionNotAllowed("user %d does not own task %d" % (user_id, self.id)) - def lock(self, host_id, newstate='OPEN', force=False, workflow_id=None): + def lock(self, host_id, newstate='OPEN', force=False, workflow=False): """Attempt to associate the task for host, either to assign or open :param int host_id: id of host, can also be None :param str newstate: task state to set, as a string, default: "OPEN" :param bool force: force operation, default False - :param workflow_id: workflow id, default None + :param bool workflow: task is a workflow stub - If host_id is specfied as None, workflow_id must be given. - If workflow_id is given, it must match the task. + If host_id is specfied as None, workflow must be True + If workflow is True, the task must be a workflow stub returns True if successful, False otherwise""" info = self.getInfo(request=True) self.runCallbacks('preTaskStateChange', info, 'state', koji.TASK_STATES[newstate]) - if workflow_id is not None: + if workflow: host_id = None if host_id is not None: self.runCallbacks('preTaskStateChange', info, 'host_id', host_id) @@ -248,7 +248,7 @@ class Task(object): # note the QueryProcessor...opts={'rowlock': True} task_id = self.id if not force: - query = QueryProcessor(columns=['state', 'host_id', 'workflow_id'], tables=['task'], + query = QueryProcessor(columns=['state', 'host_id', 'is_workflow'], tables=['task'], clauses=['id=%(task_id)s'], values={'task_id': task_id}, lock=True) r = query.executeOne() @@ -256,11 +256,11 @@ class Task(object): raise koji.GenericError("No such task: %i" % task_id) state = r['state'] otherhost = r['host_id'] - if workflow_id is not None: - if workflow_id != r['workflow_id']: + if workflow: + # workflows can manage their stubs + if not r['is_workflow']: # should not happen - raise ValueError('workflow id mismatch') - # otherwise a workflow can manage its own stub + raise ValueError(f'task {self.id} is not a workflow') elif state == koji.TASK_STATES['FREE']: if otherhost is not None: log_error(f"Error: task {task_id} is both free " @@ -317,11 +317,11 @@ class Task(object): returns True if successful, False otherwise""" return self.lock(host_id, 'ASSIGNED', force) - def open(self, host_id=None, workflow_id=None): - """Attempt to open the task for host. + def open(self, host_id=None, workflow=False): + """Attempt to open the task for host or workflow returns task data if successful, None otherwise""" - if self.lock(host_id, 'OPEN', workflow_id=workflow_id): + if self.lock(host_id, 'OPEN', workflow=workflow): # get more complete data to return fields = self.fields + (('task.request', 'request'),) query = QueryProcessor(tables=['task'], clauses=['id=%(id)i'], values=vars(self), diff --git a/kojihub/workflow.py b/kojihub/workflow.py index b140453..0f63aea 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -357,7 +357,7 @@ class BaseWorkflow: self.data = {'steps': self.get_steps()} # also open our stub task stub = kojihub.Task(self.info['stub_id']) - stub.open(workflow_id=self.info['id']) + stub.open(workflow=True) @classmethod def step(cls, name=None): From be63889adc52f9913cdec882a196bc17d733e779 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 06:32:44 +0000 Subject: [PATCH 24/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 0f63aea..e26c5b0 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -492,7 +492,7 @@ class BaseWorkflow: # we don't worry about checks here because the entry is just a stub logger.info('Closing workflow task %(stub_id)i', self.info) stub = kojihub.Task(self.info['stub_id']) - stub._close(result, stub_state) + stub._close(result, koji.TASK_STATES[stub_state]) # TODO handle failure def cancel(self): From 08b9ada7b2c8292f8721e8548fb8fdb3e12a9f93 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 06:53:03 +0000 Subject: [PATCH 25/51] make sure we encode stub result --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index bbf698f..9d20114 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -390,15 +390,26 @@ class Task(object): for (child_id,) in query.execute(): Task(child_id).setPriority(priority, recurse=True) - def _close(self, result, state): + def _close(self, result, state, encode=False): """Mark task closed and set response + :param result: the task result to set + :type result: str or object + :param state: the state to set + :type state: int + :param encode: whether to encode the result, default: False + :type encode: bool + + If encode is False (the default), the result should already be encoded + Returns True if successful, False if not""" # access checks should be performed by calling function # this is an approximation, and will be different than what is in the database # the actual value should be retrieved from the 'new' value of the post callback now = time.time() info = self.getInfo(request=True) + if encode: + result = koji.xmlrpcplus.dumps((result,), methodresponse=1, allow_none=1) info['result'] = result self.runCallbacks('preTaskStateChange', info, 'state', state) self.runCallbacks('preTaskStateChange', info, 'completion_ts', now) diff --git a/kojihub/workflow.py b/kojihub/workflow.py index e26c5b0..b1ff599 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -163,6 +163,7 @@ def handle_waits(): update.set(handled=True) update.execute() + # XXX we should not mark them handled until workflow actually runs for info in handled: logger.info('Handled %(wait_type)s wait %(id)s for workflow %(workflow_id)s', info) if handled: @@ -492,7 +493,7 @@ class BaseWorkflow: # we don't worry about checks here because the entry is just a stub logger.info('Closing workflow task %(stub_id)i', self.info) stub = kojihub.Task(self.info['stub_id']) - stub._close(result, koji.TASK_STATES[stub_state]) + stub._close(result, koji.TASK_STATES[stub_state], encode=True) # TODO handle failure def cancel(self): From e6a95561a150b6882705a471e25caf1817560ba7 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 15:41:44 +0000 Subject: [PATCH 26/51] rename table --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index b1ff599..def89a0 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -29,19 +29,19 @@ Starting with basic tasks, e.g. class WorkQueueQuery(QueryView): - tables = ['work_queue'] + tables = ['workflow_queue'] #joinmap = { - # 'workflow': 'workflow ON work_queue.workflow_id = workflow.id', + # 'workflow': 'workflow ON workflow_queue.workflow_id = workflow.id', #} fieldmap = { - 'id': ['work_queue.id', None], - 'workflow_id': ['work_queue.workflow_id', None], - 'create_time': ['work_queue.create_time', None], - 'create_ts': ["date_part('epoch', work_queue.create_time)", None], - 'completion_time': ['work_queue.completion_time', None], - 'completion_ts': ["date_part('epoch', work_queue.completion_time)", None], - 'completed': ['work_queue.completed', None], - 'error': ['work_queue.error', None], + 'id': ['workflow_queue.id', None], + 'workflow_id': ['workflow_queue.workflow_id', None], + 'create_time': ['workflow_queue.create_time', None], + 'create_ts': ["date_part('epoch', workflow_queue.create_time)", None], + 'completion_time': ['workflow_queue.completion_time', None], + 'completion_ts': ["date_part('epoch', workflow_queue.completion_time)", None], + 'completed': ['workflow_queue.completed', None], + 'error': ['workflow_queue.error', None], } default_fields = ('id', 'workflow_id', 'create_ts', 'completion_ts', 'completed', 'error') @@ -69,7 +69,7 @@ def queue_next(): :returns: True if an entry ran, False otherwise """ # TODO maybe use scheduler logging mechanism? - query = QueryProcessor(tables=['work_queue'], + query = QueryProcessor(tables=['workflow_queue'], columns=['id', 'workflow_id'], clauses=['completed IS FALSE'], opts={'order': 'id', 'limit': 1}, @@ -84,7 +84,7 @@ def queue_next(): handle_job(job) # mark it done - update = UpdateProcessor('work_queue', clauses=['id=%(id)s'], values=job) + update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=job) update.set(completed=True) update.rawset(completion_time='NOW()') update.execute() @@ -102,7 +102,7 @@ def clean_queue(): logger.debug('Cleaning old queue entries') lifetime = 3600 # XXX config delete = DeleteProcessor( - table='work_queue', + table='workflow_queue', values={'age': f'{lifetime} seconds'}, clauses=['completed IS TRUE', "completion_time < NOW() - %(age)s::interval"], ) @@ -178,7 +178,7 @@ def handle_waits(): for workflow_id in requeue: logger.info('Re-queueing workflow %s', workflow_id) - insert = InsertProcessor('work_queue', data={'workflow_id': workflow_id}) + insert = InsertProcessor('workflow_queue', data={'workflow_id': workflow_id}) insert.execute() @@ -242,7 +242,7 @@ def handle_error(job, err): # for now we mark it completed but include the error # TODO retries? # XXX what do we do about the workflow? - update = UpdateProcessor('work_queue', clauses=['id=%(id)s'], values=job) + update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=job) update.set(completed=True) update.set(error=str(err)) update.rawset(completion_time='NOW()') @@ -477,7 +477,7 @@ class BaseWorkflow: # TODO - the result field needs to be handled better self.log('Closing %(method)s workflow' % self.info) - for table in ('workflow_wait', 'workflow_slots', 'work_queue'): + for table in ('workflow_wait', 'workflow_slots', 'workflow_queue'): delete = DeleteProcessor(table, clauses=['workflow_id = %(id)s'], values=self.info) delete.execute() @@ -502,7 +502,7 @@ class BaseWorkflow: def requeue(self): self.log('Queuing %(method)s workflow' % self.info) - insert = InsertProcessor('work_queue', data={'workflow_id': self.info['id']}) + insert = InsertProcessor('workflow_queue', data={'workflow_id': self.info['id']}) insert.execute() def update(self): @@ -581,7 +581,7 @@ def add_workflow(method, params, queue=True): if queue: # also add it to the work queue so it will start - insert = InsertProcessor('work_queue', data={'workflow_id': data['id']}) + insert = InsertProcessor('workflow_queue', data={'workflow_id': data['id']}) insert.execute() # TODO return full info? diff --git a/schemas/schema.sql b/schemas/schema.sql index fcd9269..ee09060 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1090,7 +1090,7 @@ CREATE TABLE workflow_slots ( ) WITHOUT OIDS; -CREATE TABLE work_queue ( +CREATE TABLE workflow_queue ( id SERIAL NOT NULL PRIMARY KEY, workflow_id INTEGER REFERENCES workflow(id), create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), @@ -1106,7 +1106,7 @@ CREATE TABLE locks ( ) WITHOUT OIDS; INSERT INTO locks(name) VALUES('protonmsg-plugin'); INSERT INTO locks(name) VALUES('scheduler'); -INSERT INTO locks(name) VALUES('work_queue'); +INSERT INTO locks(name) VALUES('workflow_queue'); INSERT INTO locks(name) VALUES('workflow_slots'); COMMIT WORK; From 53ab84a8b9076805e9c503c7e22cf8b0b9d76610 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 03 2024 21:39:58 +0000 Subject: [PATCH 27/51] wait handling framework --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index def89a0..d50a307 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -75,26 +75,26 @@ def queue_next(): opts={'order': 'id', 'limit': 1}, lock='skip') # note the lock=skip with limit 1. This will give us a row lock on the first unlocked row - job = query.executeOne() - if not job: + row = query.executeOne() + if not row: # either empty queue or all locked return False - logger.debug('Handling work queue id %(id)s, workflow %(workflow_id)s', job) - handle_job(job) + logger.debug('Handling work queue id %(id)s, workflow %(workflow_id)s', row) + run_workflow(row['workflow_id']) # mark it done - update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=job) + update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=row) update.set(completed=True) update.rawset(completion_time='NOW()') update.execute() - logger.debug('Finished handling work queue id %(id)s', job) + logger.debug('Finished handling work queue id %(id)s', row) return True def update_queue(): - handle_waits() + check_waits() clean_queue() @@ -111,7 +111,7 @@ def clean_queue(): logger.info('Deleted %i old queue entries', count) -def handle_waits(): +def check_waits(): """Check our wait data and see if we need to update the queue Things we're checking for: @@ -122,8 +122,9 @@ def handle_waits(): logger.debug('Checking waits') query = WaitsQuery( clauses=[ - ['handled', 'IS', False], - ] + ['seen', 'IS', False], + ], + opts={'order': 'id'} ) # index by workflow @@ -132,48 +133,57 @@ def handle_waits(): wf_waits.setdefault(info['workflow_id'], []).append(info) fulfilled = [] - handled = [] + seen = [] requeue = [] for workflow_id in wf_waits: - waiting = [] - for info in wf_waits[workflow_id]: + waits = wf_waits[workflow_id] + # first pass: check fulfillment + for info in waits: if info['fulfilled']: - handled.append(info) + # fulfilled but not seen means fulfillment was noted elsewhere + # mark it seen so we don't keep checking it + seen.append(info) else: # TODO we should avoid calling wait.check quite so often cls = waits.get(info['wait_type']) wait = cls(info) if wait.check(): + info['fulfilled'] = True fulfilled.append(info) - else: - waiting.append(info) - if not waiting: + waiting = [] + nonbatch = [] + # second pass: decide whether to requeue + for info in waits: + if info['fulfilled']: + # batch waits won't trigger a requeue unless all other waits are fulfilled + if not info.get('batch'): + nonbatch.append(info) + else: + waiting.append(info) + if not waiting or nonbatch: requeue.append(workflow_id) - for info in fulfilled: + for info in fulfilled + seen: logger.info('Fulfilled %(wait_type)s wait %(id)s for workflow %(workflow_id)s', info) + if fulfilled: - # we can do these in single update update = UpdateProcessor( table='workflow_wait', clauses=['id IN %(ids)s'], values={'ids': [w['id'] for w in fulfilled]}, ) update.set(fulfilled=True) - update.set(handled=True) + update.rawset(fulfill_time='NOW()') + update.set(seen=True) update.execute() - # XXX we should not mark them handled until workflow actually runs - for info in handled: - logger.info('Handled %(wait_type)s wait %(id)s for workflow %(workflow_id)s', info) - if handled: - # we can do these in single update + if seen: update = UpdateProcessor( table='workflow_wait', clauses=['id IN %(ids)s'], - values={'ids': [w['id'] for w in handled]}, + values={'ids': [w['id'] for w in seen]}, ) - update.set(handled=True) + update.set(seen=True) update.execute() for workflow_id in requeue: @@ -208,34 +218,30 @@ class WorkflowQuery(QueryView): } -def handle_job(job): - # TODO row lock - query = WorkflowQuery(clauses=[['id', '=', job['workflow_id']]]).query +def run_workflow(workflow_id, opts=None, strict=False): + query = WorkflowQuery(clauses=[['id', '=', workflow_id]]).query query.lock = True # we must have a lock on the workflow before attempting to run it wf = query.executeOne(strict=True) if wf['completed']: + # shouldn't happen, closing the workflow should delete its queue entries logger.error('Ignoring completed %(method)s workflow in queue: %(id)i', wf) - logger.debug('Data: %r', wf) + logger.debug('Data: %r, Opts: %r', wf, opts) return - logger.debug('Handling workflow: %r', wf) + cls = workflows.get(wf['method']) handler = cls(wf) + try: - handler.run() + handler.run(opts) except Exception as err: - handle_error(job, err) + # handle_error(workflow_id, err) + # TODO sort out error handling raise # XXX def run_subtask_step(workflow_id, step): - query = WorkflowQuery(clauses=[['id', '=', workflow_id]]) - query.lock = True # we must have a lock on the workflow before attempting to run it - wf = query.executeOne(strict=True) - if wf['completed']: - raise koji.GenericError('Workflow is completed') - cls = workflows.get(wf['method']) - handler = cls(wf) - handler.run(subtask_step=step) + opts = {'from_subtask': True, 'step': step} + run_workflow(workflow_id, opts, strict=True) def handle_error(job, err): @@ -280,20 +286,24 @@ class BaseWorkflow: self.data = info['data'] self.waiting = False - def run(self, subtask_step=None): + def run(self, opts=None): if self.data is None: self.setup() + if opts is None: + opts = {} + + self.handle_waits() # TODO error handling step = self.data['steps'].pop(0) handler = self.get_handler(step) + if 'step' in opts and opts['step'] != step: + raise koji.GenericError(f'Step mismatch {opts["step"]} != {step}') is_subtask = getattr(handler, 'subtask', False) - if subtask_step is not None: + if opts.get('from_subtask'): # we've been called via a workflowStep task - if subtask_step != step: - raise koji.GenericError(f'Step mismatch {subtask_step} != {step}') - elif not is_subtask: + if not is_subtask: raise koji.GenericError(f'Not a subtask step: {step}') # otherwise we're good elif is_subtask: @@ -349,6 +359,23 @@ class BaseWorkflow: # update the db self.update() + def handle_waits(self): + query = WaitsQuery( + clauses=[['workflow_id', '=', self.info['id']], ['handled', 'IS', False]], + opts={'order': 'id'}) + mywaits = query.execute() + waiting = [] + fulfilled = [] + for info in mywaits: + if not info['fulfilled']: + # TODO should we call check here as well? + waiting.append(info) + else: + cls = waits.get(info['wait_type']) + wait = cls(info) + wait.handle(workflow=self) + return bool(waiting) + def log(self, msg, level=logging.INFO): log_both(msg, task_id=self.info['stub_id'], level=level) @@ -616,6 +643,7 @@ class WaitsQuery(QueryView): 'create_time': ['workflow_wait.create_time', None], 'create_ts': ["date_part('epoch', workflow_wait.create_time)", None], 'fulfilled': ['workflow_wait.fulfilled', None], + 'seen': ['workflow_wait.seen', None], 'handled': ['workflow_wait.handled', None], } @@ -629,19 +657,15 @@ class BaseWait: def check(self): raise NotImplementedError('wait check not defined') - # XXX does it make sense to update state here? - def set_fulfilled(self): - update = UpdateProcessor('workflow_wait', clauses=['id = %(id)s'], values=self.info) - update.set(fulfilled=True) - update.execute() - - # XXX does it make sense to update state here? def set_handled(self): # TODO what should we do if not fulfilled yet? update = UpdateProcessor('workflow_wait', clauses=['id = %(id)s'], values=self.info) update.set(handled=True) update.execute() + def handle(self): + self.set_handled() + @waits.add('task') class TaskWait(BaseWait): @@ -656,6 +680,28 @@ class TaskWait(BaseWait): state = query.singleValue() return (state in self.END_STATES) + def handle(self, workflow): + self.set_handled() + task = kojihub.Task(self.info['params']['task_id']) + tinfo = task.getInfo() + ret = {'task': tinfo} + if tinfo['state'] == koji.TASK_STATES['FAILED']: + if not self.info['params'].get('canfail', False): + raise koji.GenericError(f'Workflow task {tinfo["id"]} failed') + # TODO workflow failure + # otherwise we keep going + elif tinfo['state'] == koji.TASK_STATES['CANCELED']: + # TODO unclear if canfail applies here + raise koji.GenericError(f'Workflow task {tinfo["id"]} canceled') + elif tinfo['state'] == koji.TASK_STATES['CLOSED']: + # shouldn't be a fault + ret['result'] = task.getResult() + else: + # should not happen + raise koji.GenericError(f'Task not completed: {tinfo}') + # TODO: update workflow data? + return ret + @staticmethod def task_done(task_id): # TODO catch errors? diff --git a/schemas/schema.sql b/schemas/schema.sql index ee09060..7020dde 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1072,8 +1072,10 @@ CREATE TABLE workflow_wait ( wait_type TEXT, params JSONB, create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), + fulfill_time TIMESTAMPTZ, fulfilled BOOLEAN NOT NULL DEFAULT FALSE, -- wait condition fulfilled? - handled BOOLEAN NOT NULL DEFAULT FALSE -- workflow informed? + seen BOOLEAN NOT NULL DEFAULT FALSE, -- noted by scheduler? + handled BOOLEAN NOT NULL DEFAULT FALSE -- recieved by handler? -- more ??? ) WITHOUT OIDS; From 62d93f6da1690f4f3c16ad161a97d6eda98876ea Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 03:26:52 +0000 Subject: [PATCH 28/51] stab at workflow error handling --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index d50a307..784ac3b 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -8,7 +8,7 @@ from koji.context import context from . import kojihub from .scheduler import log_both from .db import QueryProcessor, InsertProcessor, UpsertProcessor, UpdateProcessor, \ - DeleteProcessor, QueryView, db_lock, nextval + DeleteProcessor, QueryView, db_lock, nextval, Savepoint logger = logging.getLogger('koji.workflow') @@ -68,7 +68,6 @@ def queue_next(): :returns: True if an entry ran, False otherwise """ - # TODO maybe use scheduler logging mechanism? query = QueryProcessor(tables=['workflow_queue'], columns=['id', 'workflow_id'], clauses=['completed IS FALSE'], @@ -81,13 +80,16 @@ def queue_next(): return False logger.debug('Handling work queue id %(id)s, workflow %(workflow_id)s', row) - run_workflow(row['workflow_id']) - # mark it done - update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=row) - update.set(completed=True) - update.rawset(completion_time='NOW()') - update.execute() + try: + run_workflow(row['workflow_id']) + + finally: + # mark it done, even if we errored + update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=row) + update.set(completed=True) + update.rawset(completion_time='NOW()') + update.execute() logger.debug('Finished handling work queue id %(id)s', row) return True @@ -136,9 +138,8 @@ def check_waits(): seen = [] requeue = [] for workflow_id in wf_waits: - waits = wf_waits[workflow_id] # first pass: check fulfillment - for info in waits: + for info in wf_waits[workflow_id]: if info['fulfilled']: # fulfilled but not seen means fulfillment was noted elsewhere # mark it seen so we don't keep checking it @@ -153,7 +154,7 @@ def check_waits(): waiting = [] nonbatch = [] # second pass: decide whether to requeue - for info in waits: + for info in wf_waits[workflow_id]: if info['fulfilled']: # batch waits won't trigger a requeue unless all other waits are fulfilled if not info.get('batch'): @@ -203,6 +204,7 @@ class WorkflowQuery(QueryView): 'stub_id': ['stub_id', None], 'started': ['workflow.started', None], 'completed': ['workflow.completed', None], + 'frozen': ['workflow.frozen', None], 'create_time': ['workflow.create_time', None], 'start_time': ['workflow.start_time', None], 'update_time': ['workflow.update_time', None], @@ -218,25 +220,46 @@ class WorkflowQuery(QueryView): } +class WorkflowFailure(Exception): + """Raised to explicitly fail a workflow""" + pass + + def run_workflow(workflow_id, opts=None, strict=False): query = WorkflowQuery(clauses=[['id', '=', workflow_id]]).query query.lock = True # we must have a lock on the workflow before attempting to run it wf = query.executeOne(strict=True) + if wf['completed']: # shouldn't happen, closing the workflow should delete its queue entries - logger.error('Ignoring completed %(method)s workflow in queue: %(id)i', wf) + logger.error('Ignoring completed %(method)s workflow: %(id)i', wf) logger.debug('Data: %r, Opts: %r', wf, opts) return + if wf['frozen']: + logger.warning('Skipping frozen %(method)s workflow: %(id)i', wf) + return cls = workflows.get(wf['method']) handler = cls(wf) + err = None + savepoint = Savepoint('pre_workflow') try: handler.run(opts) + + except WorkflowFailure as err: + # this is deliberate failure, so handle it that way + handler.fail(msg=str(err)) + except Exception as err: - # handle_error(workflow_id, err) - # TODO sort out error handling - raise # XXX + # for unplanned exceptions, we assume the worst + # rollback and freeze the workflow + savepoint.rollback() + handle_error(wf, err) + logger.exception('Error handling workflow') + + if strict and err is not None: + raise koji.GenericError(f'Error handling workflow: {str(err)}') def run_subtask_step(workflow_id, step): @@ -244,16 +267,31 @@ def run_subtask_step(workflow_id, step): run_workflow(workflow_id, opts, strict=True) -def handle_error(job, err): - # for now we mark it completed but include the error - # TODO retries? - # XXX what do we do about the workflow? - update = UpdateProcessor('workflow_queue', clauses=['id=%(id)s'], values=job) - update.set(completed=True) - update.set(error=str(err)) - update.rawset(completion_time='NOW()') +def handle_error(info, err): + # freeze the workflow + update = UpdateProcessor('workflow', clauses=['id=%(id)s'], values=info) + update.set(frozen=True) + update.rawset(update_time='NOW()') update.execute() + # record the error + error_data = { + 'error': str(err), # TODO traceback? + 'workflow_data': info['data'], + } + data = { + 'workflow_id': info['id'], + 'data': json.dumps(error_data), + } + insert = InsertProcessor('workflow_error', data=data) + insert.execute() + + # delist the workflow + for table in ('workflow_wait', 'workflow_slots', 'workflow_queue'): + delete = DeleteProcessor(table, clauses=['workflow_id = %(id)s'], + values=info) + delete.execute() + class SimpleRegistry: @@ -527,6 +565,14 @@ class BaseWorkflow: # TODO we need to do more here, but for now self.close(result='canceled', stub_state='CANCELED') + def fail(self, msg=None): + # TODO we need to do more here, but for now + if msg is not None: + msg = f'Workflow failed - {msg}' + else: + msg = 'Workflow failed' + self.close(result=msg, stub_state='FAILED') + def requeue(self): self.log('Queuing %(method)s workflow' % self.info) insert = InsertProcessor('workflow_queue', data={'workflow_id': self.info['id']}) diff --git a/schemas/schema.sql b/schemas/schema.sql index 7020dde..7c3049e 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1054,6 +1054,7 @@ CREATE TABLE workflow ( stub_id INTEGER UNIQUE NOT NULL REFERENCES task (id), started BOOLEAN NOT NULL DEFAULT FALSE, completed BOOLEAN NOT NULL DEFAULT FALSE, + frozen BOOLEAN NOT NULL DEFAULT FALSE, create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), start_time TIMESTAMPTZ, update_time TIMESTAMPTZ, @@ -1076,7 +1077,15 @@ CREATE TABLE workflow_wait ( fulfilled BOOLEAN NOT NULL DEFAULT FALSE, -- wait condition fulfilled? seen BOOLEAN NOT NULL DEFAULT FALSE, -- noted by scheduler? handled BOOLEAN NOT NULL DEFAULT FALSE -- recieved by handler? - -- more ??? +) WITHOUT OIDS; + + +CREATE TABLE workflow_error ( + id SERIAL NOT NULL PRIMARY KEY, + workflow_id INTEGER REFERENCES workflow(id), + data JSONB, + create_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), + handled BOOLEAN NOT NULL DEFAULT FALSE ) WITHOUT OIDS; From 91cfd3d86d4813a662bcba8af44b751ce3b41210 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 13:09:14 +0000 Subject: [PATCH 29/51] Merge branch 'master' into workflow --- diff --git a/builder/kojid b/builder/kojid index ace31a2..4a9727e 100755 --- a/builder/kojid +++ b/builder/kojid @@ -79,7 +79,8 @@ from koji.tasks import ( BaseTaskHandler, MultiPlatformTask, ServerExit, - ServerRestart + ServerRestart, + RefuseTask, ) from koji.util import ( dslice, @@ -5723,6 +5724,15 @@ class NewRepoTask(BaseTaskHandler): def handler(self, tag, event=None, src=False, debuginfo=False, separate_src=False): tinfo = self.session.getTag(tag, strict=True, event=event) + + # check for fs access before we try calling repoInit + top_repos_dir = joinpath(self.options.topdir, "repos") + if not os.path.isdir(top_repos_dir): + # missing or incorrect mount? + # refuse and let another host try + raise RefuseTask("No access to repos dir %s" % top_repos_dir) + + # call repoInit kwargs = {} if event is not None: kwargs['event'] = event @@ -5829,7 +5839,14 @@ class CreaterepoTask(BaseTaskHandler): toprepodir = self.pathinfo.repo(repo_id, rinfo['tag_name']) self.repodir = '%s/%s' % (toprepodir, arch) if not os.path.isdir(self.repodir): - raise koji.GenericError("Repo directory missing: %s" % self.repodir) + top_repos_dir = joinpath(self.options.topdir, "repos") + if not os.path.isdir(top_repos_dir): + # missing or incorrect mount? + # refuse and let another host try + raise RefuseTask("No access to repos dir %s" % top_repos_dir) + else: + # we seem to have fs access, but dir is missing, perhaps a repo_init bug? + raise koji.GenericError("Repo directory missing: %s" % self.repodir) groupdata = os.path.join(toprepodir, 'groups', 'comps.xml') # set up our output dir self.outdir = '%s/repo' % self.workdir diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 3981d25..e44b136 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -324,9 +324,8 @@ def handle_add_channel(goptions, session, args): if 'channel %s already exists' % channel_name in msg: error("channel %s already exists" % channel_name) elif 'Invalid method:' in msg: - version = session.getKojiVersion() error("addChannel is available on hub from Koji 1.26 version, your version is %s" % - version) + session.hub_version_str) else: error(msg) print("%s added: id %d" % (args[0], channel_id)) @@ -356,9 +355,8 @@ def handle_edit_channel(goptions, session, args): except koji.GenericError as ex: msg = str(ex) if 'Invalid method:' in msg: - version = session.getKojiVersion() error("editChannel is available on hub from Koji 1.26 version, your version is %s" % - version) + session.hub_version_str) else: warn(msg) if not result: @@ -6295,14 +6293,6 @@ def handle_cancel(goptions, session, args): if len(args) == 0: parser.error("You must specify at least one task id or build") activate_session(session, goptions) - older_hub = False - try: - hub_version = session.getKojiVersion() - v = tuple([int(x) for x in hub_version.split('.')]) - if v < (1, 33, 0): - older_hub = True - except koji.GenericError: - older_hub = True tlist = [] blist = [] for arg in args: @@ -6329,7 +6319,7 @@ def handle_cancel(goptions, session, args): for task_id in tlist: results.append(remote_fn(task_id, **opts)) for build in blist: - if not older_hub: + if session.hub_version >= (1, 33, 0): results.append(m.cancelBuild(build, strict=True)) else: results.append(m.cancelBuild(build)) @@ -7575,7 +7565,7 @@ def handle_moshimoshi(options, session, args): u = {'name': 'anonymous user'} print("%s, %s!" % (_printable_unicode(random.choice(greetings)), u["name"])) print("") - print("You are using the hub at %s" % session.baseurl) + print("You are using the hub at %s (Koji %s)" % (session.baseurl, session.hub_version_str)) authtype = u.get('authtype', getattr(session, 'authtype', None)) if authtype == koji.AUTHTYPES['NORMAL']: print("Authenticated via password") @@ -8130,3 +8120,34 @@ def handle_promote_build(goptions, session, args): error("Not a draft build: %s" % draft_build) rinfo = session.promoteBuild(binfo['id'], force=options.force) print("%s has been promoted to %s" % (binfo['nvr'], rinfo['nvr'])) + + +def anon_handle_list_users(goptions, session, args): + """[admin] List of users""" + usage = "usage: %prog list-users [options]" + parser = OptionParser(usage=get_usage_str(usage)) + parser.add_option("--usertype", help="List users that have a given usertype " + "(e.g. NORMAL, HOST, GROUP)") + parser.add_option("--prefix", help="List users that have a given prefix") + (options, args) = parser.parse_args(args) + + if len(args) > 0: + parser.error("This command takes no arguments") + activate_session(session, goptions) + + if options.usertype: + if options.usertype.upper() in koji.USERTYPES.keys(): + usertype = koji.USERTYPES[options.usertype.upper()] + else: + error("Usertype %s doesn't exist" % options.usertype) + else: + usertype = koji.USERTYPES['NORMAL'] + + if options.prefix: + prefix = options.prefix + else: + prefix = None + + users_list = session.listUsers(userType=usertype, prefix=prefix) + for user in users_list: + print(user['name']) diff --git a/koji/__init__.py b/koji/__init__.py index 6113c4b..de9b322 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2625,6 +2625,30 @@ class ClientSession(object): self.opts.setdefault('timeout', DEFAULT_REQUEST_TIMEOUT) self.exclusive = False self.auth_method = auth_method + self.__hub_version = None + + @property + def hub_version(self): + """Return the hub version as a tuple of ints""" + return tuple([int(x) for x in self.hub_version_str.split('.')]) + + @property + def hub_version_str(self): + """Return the hub version as string""" + # If any call was made before, it should be populated by Koji-Version header + # for hub >= 1.35 + if self.__hub_version is None: + # no call was made yet OR hub_version < 1.35 + try: + self.__hub_version = self.getKojiVersion() + except GenericError as e: + if 'Invalid method' in str(e): + # use latest version without the getKojiVersion handler + self.logger.debug("hub is older than 1.23, assuming 1.22.0") + self.__hub_version = '1.22.0' + else: + raise + return self.__hub_version @property def multicall(self): @@ -3056,6 +3080,9 @@ class ClientSession(object): warnings.simplefilter("ignore") r = self.rsession.post(handler, **callopts) r.raise_for_status() + hub_version = r.headers.get('Koji-Version') + if hub_version: + self.__hub_version = hub_version try: ret = self._read_xmlrpc_response(r) finally: diff --git a/koji/daemon.py b/koji/daemon.py index 78b5387..e50b229 100644 --- a/koji/daemon.py +++ b/koji/daemon.py @@ -1428,6 +1428,9 @@ class TaskManager(object): except (SystemExit, koji.tasks.ServerExit, KeyboardInterrupt): # we do not trap these raise + except koji.tasks.RefuseTask as refuse: + self.session.host.refuseTask(handler.id, msg=str(refuse)) + return except koji.tasks.ServerRestart: # freeing this task will allow the pending restart to take effect self.session.host.freeTasks([handler.id]) diff --git a/koji/tasks.py b/koji/tasks.py index 9eaa343..1a5341f 100644 --- a/koji/tasks.py +++ b/koji/tasks.py @@ -110,6 +110,11 @@ class ServerRestart(Exception): pass +class RefuseTask(Exception): + """Raise to task handler to refuse a task""" + pass + + def parse_task_params(method, params): """Parse task params into a dictionary diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 9d20114..7dbd0db 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -30,6 +30,7 @@ import calendar import copy import datetime import fcntl +import filecmp import fnmatch import functools import hashlib @@ -8016,63 +8017,92 @@ def delete_rpm_sig(rpminfo, sigkey=None, all_sigs=False): elif not sigkey: raise koji.GenericError("No signature specified") rinfo = get_rpm(rpminfo, strict=True) + nvra = "%(name)s-%(version)s-%(release)s.%(arch)s" % rinfo if rinfo['external_repo_id']: raise koji.GenericError("Not an internal rpm: %s (from %s)" % (rpminfo, rinfo['external_repo_name'])) + # Determine what signature we have rpm_query_result = query_rpm_sigs(rpm_id=rinfo['id'], sigkey=sigkey) if not rpm_query_result: - nvra = "%(name)s-%(version)s-%(release)s.%(arch)s" % rinfo raise koji.GenericError("%s has no matching signatures to delete" % nvra) + found_keys = [r['sigkey'] for r in rpm_query_result] - clauses = ["rpm_id=%(rpm_id)i"] - if sigkey is not None: - clauses.append("sigkey=%(sigkey)s") + # Delete signature entries from db + clauses = ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"] rpm_id = rinfo['id'] delete = DeleteProcessor(table='rpmsigs', clauses=clauses, values=locals()) delete.execute() delete = DeleteProcessor(table='rpm_checksum', clauses=clauses, values=locals()) delete.execute() - binfo = get_build(rinfo['build_id']) + + # Get the base build dir for our paths + binfo = get_build(rinfo['build_id'], strict=True) builddir = koji.pathinfo.build(binfo) - list_sigcaches = [] - list_sighdrs = [] + + # Check header files + hdr_renames = [] + hdr_deletes = [] for rpmsig in rpm_query_result: - list_sigcaches.append(joinpath(builddir, koji.pathinfo.sighdr(rinfo, rpmsig['sigkey']))) - list_sighdrs.append(joinpath(builddir, koji.pathinfo.signed(rinfo, rpmsig['sigkey']))) - list_paths = list_sighdrs + list_sigcaches - count = 0 - logged_user = get_user(context.session.user_id) - for file_path in list_paths: + hdr_path = joinpath(builddir, koji.pathinfo.sighdr(rinfo, rpmsig['sigkey'])) + backup_path = hdr_path + f".{rpmsig['sighash']}.save" + if not os.path.exists(hdr_path): + logger.error(f'Missing signature header file: {hdr_path}') + # this doesn't prevent us from deleting the signature + # it just means we have nothing to back up + continue + if not os.path.isfile(hdr_path): + # this should not happen and requires human intervention + raise koji.GenericError(f"Not a regular file: {hdr_path}") + if os.path.exists(backup_path): + # Likely residue of previous failed deletion + if filecmp.cmp(hdr_path, backup_path, shallow=False): + # same file contents, so we're already backed up + logger.warning(f"Signature header already backed up: {backup_path}") + hdr_deletes.append([rpmsig, hdr_path]) + else: + # this shouldn't happen + raise koji.GenericError(f"Stray header backup file: {backup_path}") + else: + hdr_renames.append([rpmsig, hdr_path, backup_path]) + + # Delete signed copies + # We do these first since they are the lowest risk + for rpmsig in rpm_query_result: + signed_path = joinpath(builddir, koji.pathinfo.signed(rinfo, rpmsig['sigkey'])) + if not os.path.exists(signed_path): + # signed copies might not exist + continue try: - os.remove(file_path) - count += 1 - except FileNotFoundError: - logger.warning("User %s has deleted file %s", logged_user['name'], file_path) + os.remove(signed_path) + logger.warning(f"Deleted signed copy {signed_path}") except Exception: - logger.error("An error happens when deleting %s, %s deleting are deleted, " - "%s deleting are skipped, the original request is %s rpm " - "and %s sigkey", file_path, - list_paths[:count], list_paths[count:], rpminfo, sigkey, exc_info=True) - raise koji.GenericError("File %s cannot be deleted." % file_path) - - for path in list_paths: - basedir = os.path.dirname(path) - if os.path.isdir(basedir) and not os.listdir(basedir): - try: - os.rmdir(basedir) - except OSError: - logger.warning("An error happens when deleting %s directory", - basedir, exc_info=True) - sigdir = os.path.dirname(basedir) - if os.path.isdir(sigdir) and not os.listdir(sigdir): - try: - os.rmdir(sigdir) - except OSError: - logger.warning("An error happens when deleting %s directory", - sigdir, exc_info=True) - logger.warning("Signed RPM %s with sigkey %s is deleted by %s", rinfo['id'], sigkey, - logged_user['name']) + logger.error(f"Failed to delete {signed_path}", exc_info=True) + raise koji.GenericError(f"Failed to delete {signed_path}") + + # Backup header files + for rpmsig, hdr_path, backup_path in hdr_renames: + # sanity checked above + try: + os.rename(hdr_path, backup_path) + logger.warning(f"Signature header saved to {backup_path}") + except Exception: + logger.error(f"Failed to rename {hdr_path} to {backup_path}", exc_info=True) + + # Delete already backed-up headers + for rpmsig, hdr_path in hdr_deletes: + # verified backup above + try: + os.remove(hdr_path) + logger.warning(f"Deleted signature header {hdr_path}") + except Exception: + logger.error(f"Failed to delete {hdr_path}", exc_info=True) + raise koji.GenericError(f"Failed to delete {hdr_path}") + + # Note: we do not delete any empty parent dirs as the primary use case for deleting these + # signatures is to allow the import of new, overlapping ones + + logger.warning("Deleted signatures %s for rpm %s", found_keys, rinfo['id']) def _scan_sighdr(sighdr, fn): @@ -8247,7 +8277,7 @@ def write_signed_rpm(an_rpm, sigkey, force=False): # make sure we have it in the db rpm_id = rinfo['id'] query = QueryProcessor(tables=['rpmsigs'], columns=['sighash'], - clauses=['rpm_id=%(rpm_id)i', 'sigkey=%(sigkey)s'], + clauses=['rpm_id=%(rpm_id)s', 'sigkey=%(sigkey)s'], values={'rpm_id': rpm_id, 'sigkey': sigkey}) sighash = query.singleValue(strict=False) if not sighash: diff --git a/kojihub/kojixmlrpc.py b/kojihub/kojixmlrpc.py index a36d920..8767cf0 100644 --- a/kojihub/kojixmlrpc.py +++ b/kojihub/kojixmlrpc.py @@ -45,6 +45,12 @@ from . import scheduler from . import workflow +# HTTP headers included in every request +GLOBAL_HEADERS = [ + ('Koji-Version', koji.__version__), +] + + class Marshaller(ExtendedMarshaller): dispatch = ExtendedMarshaller.dispatch.copy() @@ -390,7 +396,7 @@ def offline_reply(start_response, msg=None): else: faultString = msg response = dumps(Fault(faultCode, faultString)).encode() - headers = [ + headers = GLOBAL_HEADERS + [ ('Content-Length', str(len(response))), ('Content-Type', "text/xml"), ] @@ -400,7 +406,7 @@ def offline_reply(start_response, msg=None): def error_reply(start_response, status, response, extra_headers=None): response = response.encode() - headers = [ + headers = GLOBAL_HEADERS + [ ('Content-Length', str(len(response))), ('Content-Type', "text/plain"), ] @@ -817,7 +823,7 @@ def application(environ, start_response): except RequestTimeout as e: return error_reply(start_response, '408 Request Timeout', str(e) + '\n') response = response.encode() - headers = [ + headers = GLOBAL_HEADERS + [ ('Content-Length', str(len(response))), ('Content-Type', "text/xml"), ] diff --git a/kojihub/scheduler.py b/kojihub/scheduler.py index b44dab3..6be9975 100644 --- a/kojihub/scheduler.py +++ b/kojihub/scheduler.py @@ -99,7 +99,7 @@ def set_refusal(hostID, taskID, soft=True, by_host=False, msg=''): } upsert = UpsertProcessor('scheduler_task_refusals', data=data, keys=('task_id', 'host_id')) upsert.execute() - log_both('Host refused task', task_id=taskID, host_id=hostID) + log_both(f'Host refused task: {msg}', task_id=taskID, host_id=hostID) class TaskRefusalsQuery(QueryView): diff --git a/tests/test_cli/data/list-commands-admin.txt b/tests/test_cli/data/list-commands-admin.txt index 6e4422a..498bd68 100644 --- a/tests/test_cli/data/list-commands-admin.txt +++ b/tests/test_cli/data/list-commands-admin.txt @@ -42,6 +42,7 @@ admin commands: import-cg Import external builds with rich metadata import-sig Import signatures into the database and write signed RPMs list-signed List signed copies of rpms + list-users List of users lock-tag Lock a tag make-task Create an arbitrary task prune-signed-copies Prune signed copies diff --git a/tests/test_cli/data/list-commands.txt b/tests/test_cli/data/list-commands.txt index e0799eb..bcd720d 100644 --- a/tests/test_cli/data/list-commands.txt +++ b/tests/test_cli/data/list-commands.txt @@ -42,6 +42,7 @@ admin commands: import-cg Import external builds with rich metadata import-sig Import signatures into the database and write signed RPMs list-signed List signed copies of rpms + list-users List of users lock-tag Lock a tag make-task Create an arbitrary task prune-signed-copies Prune signed copies diff --git a/tests/test_cli/test_add_channel.py b/tests/test_cli/test_add_channel.py index dc9d991..99f1ea1 100644 --- a/tests/test_cli/test_add_channel.py +++ b/tests/test_cli/test_add_channel.py @@ -60,7 +60,7 @@ class TestAddChannel(utils.CliTestCase): expected_api = 'Invalid method: addChannel' expected = 'addChannel is available on hub from Koji 1.26 version, your version ' \ 'is 1.25.1\n' - self.session.getKojiVersion.return_value = '1.25.1' + self.session.hub_version_str = '1.25.1' self.session.addChannel.side_effect = koji.GenericError(expected_api) arguments = ['--description', self.description, self.channel_name] diff --git a/tests/test_cli/test_cancel.py b/tests/test_cli/test_cancel.py index 16ec5d3..7a8a1c6 100644 --- a/tests/test_cli/test_cancel.py +++ b/tests/test_cli/test_cancel.py @@ -32,7 +32,8 @@ class TestCancel(utils.CliTestCase): %s: error: {message} """ % (self.progname, self.progname) - self.session.getKojiVersion.return_value = '1.33.0' + self.session.hub_version = (1, 33, 0) + self.session.hub_version_str = '1.33.0' def test_anon_cancel(self): args = ['123'] @@ -154,7 +155,7 @@ No such build: '%s' self.session.cancelBuild.assert_called_once_with(args[1], strict=True) def test_non_exist_build_and_task_older_hub(self): - self.session.getKojiVersion.return_value = '1.32.0' + self.session.hub_version = (1, 32, 0) args = ['11111', 'nvr-1-30.1'] expected_warn = """No such task: %s """ % (args[0]) diff --git a/tests/test_cli/test_edit_channel.py b/tests/test_cli/test_edit_channel.py index cbd3fb1..f4c5f05 100644 --- a/tests/test_cli/test_edit_channel.py +++ b/tests/test_cli/test_edit_channel.py @@ -78,7 +78,7 @@ Options: expected_api = 'Invalid method: editChannel' expected = 'editChannel is available on hub from Koji 1.26 version, your version ' \ 'is 1.25.1\n' - self.session.getKojiVersion.return_value = '1.25.1' + self.session.hub_version_str = '1.25.1' self.session.editChannel.side_effect = koji.GenericError(expected_api) self.assert_system_exit( @@ -94,7 +94,6 @@ Options: self.session.editChannel.assert_called_once_with(self.channel_old, name=self.channel_new, description=self.description) self.session.getChannel.assert_called_once_with(self.channel_old) - self.session.getKojiVersion.assert_called_once_with() def test_handle_edit_channel_non_exist_channel(self): expected = 'No such channel: %s\n' % self.channel_old diff --git a/tests/test_cli/test_hello.py b/tests/test_cli/test_hello.py index 4571289..6e35638 100644 --- a/tests/test_cli/test_hello.py +++ b/tests/test_cli/test_hello.py @@ -50,6 +50,8 @@ class TestHello(utils.CliTestCase): # Mock out the xmlrpc server session.getLoggedInUser.return_value = None session.krb_principal = user['krb_principal'] + mock_hub_version = '1.35.0' + session.hub_version_str = mock_hub_version print_unicode_mock.return_value = "Hello" self.assert_system_exit( @@ -63,7 +65,7 @@ class TestHello(utils.CliTestCase): # annonymous user message = "Not authenticated\n" + "Hello, anonymous user!" - hubinfo = "You are using the hub at %s" % self.huburl + hubinfo = "You are using the hub at %s (Koji %s)" % (self.huburl, mock_hub_version) handle_moshimoshi(self.options, session, []) self.assert_console_message(stdout, "{0}\n\n{1}\n".format(message, hubinfo)) self.activate_session_mock.assert_called_once_with(session, self.options) @@ -79,7 +81,7 @@ class TestHello(utils.CliTestCase): user['krb_principal'], koji.AUTHTYPES['SSL']: 'Authenticated via client certificate %s' % cert } - hubinfo = "You are using the hub at %s" % self.huburl + # same hubinfo session.getLoggedInUser.return_value = user message = "Hello, %s!" % self.progname self.options.cert = cert diff --git a/tests/test_cli/test_list_users.py b/tests/test_cli/test_list_users.py new file mode 100644 index 0000000..174b87a --- /dev/null +++ b/tests/test_cli/test_list_users.py @@ -0,0 +1,133 @@ +from __future__ import absolute_import + +import mock +from six.moves import StringIO + +import koji +from koji_cli.commands import anon_handle_list_users +from . import utils + + +class TestListUsers(utils.CliTestCase): + def setUp(self): + self.maxDiff = None + self.options = mock.MagicMock() + self.options.debug = False + self.session = mock.MagicMock() + self.session.getAPIVersion.return_value = koji.API_VERSION + self.activate_session = mock.patch('koji_cli.commands.activate_session').start() + self.error_format = """Usage: %s list-users [options] +(Specify the --help global option for a list of other help options) + +%s: error: {message} +""" % (self.progname, self.progname) + + def tearDown(self): + mock.patch.stopall() + + @mock.patch('sys.stdout', new_callable=StringIO) + def test_list_users_default_valid(self, stdout): + arguments = [] + self.session.listUsers.return_value = [{ + 'id': 1, 'krb_principals': [], + 'name': 'kojiadmin', + 'status': 0, + 'usertype': 0}, + {'id': 2, + 'krb_principals': [], + 'name': 'testuser', + 'status': 0, + 'usertype': 0}, + ] + rv = anon_handle_list_users(self.options, self.session, arguments) + actual = stdout.getvalue() + expected = """kojiadmin +testuser +""" + self.assertMultiLineEqual(actual, expected) + self.assertEqual(rv, None) + self.session.listUsers.assert_called_once_with( + userType=koji.USERTYPES['NORMAL'], prefix=None) + + @mock.patch('sys.stdout', new_callable=StringIO) + def test_list_users_with_prefix(self, stdout): + arguments = ['--prefix', 'koji'] + self.session.listUsers.return_value = [{ + 'id': 1, 'krb_principals': [], + 'name': 'kojiadmin', + 'status': 0, + 'usertype': 0}, + ] + rv = anon_handle_list_users(self.options, self.session, arguments) + actual = stdout.getvalue() + expected = """kojiadmin +""" + self.assertMultiLineEqual(actual, expected) + self.assertEqual(rv, None) + self.session.listUsers.assert_called_once_with( + userType=koji.USERTYPES['NORMAL'], prefix='koji') + + @mock.patch('sys.stdout', new_callable=StringIO) + def test_list_users_with_usertype(self, stdout): + arguments = ['--usertype', 'host'] + self.session.listUsers.return_value = [{ + 'id': 3, 'krb_principals': [], + 'name': 'kojihost', + 'status': 0, + 'usertype': 1}, + {'id': 5, 'krb_principals': [], + 'name': 'testhost', + 'status': 0, + 'usertype': 1}, + ] + rv = anon_handle_list_users(self.options, self.session, arguments) + actual = stdout.getvalue() + expected = """kojihost +testhost +""" + self.assertMultiLineEqual(actual, expected) + self.assertEqual(rv, None) + self.session.listUsers.assert_called_once_with( + userType=koji.USERTYPES['HOST'], prefix=None) + + def test_list_users_with_usertype_non_existing(self): + arguments = ['--usertype', 'test'] + self.assert_system_exit( + anon_handle_list_users, + self.options, self.session, arguments, + stdout='', + stderr="Usertype test doesn't exist\n", + activate_session=None, + exit_code=1) + self.session.listUsers.assert_not_called() + + @mock.patch('sys.stdout', new_callable=StringIO) + def test_list_users_with_usertype_and_prefix(self, stdout): + arguments = ['--usertype', 'host', '--prefix', 'test'] + self.session.listUsers.return_value = [{ + 'id': 5, 'krb_principals': [], + 'name': 'testhost', + 'status': 0, + 'usertype': 1}, + ] + rv = anon_handle_list_users(self.options, self.session, arguments) + actual = stdout.getvalue() + expected = """testhost +""" + self.assertMultiLineEqual(actual, expected) + self.assertEqual(rv, None) + self.session.listUsers.assert_called_once_with( + userType=koji.USERTYPES['HOST'], prefix='test') + + def test_anon_handle_list_users_help(self): + self.assert_help( + anon_handle_list_users, + """Usage: %s list-users [options] +(Specify the --help global option for a list of other help options) + +Options: + -h, --help show this help message and exit + --usertype=USERTYPE List users that have a given usertype (e.g. NORMAL, + HOST, GROUP) + --prefix=PREFIX List users that have a given prefix +""" % self.progname) diff --git a/tests/test_hub/test_delete_rpm_sig.py b/tests/test_hub/test_delete_rpm_sig.py index 5840f56..907c033 100644 --- a/tests/test_hub/test_delete_rpm_sig.py +++ b/tests/test_hub/test_delete_rpm_sig.py @@ -1,9 +1,13 @@ +import os +import tempfile +import shutil import unittest import mock import koji import kojihub +from koji.util import joinpath DP = kojihub.DeleteProcessor @@ -17,6 +21,9 @@ class TestDeleteRPMSig(unittest.TestCase): return delete def setUp(self): + self.tempdir = tempfile.mkdtemp() + self.pathinfo = koji.PathInfo(self.tempdir) + mock.patch('koji.pathinfo', new=self.pathinfo).start() self.DeleteProcessor = mock.patch('kojihub.kojihub.DeleteProcessor', side_effect=self.getDelete).start() self.deletes = [] @@ -58,9 +65,31 @@ class TestDeleteRPMSig(unittest.TestCase): 'sigkey': '2f86d6a1'}] self.userinfo = {'authtype': 2, 'id': 1, 'krb_principal': None, 'krb_principals': [], 'name': 'testuser', 'status': 0, 'usertype': 0} + self.set_up_files() + + def set_up_files(self): + builddir = self.pathinfo.build(self.buildinfo) + os.makedirs(builddir) + self.builddir = builddir + self.signed = {} + self.sighdr = {} + for sig in self.queryrpmsigs: + key = sig['sigkey'] + signed = joinpath(builddir, self.pathinfo.signed(self.rinfo, key)) + self.signed[key] = signed + koji.ensuredir(os.path.dirname(signed)) + with open(signed, 'wt') as fo: + fo.write('SIGNED COPY\n') + + sighdr = joinpath(builddir, self.pathinfo.sighdr(self.rinfo, key)) + self.sighdr[key] = sighdr + koji.ensuredir(os.path.dirname(sighdr)) + with open(sighdr, 'wt') as fo: + fo.write('DETACHED SIGHDR\n') def tearDown(self): mock.patch.stopall() + shutil.rmtree(self.tempdir) def test_rpm_not_existing(self): rpm_id = 1234 @@ -84,7 +113,8 @@ class TestDeleteRPMSig(unittest.TestCase): def test_external_repo(self): rpminfo = 1234 - rinfo = {'external_repo_id': 1, 'external_repo_name': 'INTERNAL'} + rinfo = self.rinfo.copy() + rinfo.update({'external_repo_id': 1, 'external_repo_name': 'INTERNAL'}) self.get_rpm.return_value = rinfo with self.assertRaises(koji.GenericError) as ex: kojihub.delete_rpm_sig(rpminfo, all_sigs=True) @@ -108,39 +138,134 @@ class TestDeleteRPMSig(unittest.TestCase): self.get_rpm.assert_called_once_with(rpminfo, strict=True) self.query_rpm_sigs.assert_called_once_with(rpm_id=self.rinfo['id'], sigkey=None) - @mock.patch('koji.pathinfo.build', return_value='fakebuildpath') - @mock.patch('os.remove') - def test_file_not_found_error(self, os_remove, pb): - rpminfo = 2 - os_remove.side_effect = FileNotFoundError() + def test_file_not_found_error(self): + rpminfo = self.rinfo['id'] self.get_rpm.return_value = self.rinfo self.get_build.return_value = self.buildinfo self.get_user.return_value = self.userinfo self.query_rpm_sigs.return_value = self.queryrpmsigs + + # a missing signed copy or header should not error + builddir = self.pathinfo.build(self.buildinfo) + sigkey = '2f86d6a1' + os.remove(self.signed[sigkey]) + os.remove(self.sighdr[sigkey]) r = kojihub.delete_rpm_sig(rpminfo, sigkey='testkey') self.assertEqual(r, None) + # the files should still be gone + for sigkey in self.signed: + if os.path.exists(self.signed[sigkey]): + raise Exception('signed copy not deleted') + for sigkey in self.sighdr: + if os.path.exists(self.sighdr[sigkey]): + raise Exception('header still in place') + self.assertEqual(len(self.deletes), 2) delete = self.deletes[0] self.assertEqual(delete.table, 'rpmsigs') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i", "sigkey=%(sigkey)s"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) delete = self.deletes[1] self.assertEqual(delete.table, 'rpm_checksum') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i", "sigkey=%(sigkey)s"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) self.get_rpm.assert_called_once_with(rpminfo, strict=True) self.query_rpm_sigs.assert_called_once_with(rpm_id=self.rinfo['id'], sigkey='testkey') - self.get_build.assert_called_once_with(self.rinfo['build_id']) + self.get_build.assert_called_once_with(self.rinfo['build_id'], strict=True) + + def test_header_not_a_file(self): + rpminfo = self.rinfo['id'] + self.get_rpm.return_value = self.rinfo + self.get_build.return_value = self.buildinfo + self.get_user.return_value = self.userinfo + self.query_rpm_sigs.return_value = self.queryrpmsigs + + # we should error, without making any changes, if a header is not a regular file + builddir = self.pathinfo.build(self.buildinfo) + bad_sigkey = '2f86d6a1' + bad_hdr= self.sighdr[bad_sigkey] + os.remove(bad_hdr) + os.mkdir(bad_hdr) + with self.assertRaises(koji.GenericError) as ex: + r = kojihub.delete_rpm_sig(rpminfo, sigkey='testkey') + expected_msg = "Not a regular file: %s" % bad_hdr + self.assertEqual(ex.exception.args[0], expected_msg) + + # the files should still be there + for sigkey in self.signed: + if not os.path.exists(self.signed[sigkey]): + raise Exception('signed copy was deleted') + for sigkey in self.sighdr: + if not os.path.exists(self.sighdr[sigkey]): + raise Exception('header was deleted') + if not os.path.isdir(bad_hdr): + # the function should not have touched the invalid path + raise Exception('bad header file was removed') + + def test_stray_backup(self): + rpminfo = self.rinfo['id'] + self.get_rpm.return_value = self.rinfo + self.get_build.return_value = self.buildinfo + self.get_user.return_value = self.userinfo + self.query_rpm_sigs.return_value = self.queryrpmsigs + + siginfo = self.queryrpmsigs[0] + sigkey = siginfo['sigkey'] + backup = "%s.%s.save" % (self.sighdr[sigkey], siginfo['sighash']) + with open(backup, 'wt') as fo: + fo.write('STRAY FILE\n') + # different contents + with self.assertRaises(koji.GenericError) as ex: + r = kojihub.delete_rpm_sig(rpminfo, sigkey='testkey') + expected_msg = "Stray header backup file: %s" % backup + self.assertEqual(ex.exception.args[0], expected_msg) + # files should not have been removed + for sigkey in self.signed: + if not os.path.exists(self.signed[sigkey]): + raise Exception('signed copy was deleted incorrectly') + for sigkey in self.sighdr: + if not os.path.exists(self.sighdr[sigkey]): + raise Exception('header was deleted incorrectly') + + def test_dup_backup(self): + rpminfo = self.rinfo['id'] + self.get_rpm.return_value = self.rinfo + self.get_build.return_value = self.buildinfo + self.get_user.return_value = self.userinfo + self.query_rpm_sigs.return_value = self.queryrpmsigs + + siginfo = self.queryrpmsigs[0] + sigkey = siginfo['sigkey'] + backup = "%s.%s.save" % (self.sighdr[sigkey], siginfo['sighash']) + with open(backup, 'wt') as fo: + fo.write('DETACHED SIGHDR\n') + # SAME contents + + r = kojihub.delete_rpm_sig(rpminfo, sigkey='testkey') + + # the files should be gone + for sigkey in self.signed: + if os.path.exists(self.signed[sigkey]): + raise Exception('signed copy not deleted') + for sigkey in self.sighdr: + if os.path.exists(self.sighdr[sigkey]): + raise Exception('header still in place') + + # the sighdrs should be saved + for siginfo in self.queryrpmsigs: + sigkey = siginfo['sigkey'] + backup = "%s.%s.save" % (self.sighdr[sigkey], siginfo['sighash']) + with open(backup, 'rt') as fo: + self.assertEqual(fo.read(), 'DETACHED SIGHDR\n') - @mock.patch('koji.pathinfo.build', return_value='fakebuildpath') @mock.patch('os.remove', side_effect=OSError) - def test_not_valid(self, os_remove, pb): + def test_not_valid(self, os_remove): rpminfo = 2 - filepath = 'fakebuildpath/data/signed/x86_64/fs_mark-3.3-20.el8.x86_64.rpm' + filepath = '%s/packages/fs_mark/3.3/20.el8/data/signed/x86_64/fs_mark-3.3-20.el8.x86_64.rpm' % self.tempdir self.get_rpm.return_value = self.rinfo self.get_build.return_value = self.buildinfo self.query_rpm_sigs.return_value = self.queryrpmsigs - expected_msg = "File %s cannot be deleted." % filepath + expected_msg = "Failed to delete %s" % filepath with self.assertRaises(koji.GenericError) as ex: kojihub.delete_rpm_sig(rpminfo, all_sigs=True) self.assertEqual(ex.exception.args[0], expected_msg) @@ -148,18 +273,16 @@ class TestDeleteRPMSig(unittest.TestCase): self.assertEqual(len(self.deletes), 2) delete = self.deletes[0] self.assertEqual(delete.table, 'rpmsigs') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) delete = self.deletes[1] self.assertEqual(delete.table, 'rpm_checksum') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) self.get_rpm.assert_called_once_with(rpminfo, strict=True) self.query_rpm_sigs.assert_called_once_with(rpm_id=self.rinfo['id'], sigkey=None) - self.get_build.assert_called_once_with(self.rinfo['build_id']) + self.get_build.assert_called_once_with(self.rinfo['build_id'], strict=True) - @mock.patch('koji.pathinfo.build', return_value='fakebuildpath') - @mock.patch('os.remove') - def test_valid(self, os_remove, pb): + def test_valid(self): rpminfo = 2 self.get_rpm.return_value = self.rinfo self.get_build.return_value = self.buildinfo @@ -167,14 +290,29 @@ class TestDeleteRPMSig(unittest.TestCase): self.query_rpm_sigs.return_value = self.queryrpmsigs kojihub.delete_rpm_sig(rpminfo, all_sigs=True) + # the files should be gone + for sigkey in self.signed: + if os.path.exists(self.signed[sigkey]): + raise Exception('signed copy not deleted') + for sigkey in self.sighdr: + if os.path.exists(self.sighdr[sigkey]): + raise Exception('header still in place') + + # the sighdrs should be saved + for siginfo in self.queryrpmsigs: + sigkey = siginfo['sigkey'] + backup = "%s.%s.save" % (self.sighdr[sigkey], siginfo['sighash']) + with open(backup, 'rt') as fo: + self.assertEqual(fo.read(), 'DETACHED SIGHDR\n') + self.assertEqual(len(self.deletes), 2) delete = self.deletes[0] self.assertEqual(delete.table, 'rpmsigs') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) delete = self.deletes[1] self.assertEqual(delete.table, 'rpm_checksum') - self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)i"]) + self.assertEqual(delete.clauses, ["rpm_id=%(rpm_id)s", "sigkey IN %(found_keys)s"]) self.get_rpm.assert_called_once_with(rpminfo, strict=True) self.query_rpm_sigs.assert_called_once_with(rpm_id=self.rinfo['id'], sigkey=None) - self.get_build.assert_called_once_with(self.rinfo['build_id']) + self.get_build.assert_called_once_with(self.rinfo['build_id'], strict=True) diff --git a/tests/test_lib/test_client_session.py b/tests/test_lib/test_client_session.py index 3f42341..fb3de76 100644 --- a/tests/test_lib/test_client_session.py +++ b/tests/test_lib/test_client_session.py @@ -30,6 +30,36 @@ class TestClientSession(unittest.TestCase): my_rsession.close.assert_called() self.assertNotEqual(ksession.rsession, my_rsession) + @mock.patch('requests.Session') + def test_hub_version_old(self, rsession): + ksession = koji.ClientSession('http://koji.example.com/kojihub') + ksession.getKojiVersion = mock.MagicMock() + ksession.getKojiVersion.side_effect = koji.GenericError('Invalid method: getKojiVersion') + self.assertEqual(ksession.hub_version, (1, 22, 0)) + ksession.getKojiVersion.assert_called_once() + + @mock.patch('requests.Session') + def test_hub_version_interim(self, rsession): + ksession = koji.ClientSession('http://koji.example.com/kojihub') + ksession.getKojiVersion = mock.MagicMock() + ksession.getKojiVersion.return_value = '1.23.1' + self.assertEqual(ksession.hub_version, (1, 23, 1)) + ksession.getKojiVersion.assert_called_once() + + def test_hub_version_str_interim(self): + ksession = koji.ClientSession('http://koji.example.com/kojihub') + ksession.getKojiVersion = mock.MagicMock() + ksession.getKojiVersion.return_value = '1.23.1' + self.assertEqual(ksession.hub_version_str, '1.23.1') + + def test_hub_version_new(self): + ksession = koji.ClientSession('http://koji.example.com/kojihub') + ksession.getKojiVersion = mock.MagicMock() + # would be filled by random call + ksession._ClientSession__hub_version = '1.35.0' + self.assertEqual(ksession.hub_version, (1, 35, 0)) + ksession.getKojiVersion.assert_not_called() + class TestFastUpload(unittest.TestCase): diff --git a/www/kojiweb/clusterhealth.chtml b/www/kojiweb/clusterhealth.chtml index be4862d..f1b20fc 100644 --- a/www/kojiweb/clusterhealth.chtml +++ b/www/kojiweb/clusterhealth.chtml @@ -57,12 +57,10 @@ Builder readiness #for $channel in $channels + #if $channel['enabled_channel'] $util.escapeHTML($channel['name']) - #if not $channel['enabled_channel'] - [disabled] - #end if #if $channel['capacityPerc'] @@ -79,6 +77,7 @@ + #end if #end for From 2503f96135d9b35d775e67167276353b68e1fbaa Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 13:34:43 +0000 Subject: [PATCH 30/51] prepRepo stub --- diff --git a/builder/kojid b/builder/kojid index 4a9727e..4cc9f5a 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5639,8 +5639,8 @@ Build Info: %(weburl)s/buildinfo?buildID=%(build_id)i\r class NewRepoTask(BaseTaskHandler): - Methods = ['newRepo'] - _taskWeight = 0.1 + Methods = ['prepRepo'] + _taskWeight = 0.5 def copy_arch_repo(self, src_repo_id, src_repo_path, repo_id, arch): """Copy repodata, return False if it fails""" @@ -5722,29 +5722,18 @@ class NewRepoTask(BaseTaskHandler): self.logger.debug('Arch repo test passed %s' % arch) return True - def handler(self, tag, event=None, src=False, debuginfo=False, separate_src=False): - tinfo = self.session.getTag(tag, strict=True, event=event) - - # check for fs access before we try calling repoInit + def handler(self, tag, repo, opts): + # check for fs access before we go any further top_repos_dir = joinpath(self.options.topdir, "repos") if not os.path.isdir(top_repos_dir): # missing or incorrect mount? # refuse and let another host try raise RefuseTask("No access to repos dir %s" % top_repos_dir) - # call repoInit - kwargs = {} - if event is not None: - kwargs['event'] = event - if src: - kwargs['with_src'] = True - if separate_src: - kwargs['with_separate_src'] = True - # generate debuginfo repo if requested or if specified in sidetag's extra - if debuginfo or tinfo['extra'].get('with_debuginfo'): - kwargs['with_debuginfo'] = True - - repo_id, event_id = self.session.host.repoInit(tinfo['id'], task_id=self.id, **kwargs) + # workflow has already called repo_init + tinfo = tag + repo_id = repo['id'] + event_id = repo['event_id'] path = koji.pathinfo.repo(repo_id, tinfo['name']) if not os.path.isdir(path): @@ -5784,13 +5773,13 @@ class NewRepoTask(BaseTaskHandler): newrepo = {'tag_id': tinfo['id'], 'create_event': event_id} if self.options.copy_old_repodata: possibly_clonable = self.check_repo(oldrepo_path, newrepo_path, - oldrepo, newrepo, kwargs) + oldrepo, newrepo, opts) else: possibly_clonable = False - subtasks = {} data = {} cloned_archs = [] + needed_archs = [] for arch in arches: if possibly_clonable and self.check_arch_repo(oldrepo_path, newrepo_path, arch): result = self.copy_arch_repo(oldrepo['id'], oldrepo_path, repo_id, arch) @@ -5798,6 +5787,13 @@ class NewRepoTask(BaseTaskHandler): data[arch] = result cloned_archs.append(arch) continue + # otherwise we need a createrepo task for this arch + needed_arches.append(arch) + + return {'cloned': data, 'needed': needed_arches} + + subtasks = {} + for arch in needed_archs: # if we can't copy old repo directly, trigger normal createrepo arglist = [repo_id, arch, oldrepo] subtasks[arch] = self.session.host.subtask(method='createrepo', diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 784ac3b..b13dd99 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -919,23 +919,25 @@ def finish(): @workflows.add('new-repo') class NewRepoWorkflow(BaseWorkflow): - STEPS = ['init', 'repos', 'finalize'] + STEPS = ['repo_init', 'repos', 'repo_done'] PARAMS = { 'tag': (int, str, dict), - 'event': (int,), + 'event': (int, type(None)), 'opts': (dict,), } - @slot('repo-init') - def init(self, tag, event=None, opts=None): + @subtask() + def repo_init(self, tag, event=None, opts=None): tinfo = kojihub.get_tag(tag, strict=True, event=event) - kw = self.params - # ??? should we call repo_init ourselves? - self.data['task_id'] = self.task('initRepo', kw) - # TODO mechanism for task_id value to persist to next step + opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src'), strict=True) + # TODO further opts validation? + repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) + repo_info = kojihub.repo_info(repo_id) + kw = {'tag': tinfo, 'repo': repo_info, 'opts': opts} + self.data['task_id'] = self.task('prepRepo', kw) def repos(self): - # TODO fetch archlist from task + repo_tasks = [] for arch in self.needed_arches: params = {'repo_id': repo_id, 'arch': arch, 'oldrepo': oldrepo} From bc3a2820e5a097b77ce4d2e83fe09a2624fb5c6d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:12:00 +0000 Subject: [PATCH 31/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index b13dd99..f5f5713 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -934,18 +934,35 @@ class NewRepoWorkflow(BaseWorkflow): repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) repo_info = kojihub.repo_info(repo_id) kw = {'tag': tinfo, 'repo': repo_info, 'opts': opts} - self.data['task_id'] = self.task('prepRepo', kw) + self.data['prep_id'] = self.task('prepRepo', kw) + self.data['repo'] = repo_info - def repos(self): - + def repos(self, prep_id): + # TODO better mechanism for fetching task result + prepdata = kojihub.Task(prep_id).getResult() repo_tasks = [] - for arch in self.needed_arches: + for arch in prepdata['needed']: params = {'repo_id': repo_id, 'arch': arch, 'oldrepo': oldrepo} repo_tasks[arch] = self.task('createrepo', params) + # TODO fail workflow on any failed subtask + self.data['cloned'] = prepdata['cloned'] + self.data['repo_tasks'] = repo_tasks - def finalize(self): - # TODO fetch params from self/tasks - repo_done(...) + @subtask() + def repo_done(self, event, cloned, repo_tasks): + data = cloned.copy() + for arch in repo_tasks: + data[arch] = kojihub.Task(repo_tasks[arch]).getResult() + + kwargs = {} + if event is not None: + kwargs['expire'] = True + if cloned: + kwargs['repo_json_updates'] = { + 'cloned_from_repo_id': 0, # XXX + 'cloned_archs': list(sorted(cloned)), + } + kojihub.repo_done(repo_id, data, **kwargs) class WorkflowExports: From 651a640ef01548edaa21bf220ff0b16abcd85784 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:29:36 +0000 Subject: [PATCH 32/51] fix variable scope --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index f5f5713..3af58bb 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -242,24 +242,26 @@ def run_workflow(workflow_id, opts=None, strict=False): cls = workflows.get(wf['method']) handler = cls(wf) - err = None + error = None savepoint = Savepoint('pre_workflow') try: handler.run(opts) except WorkflowFailure as err: # this is deliberate failure, so handle it that way - handler.fail(msg=str(err)) + error = str(err) + handler.fail(msg=error) except Exception as err: # for unplanned exceptions, we assume the worst # rollback and freeze the workflow savepoint.rollback() - handle_error(wf, err) + error = str(err) + handle_error(wf, error) logger.exception('Error handling workflow') - if strict and err is not None: - raise koji.GenericError(f'Error handling workflow: {str(err)}') + if strict and error is not None: + raise koji.GenericError(f'Error handling workflow: {error}') def run_subtask_step(workflow_id, step): @@ -267,7 +269,7 @@ def run_subtask_step(workflow_id, step): run_workflow(workflow_id, opts, strict=True) -def handle_error(info, err): +def handle_error(info, error): # freeze the workflow update = UpdateProcessor('workflow', clauses=['id=%(id)s'], values=info) update.set(frozen=True) @@ -276,7 +278,7 @@ def handle_error(info, err): # record the error error_data = { - 'error': str(err), # TODO traceback? + 'error': error, # TODO traceback? 'workflow_data': info['data'], } data = { @@ -949,7 +951,7 @@ class NewRepoWorkflow(BaseWorkflow): self.data['repo_tasks'] = repo_tasks @subtask() - def repo_done(self, event, cloned, repo_tasks): + def repo_done(self, cloned, repo_tasks, event=None): data = cloned.copy() for arch in repo_tasks: data[arch] = kojihub.Task(repo_tasks[arch]).getResult() From eeb09228a26a78d3b405e3c520e49bfe2a8ad9f7 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:37:15 +0000 Subject: [PATCH 33/51] ... --- diff --git a/builder/kojid b/builder/kojid index 4cc9f5a..79bff04 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5790,34 +5790,7 @@ class NewRepoTask(BaseTaskHandler): # otherwise we need a createrepo task for this arch needed_arches.append(arch) - return {'cloned': data, 'needed': needed_arches} - - subtasks = {} - for arch in needed_archs: - # if we can't copy old repo directly, trigger normal createrepo - arglist = [repo_id, arch, oldrepo] - subtasks[arch] = self.session.host.subtask(method='createrepo', - arglist=arglist, - label=arch, - parent=self.id, - arch='noarch') - # gather subtask results - if subtasks: - results = self.wait(to_list(subtasks.values()), all=True, failany=True) - for (arch, task_id) in six.iteritems(subtasks): - data[arch] = results[task_id] - - # finalize - kwargs = {} - if event is not None: - kwargs['expire'] = True - if cloned_archs: - kwargs['repo_json_updates'] = { - 'cloned_from_repo_id': oldrepo['id'], - 'cloned_archs': cloned_archs, - } - self.session.host.repoDone(repo_id, data, **kwargs) - return repo_id, event_id + return {'cloned': data, 'needed': needed_arches, 'oldrepo': oldrepo} class CreaterepoTask(BaseTaskHandler): diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 3af58bb..70cfde4 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -5,6 +5,7 @@ import time import koji from koji.context import context +from koji.util import dslice from . import kojihub from .scheduler import log_both from .db import QueryProcessor, InsertProcessor, UpsertProcessor, UpdateProcessor, \ @@ -405,7 +406,6 @@ class BaseWorkflow: opts={'order': 'id'}) mywaits = query.execute() waiting = [] - fulfilled = [] for info in mywaits: if not info['fulfilled']: # TODO should we call check here as well? @@ -905,12 +905,14 @@ class TestWorkflow(BaseWorkflow): # STEPS = ['start', 'finish'] PARAMS = {'a': int, 'b': (int, type(None)), 'c': str} + @TestWorkflow.step() def start(workflow, a, b): # fire off a do-nothing task logger.info('TEST WORKFLOW START') workflow.data['task_id'] = workflow.task('sleep', {'n': 1}) + @subtask() @TestWorkflow.step() def finish(): @@ -933,25 +935,26 @@ class NewRepoWorkflow(BaseWorkflow): tinfo = kojihub.get_tag(tag, strict=True, event=event) opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src'), strict=True) # TODO further opts validation? - repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) + repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, + task_id=self.info['stub_id'], **opts) repo_info = kojihub.repo_info(repo_id) kw = {'tag': tinfo, 'repo': repo_info, 'opts': opts} self.data['prep_id'] = self.task('prepRepo', kw) self.data['repo'] = repo_info - def repos(self, prep_id): + def repos(self, prep_id, repo): # TODO better mechanism for fetching task result prepdata = kojihub.Task(prep_id).getResult() repo_tasks = [] for arch in prepdata['needed']: - params = {'repo_id': repo_id, 'arch': arch, 'oldrepo': oldrepo} + params = {'repo_id': repo['id'], 'arch': arch, 'oldrepo': prepdata['oldrepo']} repo_tasks[arch] = self.task('createrepo', params) # TODO fail workflow on any failed subtask self.data['cloned'] = prepdata['cloned'] self.data['repo_tasks'] = repo_tasks @subtask() - def repo_done(self, cloned, repo_tasks, event=None): + def repo_done(self, repo, cloned, repo_tasks, event=None): data = cloned.copy() for arch in repo_tasks: data[arch] = kojihub.Task(repo_tasks[arch]).getResult() @@ -964,7 +967,10 @@ class NewRepoWorkflow(BaseWorkflow): 'cloned_from_repo_id': 0, # XXX 'cloned_archs': list(sorted(cloned)), } - kojihub.repo_done(repo_id, data, **kwargs) + kojihub.repo_done(repo['id'], data, **kwargs) + + # do we need a return? + return repo['id'], repo['event_id'] class WorkflowExports: From 90047a3fdb4208e8e92c3a5471e9f11885700ca1 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:40:13 +0000 Subject: [PATCH 34/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 70cfde4..e89a14b 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -933,6 +933,8 @@ class NewRepoWorkflow(BaseWorkflow): @subtask() def repo_init(self, tag, event=None, opts=None): tinfo = kojihub.get_tag(tag, strict=True, event=event) + if opts is None: + opts = {} opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src'), strict=True) # TODO further opts validation? repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, From 8355ccb6400899c2dbc5a7e7e39173910154c8b5 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:43:13 +0000 Subject: [PATCH 35/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index e89a14b..4fe423f 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -935,7 +935,7 @@ class NewRepoWorkflow(BaseWorkflow): tinfo = kojihub.get_tag(tag, strict=True, event=event) if opts is None: opts = {} - opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src'), strict=True) + opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src')) # TODO further opts validation? repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) From dd02382591c8cb181a9ef66e75d968089acb289c Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:44:57 +0000 Subject: [PATCH 36/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 4fe423f..0922c25 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -933,6 +933,7 @@ class NewRepoWorkflow(BaseWorkflow): @subtask() def repo_init(self, tag, event=None, opts=None): tinfo = kojihub.get_tag(tag, strict=True, event=event) + event = kojihub.convert_value(event, cast=int, none_allowed=True) if opts is None: opts = {} opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src')) From 32c185671bd429e73250fe8b695c522941cc1ce6 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 04 2024 21:46:50 +0000 Subject: [PATCH 37/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 0922c25..24f3494 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -936,7 +936,7 @@ class NewRepoWorkflow(BaseWorkflow): event = kojihub.convert_value(event, cast=int, none_allowed=True) if opts is None: opts = {} - opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src')) + opts = dslice(opts, ('with_src', 'with_debuginfo', 'with_separate_src'), strict=False) # TODO further opts validation? repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) From 9f768a6cbb602decab824dd71c7bb9fa1a488f6d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 01:52:05 +0000 Subject: [PATCH 38/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 24f3494..c210a7c 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -941,6 +941,7 @@ class NewRepoWorkflow(BaseWorkflow): repo_id, event_id = kojihub.repo_init(tinfo['id'], event=event, task_id=self.info['stub_id'], **opts) repo_info = kojihub.repo_info(repo_id) + del repo_info['creation_time'] # json unfriendly kw = {'tag': tinfo, 'repo': repo_info, 'opts': opts} self.data['prep_id'] = self.task('prepRepo', kw) self.data['repo'] = repo_info From d84c5164a7d2edb085cf77f11167c7a34331e3ef Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 01:53:39 +0000 Subject: [PATCH 39/51] ... --- diff --git a/builder/kojid b/builder/kojid index 79bff04..452a5f7 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5733,7 +5733,7 @@ class NewRepoTask(BaseTaskHandler): # workflow has already called repo_init tinfo = tag repo_id = repo['id'] - event_id = repo['event_id'] + event_id = repo['create_event'] path = koji.pathinfo.repo(repo_id, tinfo['name']) if not os.path.isdir(path): From 7dc24f3d2f1f18489180d9d1feeb0c3740e75c3d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 02:08:44 +0000 Subject: [PATCH 40/51] ... --- diff --git a/builder/kojid b/builder/kojid index 452a5f7..dd4c192 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5788,9 +5788,9 @@ class NewRepoTask(BaseTaskHandler): cloned_archs.append(arch) continue # otherwise we need a createrepo task for this arch - needed_arches.append(arch) + needed_archs.append(arch) - return {'cloned': data, 'needed': needed_arches, 'oldrepo': oldrepo} + return {'cloned': data, 'needed': needed_archs, 'oldrepo': oldrepo} class CreaterepoTask(BaseTaskHandler): From f55913a23275bbe50c2ae94416b5458beda451d2 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:19:04 +0000 Subject: [PATCH 41/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index c210a7c..5958e99 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -531,6 +531,7 @@ class BaseWorkflow: task_id = kojihub.make_task(method, args, **opts) if wait: self.wait_task(task_id) + return task_id def wait_slot(self, name, request=True): self.wait('slot', {'name': name}) From 1044cf883598d9e6922e1cc6a73ea3e9c26f954d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:20:39 +0000 Subject: [PATCH 42/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 5958e99..193f536 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -950,7 +950,7 @@ class NewRepoWorkflow(BaseWorkflow): def repos(self, prep_id, repo): # TODO better mechanism for fetching task result prepdata = kojihub.Task(prep_id).getResult() - repo_tasks = [] + repo_tasks = {} for arch in prepdata['needed']: params = {'repo_id': repo['id'], 'arch': arch, 'oldrepo': prepdata['oldrepo']} repo_tasks[arch] = self.task('createrepo', params) From 12aa4670b9d1b1c8b705e8324a9eccab3ececc73 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:26:15 +0000 Subject: [PATCH 43/51] repo_done() --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 7dbd0db..3589abf 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -3002,6 +3002,78 @@ def dist_repo_init(tag, keys, task_opts): return repo_id, event +def repo_done(repo_id, data, expire=False, repo_json_updates=None): + """Finalize a repo + + repo_id: the id of the repo + data: a dictionary of repo files in the form: + { arch: [uploadpath, [file1, file2, ...]], ...} + expire: if set to true, mark the repo expired immediately [*] + repo_json_updates: dict - if provided it will be shallow copied + into repo.json file + + Actions: + * Move uploaded repo files into place + * Mark repo ready + * Expire earlier repos + * Move/create 'latest' symlink + + For dist repos, the move step is skipped (that is handled in + distRepoMove). + + * This is used when a repo from an older event is generated + """ + rinfo = repo_info(repo_id, strict=True) + convert_value(data, cast=dict, check_only=True) + koji.plugin.run_callbacks('preRepoDone', repo=rinfo, data=data, expire=expire) + if rinfo['state'] != koji.REPO_INIT: + raise koji.GenericError("Repo %(id)s not in INIT state (got %(state)s)" % rinfo) + repodir = koji.pathinfo.repo(repo_id, rinfo['tag_name']) + workdir = koji.pathinfo.work() + if repo_json_updates: + repo_json = koji.load_json(f'{repodir}/repo.json') + repo_json.update(repo_json_updates) + koji.dump_json(f'{repodir}/repo.json', repo_json, indent=2) + if not rinfo['dist']: + for arch, (uploadpath, files) in data.items(): + archdir = "%s/%s" % (repodir, koji.canonArch(arch)) + if not os.path.isdir(archdir): + raise koji.GenericError("Repo arch directory missing: %s" % archdir) + datadir = "%s/repodata" % archdir + koji.ensuredir(datadir) + for fn in files: + src = "%s/%s/%s" % (workdir, uploadpath, fn) + if fn.endswith('pkglist'): + dst = '%s/%s' % (archdir, fn) + else: + dst = "%s/%s" % (datadir, fn) + if not os.path.exists(src): + raise koji.GenericError("uploaded file missing: %s" % src) + safer_move(src, dst) + if expire: + repo_expire(repo_id) + koji.plugin.run_callbacks('postRepoDone', repo=rinfo, data=data, expire=expire) + return + # else: + repo_ready(repo_id) + repo_expire_older(rinfo['tag_id'], rinfo['create_event'], rinfo['dist']) + + # make a latest link + if rinfo['dist']: + latestrepolink = koji.pathinfo.distrepo('latest', rinfo['tag_name']) + else: + latestrepolink = koji.pathinfo.repo('latest', rinfo['tag_name']) + # XXX - this is a slight abuse of pathinfo + try: + if os.path.lexists(latestrepolink): + os.unlink(latestrepolink) + os.symlink(str(repo_id), latestrepolink) + except OSError: + # making this link is nonessential + log_error("Unable to create latest link for repo: %s" % repodir) + koji.plugin.run_callbacks('postRepoDone', repo=rinfo, data=data, expire=expire) + + def repo_set_state(repo_id, state, check=True): """Set repo state""" repo_id = convert_value(repo_id, cast=int) @@ -15760,55 +15832,7 @@ class HostExports(object): """ host = Host() host.verify() - rinfo = repo_info(repo_id, strict=True) - convert_value(data, cast=dict, check_only=True) - koji.plugin.run_callbacks('preRepoDone', repo=rinfo, data=data, expire=expire) - if rinfo['state'] != koji.REPO_INIT: - raise koji.GenericError("Repo %(id)s not in INIT state (got %(state)s)" % rinfo) - repodir = koji.pathinfo.repo(repo_id, rinfo['tag_name']) - workdir = koji.pathinfo.work() - if repo_json_updates: - repo_json = koji.load_json(f'{repodir}/repo.json') - repo_json.update(repo_json_updates) - koji.dump_json(f'{repodir}/repo.json', repo_json, indent=2) - if not rinfo['dist']: - for arch, (uploadpath, files) in data.items(): - archdir = "%s/%s" % (repodir, koji.canonArch(arch)) - if not os.path.isdir(archdir): - raise koji.GenericError("Repo arch directory missing: %s" % archdir) - datadir = "%s/repodata" % archdir - koji.ensuredir(datadir) - for fn in files: - src = "%s/%s/%s" % (workdir, uploadpath, fn) - if fn.endswith('pkglist'): - dst = '%s/%s' % (archdir, fn) - else: - dst = "%s/%s" % (datadir, fn) - if not os.path.exists(src): - raise koji.GenericError("uploaded file missing: %s" % src) - safer_move(src, dst) - if expire: - repo_expire(repo_id) - koji.plugin.run_callbacks('postRepoDone', repo=rinfo, data=data, expire=expire) - return - # else: - repo_ready(repo_id) - repo_expire_older(rinfo['tag_id'], rinfo['create_event'], rinfo['dist']) - - # make a latest link - if rinfo['dist']: - latestrepolink = koji.pathinfo.distrepo('latest', rinfo['tag_name']) - else: - latestrepolink = koji.pathinfo.repo('latest', rinfo['tag_name']) - # XXX - this is a slight abuse of pathinfo - try: - if os.path.lexists(latestrepolink): - os.unlink(latestrepolink) - os.symlink(str(repo_id), latestrepolink) - except OSError: - # making this link is nonessential - log_error("Unable to create latest link for repo: %s" % repodir) - koji.plugin.run_callbacks('postRepoDone', repo=rinfo, data=data, expire=expire) + return repo_done(repo_id, data, expire=expire, repo_json_updates=repo_json_updates) def distRepoMove(self, repo_id, uploadpath, arch): """ From 2587aaa680af8d65ea5d8fa1882bddbdf2801442 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:29:01 +0000 Subject: [PATCH 44/51] ... --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index 193f536..eaa14c5 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -975,7 +975,7 @@ class NewRepoWorkflow(BaseWorkflow): kojihub.repo_done(repo['id'], data, **kwargs) # do we need a return? - return repo['id'], repo['event_id'] + return repo['id'], repo['create_event'] class WorkflowExports: From 99b3cf1da69c4720bef4c4b7cc0ee620253c67a1 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:33:43 +0000 Subject: [PATCH 45/51] ... --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 3589abf..7772674 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -694,6 +694,7 @@ def make_task(method, arglist, **opts): opts.setdefault(f, pdata[f]) opts.setdefault('label', None) else: + pdata = None opts.setdefault('priority', koji.PRIO_DEFAULT) # calling function should enforce priority limitations, if applicable opts.setdefault('arch', 'noarch') From 014d1b621c88296c4bc5f6542a5f24a6482b3e30 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:43:02 +0000 Subject: [PATCH 46/51] keep original newRepo handler --- diff --git a/builder/kojid b/builder/kojid index dd4c192..090fde9 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5639,6 +5639,189 @@ Build Info: %(weburl)s/buildinfo?buildID=%(build_id)i\r class NewRepoTask(BaseTaskHandler): + Methods = ['newRepo'] + _taskWeight = 0.1 + + def copy_arch_repo(self, src_repo_id, src_repo_path, repo_id, arch): + """Copy repodata, return False if it fails""" + dst_repodata = joinpath(self.workdir, arch, 'repodata') + src_repodata = joinpath(src_repo_path, arch, 'repodata') + try: + # copy repodata + self.logger.debug('Copying repodata %s to %s' % (src_repodata, dst_repodata)) + if os.path.exists(src_repodata): + # symlink=True is not needed as they are no part of arch repodir + shutil.copytree(src_repodata, dst_repodata) + uploadpath = self.getUploadDir() + files = [] + for f in os.listdir(dst_repodata): + files.append(f) + self.session.uploadWrapper('%s/%s' % (dst_repodata, f), uploadpath, f) + return [uploadpath, files] + except Exception as ex: + self.logger.warning("Copying repo %i to %i failed. %r" % (src_repo_id, repo_id, ex)) + # Try to remove potential leftovers and fail if there is some problem + koji.util.rmtree(dst_repodata, self.logger) + return False + + def check_repo(self, src_repo_path, dst_repo_path, src_repo, dst_repo, opts): + """Check if oldrepo is reusable as is and can be directly copied""" + # with_src, debuginfo, pkglist, blocklist, grouplist + # We're ignoring maven support here. It is handled in repo_init which is called + # always, so it doesn't affect efficiency of pre-cloning rpm repos. + if not src_repo_path: + self.logger.debug("Source repo wasn't found") + return False + if not os.path.isdir(src_repo_path): + self.logger.debug("Source repo doesn't exist %s" % src_repo_path) + return False + try: + repo_json = koji.load_json(joinpath(src_repo_path, 'repo.json')) + for key in ('with_debuginfo', 'with_src', 'with_separate_src'): + if repo_json.get(key, False) != opts.get(key, False): + return False + except IOError: + self.logger.debug("Can't open repo.json for repo {repo_info['id']}") + return False + + # compare comps if they exist + src_comps_path = joinpath(src_repo_path, 'groups/comps.xml') + dst_comps_path = joinpath(dst_repo_path, 'groups/comps.xml') + src_exists = os.path.exists(src_comps_path) + if src_exists != os.path.exists(dst_comps_path): + self.logger.debug("Comps exists only in one repo") + return False + if src_exists and not filecmp.cmp(src_comps_path, dst_comps_path, shallow=False): + self.logger.debug("Comps differs") + return False + + # if there is any external repo, don't trust the repodata + if self.session.getExternalRepoList(src_repo['tag_id'], event=src_repo['create_event']): + self.logger.debug("Source repo use external repos") + return False + if self.session.getExternalRepoList(dst_repo['tag_id'], event=dst_repo['create_event']): + self.logger.debug("Destination repo use external repos") + return False + + self.logger.debug('Repo test passed') + return True + + def check_arch_repo(self, src_repo_path, dst_repo_path, arch): + """More checks based on architecture content""" + for fname in ('blocklist', 'pkglist'): + src_file = joinpath(src_repo_path, arch, fname) + dst_file = joinpath(dst_repo_path, arch, fname) + # both must non/exist + if not os.path.exists(src_file) or not os.path.exists(dst_file): + self.logger.debug("%s doesn't exit in one of the repos" % fname) + return False + # content must be same + if not filecmp.cmp(src_file, dst_file, shallow=False): + self.logger.debug('%s differs' % fname) + return False + self.logger.debug('Arch repo test passed %s' % arch) + return True + + def handler(self, tag, event=None, src=False, debuginfo=False, separate_src=False): + tinfo = self.session.getTag(tag, strict=True, event=event) + + # check for fs access before we try calling repoInit + top_repos_dir = joinpath(self.options.topdir, "repos") + if not os.path.isdir(top_repos_dir): + # missing or incorrect mount? + # refuse and let another host try + raise RefuseTask("No access to repos dir %s" % top_repos_dir) + + # call repoInit + kwargs = {} + if event is not None: + kwargs['event'] = event + if src: + kwargs['with_src'] = True + if separate_src: + kwargs['with_separate_src'] = True + # generate debuginfo repo if requested or if specified in sidetag's extra + if debuginfo or tinfo['extra'].get('with_debuginfo'): + kwargs['with_debuginfo'] = True + repo_id, event_id = self.session.host.repoInit(tinfo['id'], task_id=self.id, **kwargs) + + path = koji.pathinfo.repo(repo_id, tinfo['name']) + if not os.path.isdir(path): + raise koji.GenericError("Repo directory missing: %s" % path) + arches = [] + for fn in os.listdir(path): + if fn != 'groups' and os.path.isfile("%s/%s/pkglist" % (path, fn)): + arches.append(fn) + # see if we can find a previous repo to update from + # only shadowbuild tags should start with SHADOWBUILD, their repos are auto + # expired. so lets get the most recent expired tag for newRepo shadowbuild tasks. + if tinfo['name'].startswith('SHADOWBUILD'): + oldrepo_state = koji.REPO_EXPIRED + else: + oldrepo_state = koji.REPO_READY + oldrepo = self.session.getRepo(tinfo['id'], state=oldrepo_state) + oldrepo_path = None + if oldrepo: + oldrepo_path = koji.pathinfo.repo(oldrepo['id'], tinfo['name']) + oldrepo['tag_id'] = tinfo['id'] + # If there is no old repo, try to find first usable repo in + # inheritance chain and use it as a source. oldrepo is not used if + # createrepo_update is not set, so don't waste call in such case. + if not oldrepo and self.options.createrepo_update: + tags = self.session.getFullInheritance(tinfo['id']) + # we care about best candidate which should be (not necessarily) + # something on higher levels. Sort tags according to depth. + for tag in sorted(tags, key=lambda x: x['currdepth']): + oldrepo = self.session.getRepo(tag['parent_id'], state=oldrepo_state) + if oldrepo: + parenttag = self.session.getTag(tag['parent_id']) + oldrepo_path = koji.pathinfo.repo(oldrepo['id'], parenttag['name']) + oldrepo['tag_id'] = parenttag['id'] + break + newrepo_path = koji.pathinfo.repo(repo_id, tinfo['name']) + newrepo = {'tag_id': tinfo['id'], 'create_event': event_id} + if self.options.copy_old_repodata: + possibly_clonable = self.check_repo(oldrepo_path, newrepo_path, + oldrepo, newrepo, kwargs) + else: + possibly_clonable = False + subtasks = {} + data = {} + cloned_archs = [] + for arch in arches: + if possibly_clonable and self.check_arch_repo(oldrepo_path, newrepo_path, arch): + result = self.copy_arch_repo(oldrepo['id'], oldrepo_path, repo_id, arch) + if result: + data[arch] = result + cloned_archs.append(arch) + continue + # if we can't copy old repo directly, trigger normal createrepo + arglist = [repo_id, arch, oldrepo] + subtasks[arch] = self.session.host.subtask(method='createrepo', + arglist=arglist, + label=arch, + parent=self.id, + arch='noarch') + # gather subtask results + if subtasks: + results = self.wait(to_list(subtasks.values()), all=True, failany=True) + for (arch, task_id) in six.iteritems(subtasks): + data[arch] = results[task_id] + + # finalize + kwargs = {} + if event is not None: + kwargs['expire'] = True + if cloned_archs: + kwargs['repo_json_updates'] = { + 'cloned_from_repo_id': oldrepo['id'], + 'cloned_archs': cloned_archs, + } + self.session.host.repoDone(repo_id, data, **kwargs) + return repo_id, event_id + + +class NewRepoTask(BaseTaskHandler): Methods = ['prepRepo'] _taskWeight = 0.5 From a9428deed66fa4da118ae27c3818f7c70ec8b048 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 03:46:27 +0000 Subject: [PATCH 47/51] subclass NewRepo --- diff --git a/builder/kojid b/builder/kojid index 090fde9..9566857 100755 --- a/builder/kojid +++ b/builder/kojid @@ -5821,90 +5821,10 @@ class NewRepoTask(BaseTaskHandler): return repo_id, event_id -class NewRepoTask(BaseTaskHandler): +class PrepRepoTask(BaseTaskHandler): Methods = ['prepRepo'] _taskWeight = 0.5 - def copy_arch_repo(self, src_repo_id, src_repo_path, repo_id, arch): - """Copy repodata, return False if it fails""" - dst_repodata = joinpath(self.workdir, arch, 'repodata') - src_repodata = joinpath(src_repo_path, arch, 'repodata') - try: - # copy repodata - self.logger.debug('Copying repodata %s to %s' % (src_repodata, dst_repodata)) - if os.path.exists(src_repodata): - # symlink=True is not needed as they are no part of arch repodir - shutil.copytree(src_repodata, dst_repodata) - uploadpath = self.getUploadDir() - files = [] - for f in os.listdir(dst_repodata): - files.append(f) - self.session.uploadWrapper('%s/%s' % (dst_repodata, f), uploadpath, f) - return [uploadpath, files] - except Exception as ex: - self.logger.warning("Copying repo %i to %i failed. %r" % (src_repo_id, repo_id, ex)) - # Try to remove potential leftovers and fail if there is some problem - koji.util.rmtree(dst_repodata, self.logger) - return False - - def check_repo(self, src_repo_path, dst_repo_path, src_repo, dst_repo, opts): - """Check if oldrepo is reusable as is and can be directly copied""" - # with_src, debuginfo, pkglist, blocklist, grouplist - # We're ignoring maven support here. It is handled in repo_init which is called - # always, so it doesn't affect efficiency of pre-cloning rpm repos. - if not src_repo_path: - self.logger.debug("Source repo wasn't found") - return False - if not os.path.isdir(src_repo_path): - self.logger.debug("Source repo doesn't exist %s" % src_repo_path) - return False - try: - repo_json = koji.load_json(joinpath(src_repo_path, 'repo.json')) - for key in ('with_debuginfo', 'with_src', 'with_separate_src'): - if repo_json.get(key, False) != opts.get(key, False): - return False - except IOError: - self.logger.debug("Can't open repo.json for repo {repo_info['id']}") - return False - - # compare comps if they exist - src_comps_path = joinpath(src_repo_path, 'groups/comps.xml') - dst_comps_path = joinpath(dst_repo_path, 'groups/comps.xml') - src_exists = os.path.exists(src_comps_path) - if src_exists != os.path.exists(dst_comps_path): - self.logger.debug("Comps exists only in one repo") - return False - if src_exists and not filecmp.cmp(src_comps_path, dst_comps_path, shallow=False): - self.logger.debug("Comps differs") - return False - - # if there is any external repo, don't trust the repodata - if self.session.getExternalRepoList(src_repo['tag_id'], event=src_repo['create_event']): - self.logger.debug("Source repo use external repos") - return False - if self.session.getExternalRepoList(dst_repo['tag_id'], event=dst_repo['create_event']): - self.logger.debug("Destination repo use external repos") - return False - - self.logger.debug('Repo test passed') - return True - - def check_arch_repo(self, src_repo_path, dst_repo_path, arch): - """More checks based on architecture content""" - for fname in ('blocklist', 'pkglist'): - src_file = joinpath(src_repo_path, arch, fname) - dst_file = joinpath(dst_repo_path, arch, fname) - # both must non/exist - if not os.path.exists(src_file) or not os.path.exists(dst_file): - self.logger.debug("%s doesn't exit in one of the repos" % fname) - return False - # content must be same - if not filecmp.cmp(src_file, dst_file, shallow=False): - self.logger.debug('%s differs' % fname) - return False - self.logger.debug('Arch repo test passed %s' % arch) - return True - def handler(self, tag, repo, opts): # check for fs access before we go any further top_repos_dir = joinpath(self.options.topdir, "repos") From ee5c7c8079239feebb9249fae4eb6f97f6407b03 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 06:08:58 +0000 Subject: [PATCH 48/51] add a lock --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index eaa14c5..b9f9eed 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -58,8 +58,9 @@ def nudge_queue(): if queue_next(): # if we handled a queue item, we're done return True - update_queue() - handle_slots() + if db_lock('workflow_maint', wait=False): + update_queue() + handle_slots() return False # TODO should we return something more informative? diff --git a/schemas/schema.sql b/schemas/schema.sql index 7c3049e..a841240 100644 --- a/schemas/schema.sql +++ b/schemas/schema.sql @@ -1119,5 +1119,6 @@ INSERT INTO locks(name) VALUES('protonmsg-plugin'); INSERT INTO locks(name) VALUES('scheduler'); INSERT INTO locks(name) VALUES('workflow_queue'); INSERT INTO locks(name) VALUES('workflow_slots'); +INSERT INTO locks(name) VALUES('workflow_maint'); COMMIT WORK; From 7cb41355b88fc454e0e8a1c61ef9281719dc7a2b Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 06:23:21 +0000 Subject: [PATCH 49/51] wait for all the waits before next step --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index b9f9eed..d5d7e33 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -334,7 +334,10 @@ class BaseWorkflow: if opts is None: opts = {} - self.handle_waits() + if self.handle_waits(): + # we are still waiting, so we can't go to next step + self.update() + return # TODO error handling step = self.data['steps'].pop(0) @@ -415,6 +418,7 @@ class BaseWorkflow: cls = waits.get(info['wait_type']) wait = cls(info) wait.handle(workflow=self) + self.log('Handled %(wait_type)s wait %(id)s' % info) return bool(waiting) def log(self, msg, level=logging.INFO): From f3c9050a6d9a4137dfd39e815bd36df9636437f1 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 06:41:38 +0000 Subject: [PATCH 50/51] don't wait on workflowStep tasks --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index d5d7e33..dd9841c 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -353,8 +353,9 @@ class BaseWorkflow: # otherwise we're good elif is_subtask: # this step needs to run via a subtask - self.task('workflowStep', {'workflow_id': self.info['id'], 'step': step}) - # TODO handle task failure without looping + self.task('workflowStep', {'workflow_id': self.info['id'], 'step': step}, wait=False) + # we don't need to wait for this one, because it calls us + # TODO handle task failure without stalling return # TODO slots are a better idea for tasks than for workflows From 141c48c792226d15b384c59796ffa5fb3a34316d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Mar 05 2024 07:27:50 +0000 Subject: [PATCH 51/51] trying new-repo without subtask flag --- diff --git a/kojihub/workflow.py b/kojihub/workflow.py index dd9841c..e6fb82d 100644 --- a/kojihub/workflow.py +++ b/kojihub/workflow.py @@ -937,7 +937,7 @@ class NewRepoWorkflow(BaseWorkflow): 'opts': (dict,), } - @subtask() + #@subtask() def repo_init(self, tag, event=None, opts=None): tinfo = kojihub.get_tag(tag, strict=True, event=event) event = kojihub.convert_value(event, cast=int, none_allowed=True) @@ -964,7 +964,7 @@ class NewRepoWorkflow(BaseWorkflow): self.data['cloned'] = prepdata['cloned'] self.data['repo_tasks'] = repo_tasks - @subtask() + #@subtask() def repo_done(self, repo, cloned, repo_tasks, event=None): data = cloned.copy() for arch in repo_tasks: