Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug when removing finalizer #22

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Fix bug when removing finalizer #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5125c1
Only use required fields when removing Finalizer
HonakerM Dec 5, 2023
0ae6282
Format changes
HonakerM Dec 5, 2023
9cf38f8
Fix Finalizer tests to correctly adjust
HonakerM Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions oper8/deploy_manager/dry_run_deploy_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,19 @@ def disable(self, resource_definitions):
log.debug2(
"Calling registered finalizer [%s] for [%s]", callback, key
)
callback(content)

# If finalizers have been cleared than delete object
if not self._cluster_content[namespace][kind][api_version][name][
"metadata"
].get("finalizers"):
callback(self._cluster_content[namespace][kind][api_version][name])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be behind the DRY_RUN_CLUSTER_LOCK (or alternately, have the value pulled above and then used here)?


# If finalizers have been cleared and object hasn't already been deleted then
# remove the key
current_obj = (
self._cluster_content.get(namespace, {})
.get(kind, {})
.get(api_version, {})
.get(name, {})
)
if current_obj and not current_obj.get("metadata", {}).get(
"finalizers", []
):
with DRY_RUN_CLUSTER_LOCK:
self._delete_key(namespace, kind, api_version, name)

Expand Down
15 changes: 13 additions & 2 deletions oper8/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,11 @@ def add_finalizer(session: SESSION_TYPE, finalizer: str):

log.debug("Adding finalizer: %s", finalizer)

manifest = copy.deepcopy(session.cr_manifest)
manifest = {
"kind": session.kind,
"apiVersion": session.api_version,
"metadata": copy.deepcopy(session.metadata),
}
manifest["metadata"].setdefault("finalizers", []).append(finalizer)
success, _ = session.deploy_manager.deploy([manifest])

Expand All @@ -243,7 +247,14 @@ def remove_finalizer(session: SESSION_TYPE, finalizer: str):

log.debug("Removing finalizer: %s", finalizer)

manifest = copy.deepcopy(session.cr_manifest)
# Create manifest with only required fields
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, perfect. This will be much simpler

manifest = {
"kind": session.kind,
"apiVersion": session.api_version,
"metadata": copy.deepcopy(session.metadata),
}

# Remove the finalizer
manifest["metadata"]["finalizers"].remove(finalizer)
success, _ = session.deploy_manager.deploy([manifest])

Expand Down
6 changes: 4 additions & 2 deletions oper8/watch_manager/dry_run_watch_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,10 @@ def run_reconcile(self, is_finalizer: bool, resource: dict):
if (
self._resource.get("kind") == resource.get("kind")
and self._resource.get("apiVersion") == resource.get("apiVersion")
and resource_metadata.get("name") == resource_metadata.get("name")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! Those were certainly buggy

and resource_metadata.get("namespace") == resource_metadata.get("namespace")
and resource_metadata.get("name")
== resource.get("metadata", {}).get("name")
and resource_metadata.get("namespace")
== resource.get("metadata", {}).get("namespace")
):
return

Expand Down
6 changes: 3 additions & 3 deletions tests/test_reconcile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@ def check_status(
api_version=cr.apiVersion,
)
assert obj is not None
assert obj.status
assert obj.get("status")

ready_cond = status.get_condition(status.READY_CONDITION, obj.status)
ready_cond = status.get_condition(status.READY_CONDITION, obj["status"])
if ready_reason:
assert ready_cond
assert ready_cond["reason"] == ready_reason.value
else:
assert not ready_cond

update_cond = status.get_condition(status.UPDATING_CONDITION, obj.status)
update_cond = status.get_condition(status.UPDATING_CONDITION, obj["status"])
if updating_reason:
assert update_cond
assert update_cond["reason"] == updating_reason.value
Expand Down