Merge lp:~sandy-walsh/nova/zones3 into lp:~hudson-openstack/nova/trunk

[Sandy Walsh]

This branch adds a Fanout (broadcast) Queue to all services. This lets anyone talk to all services of a particular type without having to iterate through each.

Compute, Volume and Network now derive from nova.SchedulerDependentManager, which gives them the ability to update all the Scheduler nodes of their capabilities (via the fanout queue).

These capabilities are stored in each Scheduler Zone Manager and available via the Scheduler.API

The OS API '/zones/info' call now returns the aggregated (min, max) values of each of the service capabilities: http://paste.openstack.org/show/815/

To integration test this, simply fire up another scheduler (or more), add some capabilities to Compute, Network or Volume via the services update_service_capabilities() call and watch the events appear in each Scheduler node.

[Todd Willey (xtoddx)]

I think this looks pretty good.

Is there any information (RST docs) regarding what meaningful values for capabilities are?

Also, in some flags we are using x=y,a=b syntax for multi-valued flags (defualt_log_levels, etc). Perhaps we can keep the same format? Long term perhaps we could make a new DEFINE_dict or some-such that would parse those into a dict.

It looks like the only information that is being fanned out is the service capabilities, but would it make sense to keep abilities in the database pointing to the services themselves? In other words, why are service capabilities ephemeral with the services starting and stopping? If we keep them in the db we can modify them while running and modify the capabilities with nova-manage. This of course doesn't make sense for things like changing the hypervisor for compute since that would always require a restart, but would it be practical for other capabilities?

review: Needs Information

[Sandy Walsh (sandy-walsh)]

Todd, thanks for the feedback.

I think the meaningful values for caps will come from the distributed scheduler effort. That branch really needs to drive what is required from the services.

RE flags: do you mean "x=1,x=2,x=3,y=9,y=8,y=7" vs. "x:1,2,3;y:9,8,7;" ?

RE storing the capabilities: yes, storing it in the db is a possibility. Since many capabilities are not static (disk remaining, bandwidth usage, cpu, etc) I had concerns about the frequency of the updates and the added load on the db. But this could use the same periodic storage scheme as the fanout mechanism.

[Sandy Walsh (sandy-walsh)]

Oh, perhaps the multi value keys where "x:1;2;3, y;9;8;7" ? That would be an easy change. Perhaps move the parser into utils.py as well?

[Todd Willey (xtoddx)]

From nova/flags.py:

flags.DEFINE_list('default_log_levels',
                  ['amqplib=WARN',
                   'sqlalchemy=WARN',
                   'boto=WARN',
                   'eventlet.wsgi.server=WARN'],
                  'list of logger=LEVEL pairs')

I think that is a good pattern to follow. Your argument ends up looking like

--zone_capabilities=hypervisor=xenserver,os=linux

You'll end up with the list

['hypervisor=xenserver', 'os=linux']

You're definitely treating it as a list by splitting and iterating over it, so DEFINE_list seems reasonable. Based on your comments it looks like you might specify multiple values for any individual capability, in which case using a comma would break the flag list, maybe using a colon as the item separator would be reasonable?

[Eric Day (eday)]

8: Ahh! How did nova.db get into nova.api? I must have missed this in the last branch. It would be nice to abstract this in nova/scheduler or a nova/zones/ package.

Should SchedulerDependentManager just be in nova/manager.py?

As for using a DB to store this data (Todd's comment), we need to move away from compute/network/volume hosts writing to the DB directly for security concerns. We should be using the msg queue for all communications/data and let the scheduler verify the source once we have a mechanism to do so.

[Sandy Walsh (sandy-walsh)]

eric, agree on moving SchedulerDependentManager into nova/manager.py and abstracting out the db call into the scheduler api. Will do.

Todd, colon is bad for URI's. I'll mess around the the List option.

[Sandy Walsh (sandy-walsh)]

Todd, how about x=1;2;3, y=9;8;7 .. that way we can use DEFINE_list and have multi-values?

[Sandy Walsh (sandy-walsh)]

All fixed up as requested ... thanks again for the feedback!

[Todd Willey (xtoddx)]

I'm on board with that style of flag.

review: Approve

[Joshua McKenty (joshua-mckenty)]

Typo on 79 (missing i).
Seems like service_name should be a class property rather than an instance property, any reason we can't do it this way? Then the super __init__ call doesn't need an extra arg.
I think TopicConsumer was the last of my code from the first nova-hacking weekend - I'm glad to see it go.

Log message on 231 seems wrong - is it actually "writing" at that point, or just initing?
I like how you've dropped _call_scheduler out of the API class - but do we even NEED that class any more? The rest of the class methods could be dropped down to functions as well.

467 - can we call this zone_capabilities? Caps seems like a pseudonym for "limits".
Looks like we need a simple test of the fanout in test_rpc.py.

Doesn't seem like there's any way to remove a host from service capabilities once it's been added - is this necessary?

review: Needs Information

[Sandy Walsh (sandy-walsh)]

Joshua, great feedback ... thanks for that. All good suggestions. I'll get on them.

Hmm, I was going to add something to decay the hosts after time. I may have forgotten that, lemme see.

If not, I can easily add it to Zones4 (coming soon).

If you'd like to see the direction this is heading have a look here (WIP):
https://code.launchpad.net/~sandy-walsh/nova/zones4/+merge/53726

Cheers!

[Sandy Walsh (sandy-walsh)]

Changes made ... the fanout test should be an integration test vs. a unit test. We'll need to revisit that once we get the integration test framework ironed out.

[Rick Harris (rconradharris)]

Overall this patch looks really good Sandy. Nice tests, and some really
helpful comments in there (as usual).

Generally speaking, I think a few whitespaces cleanups would make a make this
patch even better (in particular lining up arg-lists). Not a huge deal,
obviously, but worth mentioning.

In terms of design and implementation, I think it looks good. This matches up
with what we whiteboarded (AFAICT), so I'm all for it.

Specifics
=========

> 10 +from nova import log as logging

Imported but not used.

> 35 + for cap in caps:
> 36 + key_values = cap.split('=')
> 37 + zone[key_values[0]] = key_values[1]

Might be slightly easier to read with tuple-unpacking (also, passing 1 to
split in case the value has a '=' in it):

    for cap in caps:
        key, value = cap.split('=', 1)
        zone[key] = value

> 62 + super(ComputeManager, self).__init__(service_name="compute",
> 63 + *args, **kwargs)

Might be better if args aligned:

    super(ComputeManager, self).__init__(service_name="compute",
                                         *args, **kwargs)
> 77 +DEFINE_list('zone_capabilities',
> 78 + ['hypervisor=xenserver;kvm', 'os=linux;windows'],
> 79 + 'Key/Multi-value list representng capabilities of this zone')

Same as above:

    DEFINE_list('zone_capabilities',
                ['hypervisor=xenserver;kvm', 'os=linux;windows'],
                'Key/Multi-value list representng capabilities of this zone')

> 88 +from nova import log as logging

Might be worth using the LOG pattern here to get more specific logging:

    LOG = logging.getLogger('nova.manager')

> 106 + update_service_capabilities is called with non-None values."""
> 107 + def __init__(self, host=None, db_driver=None, service_name="undefined"):

Spacing. Might be better as:

       update_service_capabilities is called with non-None values.
    """

    def __init__(self, host=None, db_driver=None, service_name="undefined"):

> 313 + params=dict(service=service))

Couple spaces ---> thataway (to line up with open paren +1)

> 209 + LOG.info(_("Created '%(exchange)s' fanout exchange "
> 210 + "with '%(key)s' routing key"),
> 211 + dict(exchange=self.exchange, key=self.routing_key))

Could use a bit of formatting work:

    LOG.info(_("Created '%(exchange)s' fanout exchange "
               "with '%(key)s' routing key"),
             dict(exchange=self.exchange, key=self.routing_key))

Although, this might be personal preference on my part, so, if that change
doesn't look good to you, please feel free to ignore! :-)

> 638 + svc10_a=(99, 99), svc10_b=(99, 99)))

Spacing, a couple of spaces <---- thattaway.

review: Needs Fixing

[Matt Dietz (cerberus)]

I don't see anything glaring ;-)

lgtm. Just waiting on Rick

review: Approve

[Sandy Walsh (sandy-walsh)]

Thanks for the feedback Rick ... I agree with your suggestions. Doing the code reviews on glance and seeing the work you and jay have been doing might make me change my style :)

Stay tuned.

[Rick Harris (rconradharris)]

Looks great, thanks for the fix-ups, Sandy!

review: Approve

[OpenStack Infra (hudson-openstack)]

No proposals found for merge of lp:~sandy-walsh/nova/zones2 into lp:nova.

[Matt Dietz (cerberus)]

Wrong button

review: Approve

[Rick Harris (rconradharris)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

No proposals found for merge of lp:~sandy-walsh/nova/zones2 into lp:nova.