Code review comment for lp:~cbehrens/nova/servers-search

> > Power states belong to the compute layer, and server statuses belong to the
> > OSAPI. I think we should remove any OSAPI status-related code from the
> > power_states module and place it in either api.openstack.common or
> > api.openstack.servers. What do you think?
>
> I had the same debate. I was going to move it to api.openstack.common.. until
> I realized that compute.api needs to send the 'status' to search by to child
> zones. This means that compute.api would end up making a call into
> api.openstack.common to convert a power state to the status to search for..
> and that didn't feel quite right. I think I can solve that differently. I
> can make the OS API pass the 'status' to search for through to compute.api
> (along with 'state' which is the translation of status to power state).
> compute.api will ignore 'status' and use 'state' but it'll pass 'status' on to
> novaclient to talk to zones.
>
> That or I could leave it the way it is. :) Changing it probably makes sense
> so there's clear separation.

Yeah, I really don't want the compute api (the power states module) to depend on concepts defined in the openstack api.

> > Are there future plans to implement changes-since? I can see it is an
> accepted
> > filter, just not respected.
>
> Yeah, good question. I'm already going beyond the scope of the story that was
> given to us for the sprint, and that one seemed a little more difficult.
> Wasn't sure if compute.api should filter that.. or let OS API filter on the
> return to the client. I thought maybe titan would have a better idea of how
> that should be done. ;)

We got it :)

> > I don't see it necessary to list OSAPI-specific filters in the compute api.
> > Can we fix that known_params?
>
> Well, I need a table to map the search parameter to the function to call with
> its arguments. In the initial merge prop, I didn't have this as I used
> getattr(self, '_get_all_by_%s' % opt_name).. and called that function. The
> problem is that the generic 'column' ones need an extra argument. The
> original merge prop had a list of those search options that could be done
> generically. I could change it back... but it'd also require moving all of
> the _get_all_by* calls back outside of 'def get_all' so I could use
> getattr(self, ..) again.
>
> Hm.. did that make sense? We can chat on IRC to clarify if needed.

What bothers me is the inclusion of non-filter query params (like changes-since). If those would just go away I would be more okay with it.

> > I really want to be able to sort by multiple parameters, and I think most
> > people using the API would expect to be able to do that. How hard would it
> be
> > to support this?
>
> Yeah, I hear you. The story I was given says to throw an error if multiple
> are given, so that's the main reason why I didn't code support for it. :) My
> instinct is to say it's rather difficult... but when I really start to think
> about it, I'm not sure it's so bad. I can think about it some more while
> doing the other fixups.

I don't mean to toot my own horn here, but look at glance and see if that approach would work.