Comment 6 for bug 1725211

Hi Ruby!

Thanks for the proofreading!
TL;DR: let's have a (separate?) spec

> When providing links to code, it would be good to provide a link to an 'instance' of the file, not latest, cuz that could change and then your line numbers won't make any sense.
True, next time I'm using some release tag, sorry.

>> wrt "This however has the downside of "locking" the node in the inspecting state for periods indefinitely long[2]."
> I could be wrong, but I took a quick look at [2] and it only does a write lock when updating the node; is that what you mean by 'indefinitely long' or am I missing something? Specifically, i think it is this line [Z] that does a write lock.
I'm rather referring to: the node provisioning state need not ever change. I might be wrong here; my assumption is based just on reading through this inspector module.

> Your description mentions
>> 'abort': "upon the abort request, the node is kept in the same inspect-wait state till either".
> Why, if the user is requesting an abort, do you have to keep it in inspect-wait til ... ??
The inspector response is async on the abort call. I though it would be better not to assume inspector was able to handle the request right away (as would have been implied by having advanced the node provision state to inspect-failed). I wanted the inspect-wait instead to be indifferent to the abort call while still relaying the abort request to inspector to handle. In the end of the day, inspector would propagate the inspection failure back through the periodic sync. Should the timeout kick in, ironic would set inspect-failed state on its own. But I see this might be rather inspector-specific (and complicated). I haven't thought about aborting vendors' out-of-band inspection call-flow with regards to the state machine.

> (Again, I don't know much about the inspect stuff). Also, maybe the 'abort' feature should be a separate RFE? Well, there is a proposal for that, so are you just commenting in this, about how the abort might work with the new state, or is the intention for the abort to be implemented here?
Originally I thought these were separate issues and inspect-wait might have been beneficial on its own, as a separate RFE (mind the timeout protection and the "state-locking" issue). I'm not against merging with the spec in question[A] or having a separate one, what ever makes it smoother to adopt ;)

> Have you thought about whether we need an API version to hide/reveal this new state?
I haven't but I believe it should be exposed

> - regardless of the API version, I'm assuming that you want to put a node into this new 'inspect wait' state
hmmm... I thought one would rather put the node into inspecting state, the state machine would go to the inspect-wait on its own?

> - if so, let's say we add this new state in version 1.40. If the user is using version 1.39 and the node is in 'inspect wait' state, do we show that, or do we show 'inspect'? I think we probably need to show the node in inspect, not 'inspect wait', since that's the whole idea about our wonderful API versions.
I agree, not very nice IMO but I guess no other way, just show the "fake" state.

> If you can rewrite/answer all my questions in your one description, then I am fine with no spec. I don't want people combing through these comments, trying to figure out how this is meant to work.
I agree. Let's have a separate spec.

Thanks again!
milan

[A] https://review.openstack.org/#/c/482867/