🐛 Proceed to power off after deprovisioning #1312

honza · 2023-07-14T06:51:13Z

Follow up to #1176

zhouhao3 · 2023-07-26T01:46:48Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

elfosardo · 2023-07-26T07:41:28Z

/lgtm

zhouhao3 · 2023-07-26T08:23:28Z

/lgtm

controllers/metal3.io/host_state_machine.go

dtantsur · 2023-08-09T11:23:05Z

/lgtm

dtantsur · 2023-08-14T12:32:37Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

kashifest · 2023-08-23T07:47:49Z

Can this change be reflected back also in the state diagram https://github.com/metal3-io/baremetal-operator/blob/main/docs/BaremetalHost_ProvisioningState.png ? We are planning for a minor release of BMO soon, can we take this bug fix in before that @honza @zaneb

honza · 2023-08-25T08:40:05Z

Good catch! I updated the diagram, please have a look.

controllers/metal3.io/host_state_machine.go

zaneb · 2023-08-28T00:37:50Z

docs/BaremetalHost_ProvisioningState.dot

@@ -51,11 +51,13 @@ digraph BaremetalHost {
    Provisioned -> Deprovisioning [label="!DeletionTimestamp.IsZero()"]

    ExternallyProvisioned [shape=doublecircle]
-    ExternallyProvisioned -> Deleting [label="!DeletionTimestamp.IsZero()"]
+    ExternallyProvisioned -> PoweringOffBeforeDelete [label="!DeletionTimestamp.IsZero()"]


People have complained about all the Deleting3 [shape=point] stuff making it unclear that the host goes into the Deleting state no matter where it was when deleted.
I wonder if this is the point where it finally becomes completely untenable to hide this info for the sake of saving a few lines on the diagram. How bad does it look if we include all the edges?

Something like this:

🤔 that's... not outstanding, but maybe not so bad that it's worth the ongoing confusion to avoid it. WDYT?
We don't actually go to PoweringOffBeforeDelete from Unmanaged or Registering, do we?

We don't actually go to PoweringOffBeforeDelete from Unmanaged or Registering, do we?

We actually do, but the (inevitable) error is handled, so I guess not technically a bug even though it is a bit messy for the user.

OK, I pushed the diagram changes. I think it's reasonably clear.

zaneb

/approve

zaneb · 2023-09-14T02:17:26Z

docs/BaremetalHost_ProvisioningState.dot

-    Registering -> Deleting2 [label="!DeletionTimestamp.IsZero()"]
-
-    Deleting2 [shape=point]
+    Registering -> PoweringOffBeforeDelete [label="!DeletionTimestamp.IsZero()"]

    ExternallyProvisioned -> Inspecting [label="!externallyProvisioned && NeedsHardwareInspection()"]


I just discovered that if we change all these ones with && or || to e.g.

Suggested change

ExternallyProvisioned -> Inspecting [label="!externallyProvisioned && NeedsHardwareInspection()"]

ExternallyProvisioned -> Inspecting [label="!externallyProvisioned &&\nNeedsHardwareInspection()"]

it makes everything a bit more compact, and this one in particular fixes that diabolical intersection of the two edges at the top right.

metal3-io-bot · 2023-09-14T02:17:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [zaneb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zaneb · 2023-09-14T21:18:59Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

dtantsur · 2023-09-15T11:46:16Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

dtantsur · 2023-09-15T14:29:33Z

docs/BaremetalHost_ProvisioningState.dot

-    Unmanaged -> Deleting1 [label="!DeletionTimestamp.IsZero()"]
-
-    Deleting1 [shape=point]
+    Unmanaged -> PoweringOffBeforeDelete [label="!DeletionTimestamp.IsZero()"]


This make me worried. Unmanaged hosts cannot be powered off. Isn't it the cause of the issue that we've recently discussed?

We already have a follow up for this here #1356

dtantsur · 2023-09-15T14:29:40Z

docs/BaremetalHost_ProvisioningState.dot

-    Registering -> Deleting2 [label="!DeletionTimestamp.IsZero()"]
-
-    Deleting2 [shape=point]
+    Registering -> PoweringOffBeforeDelete [label="!DeletionTimestamp.IsZero()"]


We already have a follow up for this here #1356

dtantsur · 2023-09-15T15:41:12Z

/lgtm

honza changed the title ~~Proceed to power off after deprovisioning~~ 🐛 Proceed to power off after deprovisioning Jul 14, 2023

metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 14, 2023

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2023

dtantsur reviewed Aug 7, 2023

View reviewed changes

controllers/metal3.io/host_state_machine.go Outdated Show resolved Hide resolved

honza force-pushed the power-off-after-deprov branch from cf3a4f5 to 0c960c4 Compare August 7, 2023 13:22

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2023

zaneb reviewed Aug 28, 2023

View reviewed changes

honza added 2 commits September 12, 2023 18:29

Proceed to power off after deprovisioning

7f08b91

Add PoweringOffBeforeDelete state to diagram

5a01d08

honza force-pushed the power-off-after-deprov branch from bb29507 to 5a01d08 Compare September 12, 2023 21:37

Simplify diagram by removing fake deleting states

abb1d1c

honza mentioned this pull request Sep 14, 2023

🐛 Prevent hosts from getting into infinite power-off loops #1356

Merged

metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 14, 2023

zaneb reviewed Sep 14, 2023

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2023

honza force-pushed the power-off-after-deprov branch from abb1d1c to 8cf8e29 Compare September 14, 2023 19:17

metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 14, 2023

honza force-pushed the power-off-after-deprov branch from 8cf8e29 to 2359fba Compare September 14, 2023 19:19

metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 14, 2023

honza force-pushed the power-off-after-deprov branch from 2359fba to abb1d1c Compare September 14, 2023 19:20

metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 14, 2023

docs: Add line breaks to long diagram labels

7e9dbc5

dtantsur reviewed Sep 15, 2023

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2023

metal3-io-bot merged commit ca4f58f into metal3-io:main Sep 15, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Proceed to power off after deprovisioning #1312

🐛 Proceed to power off after deprovisioning #1312

honza commented Jul 14, 2023

zhouhao3 commented Jul 26, 2023

elfosardo commented Jul 26, 2023

zhouhao3 commented Jul 26, 2023

dtantsur commented Aug 9, 2023

dtantsur commented Aug 14, 2023

kashifest commented Aug 23, 2023

honza commented Aug 25, 2023

zaneb Aug 28, 2023

honza Sep 12, 2023

zaneb Sep 12, 2023

zaneb Sep 12, 2023

honza Sep 14, 2023

zaneb left a comment

zaneb Sep 14, 2023

honza Sep 14, 2023

metal3-io-bot commented Sep 14, 2023

zaneb commented Sep 14, 2023

dtantsur commented Sep 15, 2023

dtantsur Sep 15, 2023

honza Sep 15, 2023

dtantsur Sep 15, 2023

honza Sep 15, 2023

dtantsur commented Sep 15, 2023

	ExternallyProvisioned -> Inspecting [label="!externallyProvisioned && NeedsHardwareInspection()"]
	ExternallyProvisioned -> Inspecting [label="!externallyProvisioned &&\nNeedsHardwareInspection()"]

🐛 Proceed to power off after deprovisioning #1312

🐛 Proceed to power off after deprovisioning #1312

Conversation

honza commented Jul 14, 2023

zhouhao3 commented Jul 26, 2023

elfosardo commented Jul 26, 2023

zhouhao3 commented Jul 26, 2023

dtantsur commented Aug 9, 2023

dtantsur commented Aug 14, 2023

kashifest commented Aug 23, 2023

honza commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metal3-io-bot commented Sep 14, 2023

zaneb commented Sep 14, 2023

dtantsur commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtantsur commented Sep 15, 2023