Open Stack

On 11/10/2015 11:18 AM, Dmitry Tantsur wrote:
> On 11/10/2015 06:08 PM, Ben Nemec wrote:
>> On 11/10/2015 10:28 AM, John Trowbridge wrote:
>>>
>>>
>>> On 11/10/2015 10:43 AM, Ben Nemec wrote:
>>>> On 11/10/2015 05:26 AM, John Trowbridge wrote:
>>>>>
>>>>>
>>>>> On 11/09/2015 07:44 AM, Dmitry Tantsur wrote:
>>>>>> Hi OOO'ers, hopefully the subject caught your attentions :)
>>>>>>
>>>>>> Currently, tripleoclient exposes several commands in "openstack
>>>>>> baremetal" and "openstack baremetal introspection" namespaces belonging
>>>>>> to ironic and ironic-inspector accordingly. TL;DR of this email is to
>>>>>> deprecate them and move to TripleO-specific namespaces. Read on to know
>>>>>> why.
>>>>>>
>>>>>> Problem
>>>>>> =======
>>>>>>
>>>>>> I realized that we're doing a wrong thing when people started asking me
>>>>>> why "baremetal introspection start" and "baremetal introspection bulk
>>>>>> start" behave so differently (the former is from ironic-inspector, the
>>>>>> latter is from tripleoclient). The problem with TripleO commands is that
>>>>>> they're highly opinionated workflows commands, but there's no way a user
>>>>>> can distinguish them from general-purpose ironic/ironic-inspector
>>>>>> commands. The way some of them work is not generic enough ("baremetal
>>>>>> import"), or uses different defaults from an upstream project
>>>>>> ("configure boot"), or does something completely unacceptable upstream
>>>>>> (e.g. the way "introspection bulk start" deals with node states).
>>>>>>
>>>>>> So, here are commands that tripleoclient exposes with my comments:
>>>>>>
>>>>>> 1. baremetal instackenv validate
>>>>>>
>>>>>>   This command assumes there's an "baremetal instackenv" object, while
>>>>>> instackenv is a tripleo-specific file format.
>>>>>>
>>>>>> 2. baremetal import
>>>>>>
>>>>>>   This command supports a limited subset of ironic drivers and driver
>>>>>> properties, only those known to os-cloud-config.
>>>>>>
>>>>>> 3. baremetal introspection bulk start
>>>>>>
>>>>>>   This command does several bad (IMO) things:
>>>>>>   a. Messes with ironic node states
>>>>>>   b. Operates implicitly on all nodes (in a wrong state)
>>>>>>   c. Defaults to polling
>>>>>>
>>>>>
>>>>> I have considered this whole command as a bug for a while now. I
>>>>> understand what we were trying to do and why, but it is pretty bad to
>>>>> hijack another project's namespace with a command that would get a firm
>>>>> -2 there.
>>>>>
>>>>>> 4. baremetal show capabilities
>>>>>>
>>>>>>   This is the only commands that is generic enough and could actually
>>>>>> make it to ironicclient itself.
>>>>>>
>>>>>> 5. baremetal introspection bulk status
>>>>>>
>>>>>>   See "bulk start" above.
>>>>>>
>>>>>> 6. baremetal configure ready state
>>>>>>
>>>>>>   First of all, this and the next command use "baremetal configure"
>>>>>> prefix. I would not promise we'll never start using it in ironic,
>>>>>> breaking the whole TripleO.
>>>>>>
>>>>>>   Seconds, it's actually DELL-specific.
>>>>>>
>>>>>> 7. baremetal configure boot
>>>>>>
>>>>>>   This one is nearly ok, but it defaults to local boot, which is not an
>>>>>> upstream default. Default values for images may not work outside of
>>>>>> TripleO as well.
>>>>>>
>>>>>> Proposal
>>>>>> ========
>>>>>>
>>>>>> As we already have "openstack undercloud" and "openstack overcloud"
>>>>>> prefixes for TripleO, I suggest we move these commands under "openstack
>>>>>> overcloud nodes" namespace. So we end up with:
>>>>>>
>>>>>>   overcloud nodes import
>>>>>>   overcloud nodes configure ready state --drac
>>>>>>   overcloud nodes configure boot
>>>>>>
>>>>>> As you see, I require an explicit --drac argument for "ready state"
>>>>>> command. As to the remaining commands:
>>>>>>
>>>>>> 1. baremetal introspection status --all
>>>>>>
>>>>>>    This is fine to move to inspector-client, as inspector knows which
>>>>>> nodes are/were on introspection. We'll need a new API though.
>>>>>>
>>>>>> 2. baremetal show capabilities
>>>>>>
>>>>>>    We'll have this or similar command in ironic, hopefully this cycle.
>>>>>>
>>>>>> 3. overcloud nodes introspect --poll --allow-available
>>>>>>
>>>>>>    I believe that we need to make 2 things explicit in this replacement
>>>>>> for "introspection bulk status": polling and operating on "available"
>>>>>> nodes.
>>>>>
>>>>> I am not totally convinced that we gain a huge amount by hiding the
>>>>> state manipulation in this command. We need to move that logic to
>>>>> tripleo-common anyways, so I think it is worth considering splitting it
>>>>> from the introspect command.
>>>>>
>>>>> Dmitry and I discussed briefly at summit having the ability to pass a
>>>>> list of nodes to the inspector client for introspection as well. So if
>>>>> we separated out the bulk state manipulation bit, we could just use that.
>>>>>
>>>>> I get that this is going in the opposite direction of the original
>>>>> intention of lowering the amount of commands needed to get a functional
>>>>> deployment. However, I think that goal is better solved elsewhere
>>>>> (tripleo.sh, some ansible playbooks, etc.). Instead it would be nice if
>>>>> the tripleoclient was more transparent.
>>>>
>>>> -2.  This is exactly the thing that got us to a place where our GUI was
>>>> unusable.  Business logic (and state management around Ironic node
>>>> inspection is just that) has to live in the API so all consumers can
>>>> take advantage of it.  Otherwise everyone has to reimplement it
>>>> themselves and anything but the developer-used CLI interfaces (like
>>>> tripleo.sh) fall behind, and we end up right back where we are today.
>>>>
>>>> It's not simply about reducing the number of commands to deploy a cloud,
>>>> it's also (and more importantly) about codifying the process behind an
>>>> API that can be used by the CLI, GUI, and any other future clients
>>>> someone wants to write.
>>>
>>> I think you misunderstand my suggestion. The basic premise is that we
>>> have to move the state manipulation logic into tripleo-common along with
>>> the inspection logic, so that is consumable via the REST API. At that
>>> point, what is gained by having it mixed together?
>>>
>>> It is the mixing of these two bits of logic that I object to. We are
>>> consuming two totally separate service APIs (ironic and inspector) in
>>> the same command in a way that is not transparent to end users.
>>>
>>> More so, it limits our ability to take better implementations from
>>> upstream as they become available. The ability to do bulk inspection
>>> will definitely land before the ability to do bulk state manipulation in
>>> Ironic.
>>>
>>> So we are in total agreement on the fact that the process needs to be
>>> codified behind an API. My suggestion is just that it does not need to
>>> be a SINGLE API call for these two very different operations. Otherwise,
>>> why not just have `openstack baremetal go` that just does all the steps
>>> we currently have in the baremetal namespace. It is this part (the
>>> sequencing of commands or API calls into a workflow with no logic) that
>>> I think is better handled by
>>> tripleo.sh/ansible/insert_workflow_management_tool_of_choice.
>>
>> Actually, I would love an openstack baremetal go command. :-)
>>
>> In fact, I'm proposing that we coalesce some of our existing baremetal
>> commands because (for example) it's silly for us to register nodes and
>> not configure them to be able to boot:
>> https://review.openstack.org/#/c/238192/  I'm not sure this fits the
>> same pattern because introspection is optional, but I wouldn't hate the
>> idea of a register/configure/introspect call that just took your list of
>> nodes as input and ended with all of the nodes ready for use.
>>
>> In any case, my main argument is that we aren't doing two very different
>> operations here.  The user is asking us to introspect all of their
>> nodes, and we're doing it.  Yes, from an Ironic/Inspector perspective
>> there's a bunch of stuff going on here, but from a user perspective
>> there's one: introspection of all nodes.  The user shouldn't have to
>> care about the node state song and dance that we're doing to make it
>> happen IMNSHO.
> 
> Well, users will care when they figure out that our shortcuts break 
> promises made by ironic. E.g. introspection nodes in available state 
> means that we can start introspection on nodes that nova is going to 
> schedule on.

That's not actually how it works.  The client sets them to manageable
before starting introspection, so Nova's not going to schedule to them
while introspection is going on.  Basically we're doing the alternative
mentioned on the enroll spec (which we had to because enroll didn't
exist yet).  It may not be ideal, but it's what Ironic supported at the
time this was all written.

> 
> Actually, all our scripts assume that they'll always be run sequentially 
> as written in the documentation. It is not true, as we know now.

Right, because people are bad at following documentation. :-)

I'm proposing that we not let them run things out of order by codifying
that order as much as possible.

> 
> Finally, we'll soon have no choice. Latest versions of Ironic API make 
> ENROLL the default state, so you have to care about it.

That's an extra step for users not doing introspection, which is kind of
unfortunate, but I don't think it changes my view that our bulk
introspection should handle the enroll->manageable transition, start
introspection, then handle the manageable->available transition
automatically.

> 
>>
>> This is getting kind of off-topic for the CLI namespace question though,
>> so we might want to move this discussion to an API spec.
>>
>>>
>>>>>
>>>>> Thanks Dmitry for starting this discussion.
>>>>>