Open Stack

On 11/10/2015 10:40 AM, Dmitry Tantsur wrote:
> On 11/10/2015 05:18 PM, Ben Nemec wrote:
>> +1 to moving anything we can into Ironic itself.
>>
>> I do want to note that if we rename anything, we can't just rip and
>> replace.  We have users of the current commands, and we need to
>> deprecate those to give people a chance to move to the new ones.
> 
> Definitely. I think I used word deprecation somewhere in this long letter :)

Cool, just wanted to make sure we were all on the same page. :-)

> 
>>
>> A few more thoughts inline.
>>
>> On 11/09/2015 06: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.
>>
>> True, although I feel like an "import from JSON" feature would not be
>> inappropriate for inclusion in Ironic.  I can't believe that we're the
>> only ones who would be interested in mass importing nodes from a very
>> common format like this.
> 
> I would warmly welcome such command, but it should not require patching 
> os-cloud-config every time we add a new driver or driver property.

Yeah, I wonder if we should just drop the os-cloud-config abstraction
and have the JSON contain the actual node parameters.  A bunch of the
drivers have driver-specific configuration options anyway, so it's not
like you can just change the driver name without altering the other bits
of the configuration anyway.

The only drawback to that is if a driver changed the name of a
parameter, but if this lived in Ironic you could just add a name mapping
to the import code at the same time, whereas having it in
os-cloud-config you'd still break, but there's an extra layer of
indirection to getting it fixed.

The more I think about it the more I like it.  Not sure I'm going to
have time to follow up though. :-/

> 
>>
>>>
>>> 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)
>>
>> I thought that was fixed?  It used to try to introspect nodes that were
>> in an invalid state (like active), but it shouldn't anymore.
> 
> It introspects nodes in "available" state, which is a rude violation of 
> the ironic state machine ;)
> 
>>
>> Unless your objection is that it introspects things in an available
>> state, which I think has to do with the state Ironic puts (or used to
>> put) nodes in after registration.  In any case, this one likely requires
>> some more discussion over how it should work.
> 
> Well, if we upgrade baremetal API version we used from 1.6 (essentially 
> Kilo) to Liberty one, nodes would appear in a new ENROLL state. I've 
> started a patch for it, but I don't have time to finish. Anyone is free 
> to overtake: https://review.openstack.org/#/c/235158/

This seems like the right thing to do all around.  It allows us to
introspect newly registered nodes without breaking the Ironic state
machine.  I've subscribed to the review at least, will see if I have
time to do anything with it.

> 
>>
>>>    c. Defaults to polling
>>>
>>> 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.
>>
>> Well, as I understand it we don't intend for it to be Dell-specific,
>> it's just that the Dell implementation is the only one that has been
>> done so far.
>>
>> That said, since I think this is just layering some TripleO-specific
>> logic on top of the vendor-specific calls Ironic provides I agree that
>> it probably doesn't belong in the baremetal namespace.
>>
>>>
>>> 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.
>>
>> +1
>>
>>>
>>> 2. baremetal show capabilities
>>>
>>>     We'll have this or similar command in ironic, hopefully this cycle.
>>
>> \o/
>>
>>>
>>> 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.
>>
>> The only thing is that because this does the state manipulation, polling
>> is essentially the only way it can work, although moving this to the API
>> will change that.  Like I said above, I think this one is going to
>> require some ongoing discussion as part of the API work.
>>
>>>
>>> 4. overcloud nodes import --dry-run
>>>
>>>     could be a replacement for "baremetal instackenv validate".
>>
>> I'm wondering if we should even bother with --dry-run, and instead just
>> run this validation as part of the import in the first place.  That's
>> essentially what --dry-run would end up looking like anyway.  You'd look
>> at the file, do the validation, then depending on whether --dry-run was
>> specified either register the nodes or not.  We could provide a --force
>> in case someone really, really wants to import a node list that won't
>> work for some reason, and that way we don't have to worry about people
>> forgetting to run the validation.
> 
> +1
> 
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
> 
> 
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>