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

Proposed by Sandy Walsh
Status: Merged
Merge reported by: Sandy Walsh
Merged at revision: not available
Proposed branch: lp:~sandy-walsh/nova/zones3
Merge into: lp:~hudson-openstack/nova/trunk
Prerequisite: lp:~sandy-walsh/nova/zones2
Diff against target: 0 lines
To merge this branch: bzr merge lp:~sandy-walsh/nova/zones3
Reviewer Review Type Date Requested Status
Rick Harris (community) Approve
Matt Dietz (community) Approve
Joshua McKenty Pending
Todd Willey Pending
Review via email: mp+54743@code.launchpad.net

This proposal supersedes a proposal from 2011-03-08.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Todd Willey (xtoddx) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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?

Revision history for this message
Todd Willey (xtoddx) wrote : Posted in a previous version of this proposal

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?

Revision history for this message
Eric Day (eday) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Todd Willey (xtoddx) wrote : Posted in a previous version of this proposal

I'm on board with that style of flag.

review: Approve
Revision history for this message
Joshua McKenty (joshua-mckenty) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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!

Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Rick Harris (rconradharris) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Matt Dietz (cerberus) wrote : Posted in a previous version of this proposal

I don't see anything glaring ;-)

lgtm. Just waiting on Rick

review: Approve
Revision history for this message
Sandy Walsh (sandy-walsh) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Rick Harris (rconradharris) wrote : Posted in a previous version of this proposal

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

review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Matt Dietz (cerberus) wrote :

Wrong button

review: Approve
Revision history for this message
Rick Harris (rconradharris) wrote :

lgtm

review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Preview Diff

Empty