Comment 5 for bug 1725211

Revision history for this message
Ruby Loo (rloo) wrote :

Hi Milan, thx for the RFE.

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.

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.

You're going to need to update our state diagram [Y]. It isn't clear to me what/how you transition from inspect to "inspect wait" back to inspect or "inspect fail". Is it clear to the folks that know how inspecting works?

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 ... ?? (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?

Have you thought about whether we need an API version to hide/reveal this new state?
- regardless of the API version, I'm assuming that you want to put a node into this new 'inspect wait' state
- 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.

In general, I'm good with an 'inspect wait' state; it makes it more consistent with the rest of our states. If your description works for the folks that are more familiar with inspect and for the folks that will be reviewing this code, then it is fine with me.

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.

[Y]: https://docs.openstack.org/ironic/latest/contributor/states.html

[Z] https://github.com/openstack/ironic/blob/7337fefd978b14d8a5e04314565e696e86879f21/ironic/drivers/modules/inspector.py#L185