Skip to content

Commit f53e17f

Browse files
syedhaseebahmedsyedhaseebahmed
committed
dropping unused ironicUUID
1 parent eaf68ef commit f53e17f

5 files changed

Lines changed: 82 additions & 38 deletions

File tree

‎components/openstack-sync-operator/crds/baremetal.ironicproject.org_ironicrunbooks.yaml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,9 +167,6 @@ spec:
167167
description: IronicRunbookStatus defines the observed sync state.
168168
type: object
169169
properties:
170-
ironicUUID:
171-
description: Ironic UUID of this runbook.
172-
type: string
173170
syncStatus:
174171
description: SyncStatus indicates the synchronization state with Ironic.
175172
type: string

‎python/openstack-sync/openstack_sync/plugins/ironic/runbooks/client.py‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,30 @@ def probe() -> None:
6767
wait_for_openstack_api("Ironic", probe, retries=retries, delay=delay)
6868

6969

70+
# ---------------------------------------------------------------------------
71+
# Identifiers
72+
# ---------------------------------------------------------------------------
73+
74+
75+
def assigned_uuid(runbook: dict[str, Any], name: str) -> str:
76+
"""Return the UUID Ironic assigned to *runbook*.
77+
78+
Every write goes to the UUID rather than the name. Ironic accepts either in
79+
the path, but a name can be reassigned to another runbook, so a write
80+
addressed to the UUID of the runbook just read cannot be redirected by a
81+
rename that lands afterwards. Which runbook this is was still decided by the
82+
read that resolved the name.
83+
"""
84+
uuid = str(runbook.get("uuid") or "")
85+
if not uuid:
86+
raise ConfigError(
87+
f"Ironic returned runbook {name!r} without a uuid, so it cannot be "
88+
"written to; the response was truncated or the API is not serving "
89+
"runbooks as expected"
90+
)
91+
return uuid
92+
93+
7094
# ---------------------------------------------------------------------------
7195
# Requests
7296
# ---------------------------------------------------------------------------
@@ -145,12 +169,17 @@ def patch_runbook(
145169
return _json_body(response)
146170

147171

148-
def delete_runbook(conn: Any, name: str) -> None:
149-
"""Delete the runbook named *name*, treating an absent one as success."""
172+
def delete_runbook(conn: Any, runbook_uuid: str) -> None:
173+
"""Delete the runbook with UUID *runbook_uuid*, treating an absent one as success.
174+
175+
Addressed by UUID for the same reason as :func:`patch_runbook`, and with more
176+
at stake: the runbook being deleted was chosen from a list snapshot, and the
177+
name it carried there may since belong to a different runbook.
178+
"""
150179
try:
151-
_request(conn, "DELETE", f"{_RUNBOOKS_PATH}/{name}")
180+
_request(conn, "DELETE", f"{_RUNBOOKS_PATH}/{runbook_uuid}")
152181
except openstack_exceptions.NotFoundException:
153-
LOG.info("Runbook %s is already absent from Ironic", name)
182+
LOG.info("Runbook %s is already absent from Ironic", runbook_uuid)
154183

155184

156185
def set_traits(conn: Any, runbook_uuid: str, traits: list[str]) -> None:

‎python/openstack-sync/openstack_sync/plugins/ironic/runbooks/prune.py‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,6 @@
2424
LOG = logging.getLogger(__name__)
2525

2626

27-
def _delete_runbook(conn: Any, name: str) -> None:
28-
LOG.info("Deleting removed Ironic runbook %s", name)
29-
try:
30-
client.delete_runbook(conn, name)
31-
except openstack_exceptions.ConflictException:
32-
LOG.info("Ironic runbook %s is still in use; skipping delete", name)
33-
34-
3527
def prune_removed_runbooks(
3628
conn: Any,
3729
desired_specs: list[dict[str, Any]],
@@ -63,4 +55,8 @@ def prune_removed_runbooks(
6355
if not is_managed_runbook(runbook):
6456
LOG.info("Keeping Ironic runbook %s; it is not operator-owned", name)
6557
continue
66-
_delete_runbook(conn, str(name))
58+
LOG.info("Deleting removed Ironic runbook %s", name)
59+
try:
60+
client.delete_runbook(conn, client.assigned_uuid(runbook, str(name)))
61+
except openstack_exceptions.ConflictException:
62+
LOG.info("Ironic runbook %s is still in use; skipping delete", name)

‎python/openstack-sync/openstack_sync/plugins/ironic/runbooks/reconcile.py‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -154,23 +154,6 @@ def set_field(field: str, value: Any) -> None:
154154
return operations
155155

156156

157-
def _runbook_uuid(runbook: dict[str, Any], name: str) -> str:
158-
"""Return the UUID Ironic assigned to *runbook*.
159-
160-
Every write goes to the UUID rather than the name. Ironic accepts either in
161-
the path, but the UUID is what the runbook keeps across a rename, so a
162-
write can never land on whatever else answers to that name.
163-
"""
164-
uuid = str(runbook.get("uuid") or "")
165-
if not uuid:
166-
raise ConfigError(
167-
f"Ironic returned runbook {name!r} without a uuid, so it cannot be "
168-
"updated; the response was truncated or the API is not serving "
169-
"runbooks as expected"
170-
)
171-
return uuid
172-
173-
174157
def ensure_runbook(conn: Any, spec: dict[str, Any]) -> dict[str, Any]:
175158
"""Create or converge the runbook *spec* describes, and return it."""
176159
name = str(spec["runbookName"])
@@ -201,7 +184,7 @@ def ensure_runbook(conn: Any, spec: dict[str, Any]) -> dict[str, Any]:
201184
name,
202185
", ".join(operation["path"] for operation in operations),
203186
)
204-
return client.patch_runbook(conn, _runbook_uuid(existing, name), operations)
187+
return client.patch_runbook(conn, client.assigned_uuid(existing, name), operations)
205188

206189

207190
# ---------------------------------------------------------------------------
@@ -225,7 +208,7 @@ def reconcile_traits(
225208
sorted(current),
226209
sorted(desired),
227210
)
228-
client.set_traits(conn, _runbook_uuid(runbook, name), desired)
211+
client.set_traits(conn, client.assigned_uuid(runbook, name), desired)
229212
return desired
230213

231214

‎python/openstack-sync/tests/test_ironic_runbooks_prune.py‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import pytest
99
from openstack import exceptions as openstack_exceptions
1010

11+
from openstack_sync.plugins.common import ConfigError
1112
from openstack_sync.plugins.ironic.runbooks import client
1213
from openstack_sync.plugins.ironic.runbooks import markers
1314
from openstack_sync.plugins.ironic.runbooks import prune
@@ -43,7 +44,7 @@ def test_owned_runbook_absent_from_the_desired_set_is_deleted():
4344
_prune(fake, [_spec("CUSTOM_KEEP")])
4445

4546
assert sorted(fake.runbooks) == ["CUSTOM_KEEP"]
46-
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE"]
47+
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE-uuid"]
4748

4849

4950
def test_owned_runbook_on_the_second_page_is_deleted():
@@ -63,7 +64,7 @@ def test_owned_runbook_on_the_second_page_is_deleted():
6364
{"detail": "true", "limit": 1, "marker": "CUSTOM_KEEP-uuid"},
6465
{"detail": "true", "limit": 1, "marker": "CUSTOM_GONE-uuid"},
6566
]
66-
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE"]
67+
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE-uuid"]
6768

6869

6970
def test_runbook_the_operator_does_not_own_is_kept():
@@ -111,7 +112,7 @@ def vanish(path: str, method: str, **kwargs: Any) -> Any:
111112
with mock.patch.object(fake, "request", side_effect=vanish):
112113
_prune(fake, [_spec("CUSTOM_KEEP")])
113114

114-
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE"]
115+
assert fake.calls_for("DELETE") == ["/runbooks/CUSTOM_GONE-uuid"]
115116

116117

117118
def test_a_conflict_leaves_the_runbook_in_place():
@@ -128,6 +129,44 @@ def conflict(path: str, method: str, **kwargs: Any) -> Any:
128129
assert sorted(fake.runbooks) == ["CUSTOM_GONE"]
129130

130131

132+
# ---------------------------------------------------------------------------
133+
# Identifiers
134+
# ---------------------------------------------------------------------------
135+
136+
137+
def test_the_delete_is_addressed_to_the_uuid_not_the_name():
138+
"""Prune chose this runbook from a list snapshot taken earlier.
139+
140+
By the time the DELETE goes out the name may belong to a runbook another CR
141+
has just created or adopted. The UUID from the snapshot cannot be reassigned
142+
that way, so it is what the delete addresses.
143+
"""
144+
fake = FakeBaremetal([_owned("CUSTOM_GONE")])
145+
uuid = fake.uuid_of("CUSTOM_GONE")
146+
147+
_prune(fake, [_spec("CUSTOM_KEEP")])
148+
149+
assert fake.calls_for("DELETE") == [f"/runbooks/{uuid}"]
150+
assert fake.runbooks == {}
151+
152+
153+
def test_a_runbook_with_no_uuid_fails_the_prune_instead_of_deleting():
154+
"""Better a failed prune than a DELETE to /runbooks/None."""
155+
book = _owned("CUSTOM_GONE")
156+
del book["uuid"]
157+
fake = FakeBaremetal([book])
158+
159+
with pytest.raises(ConfigError, match="without a uuid"):
160+
_prune(fake, [_spec("CUSTOM_KEEP")])
161+
162+
assert fake.calls_for("DELETE") == []
163+
164+
165+
# ---------------------------------------------------------------------------
166+
# Failures
167+
# ---------------------------------------------------------------------------
168+
169+
131170
def test_a_failure_other_than_conflict_or_not_found_stops_the_prune():
132171
"""The framework reports a failed prune as a non-zero exit."""
133172
fake = FakeBaremetal([_owned("CUSTOM_GONE")])

0 commit comments

Comments
 (0)