Keep a host with no primary MAC loadable, and self-heal it - #1234
Merged
Conversation
Port of working-1.6's 1cd7446, plus the tests that commit did not ship. `$databaseFieldClassRelationships` entries may carry an optional 4th element -- a filter on the joined table. One exists in the whole codebase, and it is load-bearing: 'MACAddressAssociation' => ['hostID', 'id', 'primac', ['primary' => 1]] so `$host->get('primac')` is the PRIMARY MAC rather than whichever row came back first. buildQuery() emitted that filter into $whereArrayAnd, and a WHERE predicate on the right-hand table of a LEFT OUTER JOIN is not a filter -- it is an INNER JOIN written the long way. Rows with nothing to join to are dropped, so a host with no hmPrimary='1' row stopped existing: new Host($id)->isValid() -> false HostManager->find(['id' => $id]) -> 0 objects GET /fog/host/$id -> 404 on a row sitting in the table -- un-loadable, un-editable, un-deletable. Verified against a 2079-host database where 1953 hosts have no primary MAC. buildQuery() recurses, so the same predicate reached every class whose relationship chain passes through Host: Task, SnapinJob, SnapinTask, ImagingLog, NodeFailure, UserTracking, LocationAssociation and SiteHostAssociation. A task belonging to such a host was invisible to TaskManager, which is the half that turns a display bug into an operational one -- and the reason Site::hostIDsForSites() had to read siteHostAssoc directly in #1233. That direct read is now belt and braces rather than the only thing holding. Moving the filter into the JOIN ON clause fixes every caller at once; the $whereInfo closure it was the sole user of goes with it. Second half: Host::save() now promotes the first remaining approved (non-pending) MAC when nothing is flagged primary, so an update that replaces the MAC set cannot strand the host again. It leaves a pending MAC pending -- approving an unapproved MAC as a side effect of an unrelated save would be a worse bug than the one being fixed -- and does nothing when a primary already exists. Two tests, because the two arms catch different regressions: - relationship-filter-in-join.test.php reads the emitted SQL, needs no database, and so is the arm that gates CI. Also added to working-1.6, which has carried the fix untested since June. - macless-host-reachable.test.php drives real rows through a real schema -- five host shapes, load, find, a transitive TaskManager lookup and all four self-heal cases -- and SKIPs where there is no database. Both were written failing and each gate mutation-verified. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of working-1.6's
1cd7446f6, plus the tests that commit did not ship.The bug
$databaseFieldClassRelationshipsentries may carry an optional 4th element — a filter on the joined table. Exactly one exists in the whole codebase, and it is load-bearing:so that
$host->get('primac')is the host's primary MAC rather than whichever row came back first.buildQuery()emitted that filter into$whereArrayAnd— and a WHERE predicate on the right-hand table of a LEFT OUTER JOIN is not a filter, it is an INNER JOIN written the long way. Rows with nothing to join to are dropped by the WHERE, so a host with nohmPrimary='1'row stopped existing:on a row sitting in the table. Un-loadable, un-editable, un-deletable. Verified against a 2079-host database in which 1953 hosts have no primary MAC.
buildQuery()recurses, so the same predicate reached every class whose relationship chain passes throughHost:Task,SnapinJob,SnapinTask,ImagingLog,NodeFailure,UserTracking,LocationAssociation,SiteHostAssociation. A task belonging to such a host was invisible toTaskManager— the half that turns a display bug into an operational one, and the reasonSite::hostIDsForSites()had to readsiteHostAssocdirectly in #1233. That direct read is now belt and braces rather than the only thing holding.How a host loses its primary MAC: an update that replaces the MAC set, a primary MAC deleted, a MAC left pending, or a host created through the API or a CSV import with no MAC at all.
The fix
FOGController::buildQuery()— the relationship filter is emitted into the JOINONclause instead ofWHERE. One change, every caller. The$whereInfoclosure it was the sole user of goes with it.Host::save()— promotes the first remaining approved (non-pending) MAC when nothing is flagged primary, so an update that replaces the MAC set cannot strand the host again. It leaves a pending MAC pending (approving an unapproved MAC as a side effect of an unrelated save would be a worse bug than the one being fixed) and does nothing when a primary already exists.Tests
Two arms, because they catch different regressions. Both were written failing, and every gate was mutation-verified.
tests/relationship-filter-in-join.test.phptests/macless-host-reachable.test.phpload(),find(), a transitiveTaskManagerlookup and all four self-heal casessh tests/run-all.sh— 19 passed, 0 failed.Also on working-1.6
relationship-filter-in-join.test.phpis being added there too: that branch has carried the fix since June with no test.