diff --git a/CHANGELOG.md b/CHANGELOG.md index 832c005..4ee7097 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,10 +55,23 @@ ADR-007 and ADR-002. that had been pulled out. The link is a `controls` relationship carrying a `collector:machine` origin label, so a link made by hand is never touched. Superseding a PC archives the old link rather than deleting it, keeping the - history of which PC ran a machine and when, and raises an alert by email and - webhook. The retired PC's status is deliberately left alone: the collector - cannot tell whether it was shelved, sent for repair or re-imaged for another - bay, so a person decides. + history of which PC ran a machine and when. The retired PC's status is + deliberately left alone: the collector cannot tell whether it was shelved, + sent for repair or re-imaged for another bay, so a person decides. + + A second PC reporting a machine another PC already holds is treated as a + claim, not a handover. A PC imaged for a machine carries that number before + it ever reaches the floor, so the PC actually running the machine keeps the + link while it is still reporting and still In Use, and the challenger is + recorded as a dormant link. The handover completes on its own once the old PC + has been quiet for a day, or immediately when someone moves it off In Use. + Without this the two PCs traded the link back and forth on every collector + cycle. + + Alerts for both cases go out by email and webhook, gated on a new + `computers_machinelink_alerts` setting that ships OFF: a site may legitimately + run several PCs on one machine number, and there the alerts fire on correct + data. The collector response warns either way. - Plugin contract 0.16.0: `get_settings_defaults()` lets a plugin declare the settings it owns (key, default, type, category, description, and whether an unauthenticated caller may read it). See `docs/PLUGIN-HOOKS.md`. diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index 19e9a01..69cc4e6 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -34,6 +34,12 @@ MEASURINGTOOL_LINK_ORIGIN = 'collector:measuringtool' # touched. MACHINE_LINK_ORIGIN = 'collector:machine' +# How long a PC holding a machine may go without reporting before a second PC +# claiming that machine is treated as its replacement. A swap resolves itself +# within a day; a PC off overnight or behind a network outage keeps its bay. +# Shorter than this and a spare imaged on the bench could steal a live machine. +MACHINE_CLAIM_QUIET_HOURS = 24 + class ComputersPlugin(BasePlugin): """ @@ -145,6 +151,46 @@ class ComputersPlugin(BasePlugin): }, } + def get_settings_defaults(self) -> List[dict]: + """Settings this plugin owns. + + The framework seeds these at install, at enable, and on every + `flask plugin upgrade-all`, so a key added in a later version reaches a + site that installed an earlier one. + """ + return [ + { + 'key': 'computers_machinelink_alerts', + 'value': 'false', + 'valuetype': 'boolean', + 'category': 'computers', + 'description': 'Email and webhook alerts when a PC takes over ' + 'a machine or claims one another PC still runs. ' + 'Off while machine numbers are shared between ' + 'PCs; the collector response still warns.', + }, + ] + + def _machinelink_alerts_enabled(self): + """Whether the PC-to-machine alerts may be sent. + + Off by default, deliberately. A site may legitimately run several PCs + on one machine number - part markers do at West Jefferson - and there + both the handover and the contested case fire on normal, correct data, + which is noise rather than news. Turn it on at a site where a machine + number means exactly one PC. + + The links, the warnings in the collector response, and the archived + history all continue regardless. Only the sending is gated. + """ + from shopdb.api import Setting + + setting = Setting.query.filter_by( + key='computers_machinelink_alerts').first() + if not setting: + return False + return (setting.value or '').strip().lower() in ('true', '1', 'yes') + def apply_collector_payload(self, payload: Dict) -> Dict: """Idempotent upsert of a PC from a collector payload (by hostname).""" from datetime import datetime, timezone @@ -527,6 +573,14 @@ class ComputersPlugin(BasePlugin): deliberately NOT changed: the collector cannot tell whether it was shelved, broken or re-imaged for another bay, and guessing would overwrite whatever a person deliberately set. + + A second PC reporting a machine another PC already holds is a CLAIM, not + proof of replacement. A PC imaged on the bench for machine 3010 carries + that number before it ever reaches the floor, and treating the claim as + a handover made the two PCs trade the link back and forth at collector + cadence, alerting on every pass. The incumbent therefore keeps the + machine while it is still alive, and the challenger is recorded as a + dormant link. See _incumbent_has_yielded for what alive means. """ from shopdb.api import AssetRelationship, RelationshipType, Asset @@ -555,8 +609,8 @@ class ComputersPlugin(BasePlugin): if machine.assetid == pcasset.assetid: return [] - # This PC's own collector links: keep the one for the reported machine, - # archive any other (the PC moved bays). + # This PC's own collector links: the one for the reported machine is + # settled below, any other is archived (the PC moved bays). mine = AssetRelationship.query.filter( AssetRelationship.sourceassetid == pcasset.assetid, AssetRelationship.relationshiptypeid == controls.relationshiptypeid, @@ -566,10 +620,48 @@ class ComputersPlugin(BasePlugin): link = None for rel in mine: if rel.targetassetid == machine.assetid: - rel.isactive = True link = rel else: rel.isactive = False + + # Whoever actively holds this machine now, if it is not this PC. + held = AssetRelationship.query.filter( + AssetRelationship.targetassetid == machine.assetid, + AssetRelationship.relationshiptypeid == controls.relationshiptypeid, + AssetRelationship.label == MACHINE_LINK_ORIGIN, + AssetRelationship.sourceassetid != pcasset.assetid, + AssetRelationship.isactive.is_(True), + ).all() + blocking = [rel for rel in held + if not self._incumbent_has_yielded(rel.sourceassetid)] + + if blocking: + # Contested. The incumbent is still reporting and still In Use, so + # this is a claim on a bay it has not taken over yet. Record the + # claim dormant and leave the live link where it is; the dormant row + # is also the marker that says this was already announced, which is + # what stops an alert on every report. + names = ', '.join(self._assetname(rel.sourceassetid) + for rel in blocking) + warnings.append( + 'machine {} is still held by {}; {} claim recorded but not ' + 'linked'.format(machine.assetnumber, names, comp.hostname)) + if link is None: + db.session.add(AssetRelationship( + sourceassetid=pcasset.assetid, + targetassetid=machine.assetid, + relationshiptypeid=controls.relationshiptypeid, + label=MACHINE_LINK_ORIGIN, + isactive=False)) + self._alert_machine_contested(names, comp, machine) + else: + link.isactive = False + return [{'assetid': machine.assetid, + 'machinenumber': machine.assetnumber, + 'contestedby': names, + 'superseded': 0}] + + # Uncontested: take the machine. if link is None: link = AssetRelationship( sourceassetid=pcasset.assetid, @@ -578,19 +670,14 @@ class ComputersPlugin(BasePlugin): label=MACHINE_LINK_ORIGIN, isactive=True) db.session.add(link) + else: + link.isactive = True - # Any OTHER pc still collector-linked to this machine has been replaced. - superseded = AssetRelationship.query.filter( - AssetRelationship.targetassetid == machine.assetid, - AssetRelationship.relationshiptypeid == controls.relationshiptypeid, - AssetRelationship.label == MACHINE_LINK_ORIGIN, - AssetRelationship.sourceassetid != pcasset.assetid, - AssetRelationship.isactive.is_(True), - ).all() - for rel in superseded: + # Anything still holding it has yielded: gone quiet, or taken off In + # Use by a person. That is a replacement, so archive and say so. + for rel in held: rel.isactive = False - old = db.session.get(Asset, rel.sourceassetid) - oldname = old.assetnumber if old else str(rel.sourceassetid) + oldname = self._assetname(rel.sourceassetid) warnings.append( 'machine {} was taken over from {}; check that PC'.format( machine.assetnumber, oldname)) @@ -598,24 +685,119 @@ class ComputersPlugin(BasePlugin): return [{'assetid': machine.assetid, 'machinenumber': machine.assetnumber, - 'superseded': len(superseded)}] + 'superseded': len(held)}] + + def _assetname(self, assetid): + """Readable name for an asset id, for warnings and alerts.""" + from shopdb.api import Asset + + asset = db.session.get(Asset, assetid) + return asset.assetnumber if asset else str(assetid) + + def _incumbent_has_yielded(self, assetid): + """True when the PC currently holding a machine has given it up. + + Two ways to yield, and both are evidence rather than a guess: + + It went quiet. A PC pulled off a machine stops reporting, so silence + past MACHINE_CLAIM_QUIET_HOURS is the handover signal. The window is + long enough that a PC switched off overnight, or one behind a network + outage, never loses its bay to a spare sitting on the bench. + + Or a person moved it off In Use. Setting the old PC to Retired, + Inventory or In Repair is a deliberate statement that it no longer runs + the machine, and it is the one-step way to force a handover the moment + the swap happens instead of waiting out the window. + + An asset with no computer extension row cannot report at all, so it + cannot be alive; it yields. + """ + from datetime import datetime, timedelta, timezone + + from shopdb.api import Asset + + computer = Computer.query.filter_by(assetid=assetid).first() + if not computer: + return True + + asset = db.session.get(Asset, assetid) + status = asset.status.status if asset and asset.status else None + if status and status != 'In Use': + return True + + reported = computer.lastreporteddate + if not reported: + return True + quiet = datetime.now(timezone.utc).replace(tzinfo=None) - reported + return quiet > timedelta(hours=MACHINE_CLAIM_QUIET_HOURS) + + def _alert_machine_contested(self, holdername, newcomp, machine): + """Say that a PC claims a machine another PC is still running. + + Fires ONCE, on the report that first records the dormant claim. The + dormant relationship row is the marker: while it exists the claim is + already known, so a PC sitting on the bench for a fortnight does not + alert every collector cycle. + + Nothing is changed in the data by this. It exists because the two + legitimate readings - a swap in progress, or a machine number typed + onto the wrong PC at imaging - look identical to the collector, and + only a person can tell them apart. + """ + import logging as _logging + if not self._machinelink_alerts_enabled(): + return + try: + from shopdb.api import send_alert, Setting + + newname = newcomp.hostname if newcomp else 'a new PC' + machinename = machine.assetnumber or machine.name + subject = 'Machine {} claimed by {}, still run by {}'.format( + machinename, newname, holdername) + base = (Setting.get('site_base_url') or '').rstrip('/') + link = '{}/pcs/{}'.format(base, newcomp.computerid) if base else '' + linkhtml = ('

View {1}

'.format(link, newname) + if link else '') + html = ( + '

{0} reports that it runs machine ' + '{1}, but {2} is still ' + 'reporting on that machine and is still In Use, so the link ' + 'has been left where it is.

' + '

If this is a swap in progress, nothing needs doing: {0} ' + 'takes the machine once {2} stops reporting for a day, or ' + 'straight away if you set {2} to Retired, Inventory or In ' + 'Repair. If instead {0} was imaged with the wrong machine ' + 'number, correct it on {0}.

{3}'.format( + newname, machinename, holdername, linkhtml)) + send_alert(subject, html) + except Exception: + _logging.getLogger(__name__).exception( + 'machine-contested alert failed for machine %s', + getattr(machine, 'assetnumber', '?')) def _alert_pc_superseded(self, oldname, newcomp, machine): """Tell a human a PC was replaced on a machine. Best effort, never raises. Deliberately an ALERT and not a status change: the collector cannot tell whether the old PC was shelved, sent for repair or re-imaged for another - bay, so it says what happened and lets a person decide. Uses the same - email + webhook path as the toner alerts, falling back to the site-wide - alert_recipients when nothing more specific is set. + bay, so it says what happened and lets a person decide. + + Goes through send_alert, which is the site's configured alert fan-out: + email to the SMTP settings' alert recipients plus the alert webhook. + Resolving those recipients by hand would have missed the + SMTP_ALERT_RECIPIENTS environment fallback, so a site that configures + SMTP by environment rather than in the UI would have got the webhook + and no email. NOT a shopfloor notification: that board is for operators (General, Recertification, Recognition), and an IT asset message does not belong in front of the floor. """ import logging as _logging + if not self._machinelink_alerts_enabled(): + return try: - from shopdb.api import send_email, send_webhook, Setting + from shopdb.api import send_alert, Setting newname = newcomp.hostname if newcomp else 'a new PC' machinename = machine.assetnumber or machine.name @@ -635,13 +817,7 @@ class ComputersPlugin(BasePlugin): 'changed - set it to Inventory, In Repair or Retired as ' 'appropriate.

{3}'.format(newname, machinename, oldname, linkhtml)) - send_webhook(subject, subject) - - raw = Setting.get('alert_recipients') or '' - recipients = [a.strip() for a in raw.replace(';', ',').split(',') - if a.strip()] - if recipients: - send_email(recipients, subject, html) + send_alert(subject, html) except Exception: _logging.getLogger(__name__).exception( 'PC-superseded alert failed for machine %s', diff --git a/tests/test_core/test_collector_contract.py b/tests/test_core/test_collector_contract.py index 011f49a..9824d97 100644 --- a/tests/test_core/test_collector_contract.py +++ b/tests/test_core/test_collector_contract.py @@ -565,6 +565,52 @@ def _links(machineassetid, activeonly=True): return q.all() +def _go_quiet(db, hostname, hours=48): + """Backdate a PC's last report so it reads as gone from the floor. + + A PC pulled off a machine simply stops reporting, and there is no other + signal. Tests that want a completed handover have to age the incumbent + past MACHINE_CLAIM_QUIET_HOURS; without this it is a live PC and the + challenger is correctly held off. + """ + from datetime import datetime, timedelta, timezone + from plugins.computers.models import Computer + + comp = Computer.query.filter(Computer.hostname.ilike(hostname)).first() + comp.lastreporteddate = (datetime.now(timezone.utc).replace(tzinfo=None) + - timedelta(hours=hours)) + db.session.commit() + return comp + + +def _enable_machinelink_alerts(db): + """Turn the PC-to-machine alerts on. + + They ship OFF: several part markers legitimately share one machine number + (0613, 0615, WJPRT), so on this site the alerts would fire on correct data. + A test that wants to see one has to say so. + """ + from shopdb.core.models import Setting + db.session.add(Setting(key='computers_machinelink_alerts', value='true')) + db.session.commit() + + +def _set_status(db, hostname, statusname): + """Put a PC's asset on a named status, the way a person would in the UI.""" + from shopdb.core.models import AssetStatus + from plugins.computers.models import Computer + + status = AssetStatus.query.filter_by(status=statusname).first() + if not status: + status = AssetStatus(status=statusname, isactive=True) + db.session.add(status) + db.session.flush() + comp = Computer.query.filter(Computer.hostname.ilike(hostname)).first() + comp.asset.statusid = status.statusid + db.session.commit() + return comp + + def test_pc_reporting_an_existing_machine_number_does_not_500( client, db, collector_key, computer_assettype, machine_3015): """The machine owns assetnumber 3015. Naming the PC 3015 too hit the unique @@ -601,6 +647,7 @@ def test_replacing_a_pc_hands_the_machine_over( client.post('/api/collector/computers', json={'hostname': 'OLDPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC') r = client.post('/api/collector/computers', json={'hostname': 'NEWPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) @@ -614,12 +661,102 @@ def test_replacing_a_pc_hands_the_machine_over( assert any('taken over' in w for w in r.get_json()['data']['warnings']) +def test_a_live_pc_keeps_its_machine_against_a_new_claim( + client, db, collector_key, computer_assettype, machine_3015): + """A PC imaged for machine 3015 carries that number before it ever reaches + the floor. While the PC actually running the machine is still reporting, + the claim is not a handover and must not move the link.""" + from shopdb.core.models import Asset + client.post('/api/collector/computers', + json={'hostname': 'OLDPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + r = client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + + active = _links(machine_3015.assetid) + assert len(active) == 1 + assert db.session.get(Asset, active[0].sourceassetid).assetnumber == 'OLDPC' + assert any('still held by' in w for w in r.get_json()['data']['warnings']) + + # The claim is recorded, dormant, so nothing about it is lost. + assert len(_links(machine_3015.assetid, activeonly=False)) == 2 + + +def test_a_contested_claim_does_not_flap_or_realert( + client, db, collector_key, computer_assettype, machine_3015, + monkeypatch): + """Both PCs keep reporting on their own schedules. The old code let them + trade the link on every pass and alerted each time.""" + from shopdb.core.models import Asset + import shopdb.api as shopdbapi + + alerts = [] + monkeypatch.setattr(shopdbapi, 'send_alert', + lambda subj, html, **k: alerts.append(subj)) + _enable_machinelink_alerts(db) + + for _ in range(3): + client.post('/api/collector/computers', + json={'hostname': 'OLDPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + + active = _links(machine_3015.assetid) + assert len(active) == 1 + assert db.session.get(Asset, active[0].sourceassetid).assetnumber == 'OLDPC' + assert len(alerts) == 1, 'contested claim must alert once, not every cycle' + + +def test_retiring_the_old_pc_hands_the_machine_over_at_once( + client, db, collector_key, computer_assettype, machine_3015): + """The one-step way to force a swap without waiting out the quiet window: + a person says the old PC is no longer in use.""" + from shopdb.core.models import Asset + client.post('/api/collector/computers', + json={'hostname': 'OLDPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + _set_status(db, 'OLDPC', 'Retired') + + client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + + active = _links(machine_3015.assetid) + assert len(active) == 1 + assert db.session.get(Asset, active[0].sourceassetid).assetnumber == 'NEWPC' + + +def test_a_pc_off_overnight_does_not_lose_its_machine( + client, db, collector_key, computer_assettype, machine_3015): + """Shorter than the quiet window is not gone. A machine must not change + hands because a PC was switched off for the night or the network dropped.""" + from shopdb.core.models import Asset + client.post('/api/collector/computers', + json={'hostname': 'OLDPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC', hours=14) + client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + + active = _links(machine_3015.assetid) + assert len(active) == 1 + assert db.session.get(Asset, active[0].sourceassetid).assetnumber == 'OLDPC' + + def test_the_old_link_is_archived_not_deleted( client, db, collector_key, computer_assettype, machine_3015): """History has to survive: 'which PC ran 3015 in June' stays answerable.""" client.post('/api/collector/computers', json={'hostname': 'OLDPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC') client.post('/api/collector/computers', json={'hostname': 'NEWPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) @@ -636,6 +773,7 @@ def test_the_old_pc_status_is_left_alone( headers={'X-API-Key': collector_key}) old = Computer.query.filter(Computer.hostname.ilike('OLDPC')).first() before = old.asset.statusid + _go_quiet(db, 'OLDPC') client.post('/api/collector/computers', json={'hostname': 'NEWPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) @@ -674,33 +812,37 @@ def test_supersede_alerts_email_and_webhook(client, db, collector_key, # Patch on shopdb.api, which is where the plugin imports them from - the # names are bound there at import time, so patching shopdb.utils.mailer # would silently have no effect and the real (disabled) mailer would run. + # send_alert is the site's configured fan-out: alert_recipients by email + # plus the alert webhook. Patch the fan-out, not its two legs - resolving + # recipients here by hand is exactly the bug this replaced. sent = {} - monkeypatch.setattr( - shopdbapi, 'send_email', - lambda to, subj, html, **k: sent.update(email=(to, subj, html))) - monkeypatch.setattr(shopdbapi, 'send_webhook', - lambda subj, text, **k: sent.update(webhook=subj)) + monkeypatch.setattr(shopdbapi, 'send_alert', + lambda subj, html, **k: sent.update(alert=(subj, html))) db.session.add(Setting(key='alert_recipients', value='it@example.com')) db.session.add(Setting(key='site_base_url', value='https://shopdb/shopdb')) db.session.commit() + _enable_machinelink_alerts(db) client.post('/api/collector/computers', json={'hostname': 'OLDPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC') client.post('/api/collector/computers', json={'hostname': 'NEWPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) - assert 'webhook' in sent, 'webhook not fired on supersede' - assert 'email' in sent, 'no email sent on supersede' - assert sent['email'][0] == ['it@example.com'] - assert '3015' in sent['email'][1] and 'OLDPC' in sent['email'][1] + assert 'alert' in sent, 'no alert raised on supersede' + subject, html = sent['alert'] + assert '3015' in subject and 'OLDPC' in subject and 'NEWPC' in subject + # Must be the handover alert, not the contested-claim one. Both name the + # same three things, so the subjects alone do not tell them apart. + assert 'replaced' in subject, subject # The link must be keyed on computerid: /pcs/:id resolves through # GET /api/computers/, so an assetid opens the wrong PC. from plugins.computers.models import Computer newpc = Computer.query.filter_by(hostname='NEWPC').first() - assert '/pcs/{}"'.format(newpc.computerid) in sent['email'][2] + assert '/pcs/{}"'.format(newpc.computerid) in html def test_alert_failure_never_breaks_the_collector(client, db, collector_key, @@ -711,14 +853,41 @@ def test_alert_failure_never_breaks_the_collector(client, db, collector_key, def boom(*a, **k): raise RuntimeError('smtp down') - monkeypatch.setattr(shopdbapi, 'send_email', boom) - monkeypatch.setattr(shopdbapi, 'send_webhook', boom) + monkeypatch.setattr(shopdbapi, 'send_alert', boom) + _enable_machinelink_alerts(db) client.post('/api/collector/computers', json={'hostname': 'OLDPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC') r = client.post('/api/collector/computers', json={'hostname': 'NEWPC', 'machinenumber': '3015'}, headers={'X-API-Key': collector_key}) assert r.status_code == 200 assert len(_links(machine_3015.assetid)) == 1 + + +def test_machinelink_alerts_are_off_by_default(client, db, collector_key, + computer_assettype, + machine_3015, monkeypatch): + """Several part markers legitimately share a machine number (0613, 0615, + WJPRT), so on this site a handover alert would fire on correct data. The + link, the archive and the collector warning all still happen - only the + sending is gated, and it ships off.""" + import shopdb.api as shopdbapi + + alerts = [] + monkeypatch.setattr(shopdbapi, 'send_alert', + lambda subj, html, **k: alerts.append(subj)) + + client.post('/api/collector/computers', + json={'hostname': 'OLDPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + _go_quiet(db, 'OLDPC') + r = client.post('/api/collector/computers', + json={'hostname': 'NEWPC', 'machinenumber': '3015'}, + headers={'X-API-Key': collector_key}) + + assert alerts == [] + assert len(_links(machine_3015.assetid)) == 1 + assert any('taken over' in w for w in r.get_json()['data']['warnings'])