Open Stack

On 08/13/2014 06:05 PM, Sylvain Bauza wrote:
> 
> Le 13/08/2014 12:21, Sylvain Bauza a écrit :
>>
>> Le 12/08/2014 22:06, Sylvain Bauza a écrit :
>>>
>>> Le 12/08/2014 18:54, Nikola Đipanov a écrit :
>>>> On 08/12/2014 04:49 PM, Sylvain Bauza wrote:
>>>>> (sorry for reposting, missed 2 links...)
>>>>>
>>>>> Hi Nikola,
>>>>>
>>>>> Le 12/08/2014 12:21, Nikola Đipanov a écrit :
>>>>>> Hey Nova-istas,
>>>>>>
>>>>>> While I was hacking on [1] I was considering how to approach the fact
>>>>>> that we now need to track one more thing (NUMA node utilization)
>>>>>> in our
>>>>>> resources. I went with - "I'll add it to compute nodes table"
>>>>>> thinking
>>>>>> it's a fundamental enough property of a compute host that it
>>>>>> deserves to
>>>>>> be there, although I was considering  Extensible Resource Tracker
>>>>>> at one
>>>>>> point (ERT from now on - see [2]) but looking at the code - it did
>>>>>> not
>>>>>> seem to provide anything I desperately needed, so I went with
>>>>>> keeping it
>>>>>> simple.
>>>>>>
>>>>>> So fast-forward a few days, and I caught myself solving a problem
>>>>>> that I
>>>>>> kept thinking ERT should have solved - but apparently hasn't, and I
>>>>>> think it is fundamentally a broken design without it - so I'd really
>>>>>> like to see it re-visited.
>>>>>>
>>>>>> The problem can be described by the following lemma (if you take
>>>>>> 'lemma'
>>>>>> to mean 'a sentence I came up with just now' :)):
>>>>>>
>>>>>> """
>>>>>> Due to the way scheduling works in Nova (roughly: pick a host
>>>>>> based on
>>>>>> stale(ish) data, rely on claims to trigger a re-schedule), _same
>>>>>> exact_
>>>>>> information that scheduling service used when making a placement
>>>>>> decision, needs to be available to the compute service when
>>>>>> testing the
>>>>>> placement.
>>>>>> """
>>>>>>
>>>>>> This is not the case right now, and the ERT does not propose any
>>>>>> way to
>>>>>> solve it - (see how I hacked around needing to be able to get
>>>>>> extra_specs when making claims in [3], without hammering the DB). The
>>>>>> result will be that any resource that we add and needs user supplied
>>>>>> info for scheduling an instance against it, will need a buggy
>>>>>> re-implementation of gathering all the bits from the request that
>>>>>> scheduler sees, to be able to work properly.
>>>>> Well, ERT does provide a plugin mechanism for testing resources at the
>>>>> claim level. This is the plugin responsibility to implement a test()
>>>>> method [2.1] which will be called when test_claim() [2.2]
>>>>>
>>>>> So, provided this method is implemented, a local host check can be
>>>>> done
>>>>> based on the host's view of resources.
>>>>>
>>>>>
>>>> Yes - the problem is there is no clear API to get all the needed
>>>> bits to
>>>> do so - especially the user supplied one from image and flavors.
>>>> On top of that, in current implementation we only pass a hand-wavy
>>>> 'usage' blob in. This makes anyone wanting to use this in conjunction
>>>> with some of the user supplied bits roll their own
>>>> 'extract_data_from_instance_metadata_flavor_image' or similar which is
>>>> horrible and also likely bad for performance.
>>>
>>> I see your concern where there is no interface for user-facing
>>> resources like flavor or image metadata.
>>> I also think indeed that the big 'usage' blob is not a good choice
>>> for long-term vision.
>>>
>>> That said, I don't think as we say in French to throw the bath
>>> water... ie. the problem is with the RT, not the ERT (apart the
>>> mention of third-party API that you noted - I'll go to it later below)
>>>>>> This is obviously a bigger concern when we want to allow users to
>>>>>> pass
>>>>>> data (through image or flavor) that can affect scheduling, but
>>>>>> still a
>>>>>> huge concern IMHO.
>>>>> And here is where I agree with you : at the moment, ResourceTracker
>>>>> (and
>>>>> consequently Extensible RT) only provides the view of the resources
>>>>> the
>>>>> host is knowing (see my point above) and possibly some other resources
>>>>> are missing.
>>>>> So, whatever your choice of going with or without ERT, your patch [3]
>>>>> still deserves it if we want not to lookup DB each time a claim goes.
>>>>>
>>>>>
>>>>>> As I see that there are already BPs proposing to use this IMHO broken
>>>>>> ERT ([4] for example), which will surely add to the proliferation of
>>>>>> code that hacks around these design shortcomings in what is already a
>>>>>> messy, but also crucial (for perf as well as features) bit of Nova
>>>>>> code.
>>>>> Two distinct implementations of that spec (ie. instances and flavors)
>>>>> have been proposed [2.3] [2.4] so reviews are welcome. If you see the
>>>>> test() method, it's no-op thing for both plugins. I'm open to comments
>>>>> because I have the stated problem : how can we define a limit on
>>>>> just a
>>>>> counter of instances and flavors ?
>>>>>
>>>> Will look at these - but none of them seem to hit the issue I am
>>>> complaining about, and that is that it will need to consider other
>>>> request data for claims, not only data available for on instances.
>>>>
>>>> Also - the fact that you don't implement test() in flavor ones tells me
>>>> that the implementation is indeed racy (but it is racy atm as well) and
>>>> two requests can indeed race for the same host, and since no claims are
>>>> done, both can succeed. This is I believe (at least in case of single
>>>> flavor hosts) unlikely to happen in practice, but you get the idea.
>>>
>>> Agreed, these 2 patches probably require another iteration, in
>>> particular how we make sure that it won't be racy. So I need another
>>> run to think about what to test() for these 2 examples.
>>> Another patch has to be done for aggregates, but it's still WIP so
>>> not mentioned here.
>>>
>>> Anyway, as discussed during today's meeting, these 2 patches will not
>>> be based on ERT because of the risk it goes for Juno so let's scope
>>> them out of this thread.
>>>
>>>
>>>>>
>>>>>> I propose to revert [2] ASAP since it is still fresh, and see how
>>>>>> we can
>>>>>> come up with a cleaner design.
>>>>>>
>>>>>> Would like to hear opinions on this, before I propose the patch tho!
>>>>> IMHO, I think the problem is more likely that the regular RT misses
>>>>> some
>>>>> information for each host so it requires to handle it on a
>>>>> case-by-case
>>>>> basis, but I don't think ERT either increases complexity or creates
>>>>> another issue.
>>>>>
>>>> RT does not miss info about the host, but about the particular request
>>>> which we have to fish out of different places like image_metadata
>>>> extra_specs etc, yet - it can't really work without them. This is
>>>> definitely a RT issue that is not specific to ERT.
>>>
>>> +1, I agree with you, that's an API issue for RT : how do we pass out
>>> user-defined metrics ?
>>> I still need to figure out which kind of usecases are requiring such
>>> examples, albeit the NUMA usecase of course.
>>>
>>
>> After a night and morning of thinking about this problem, I'm writing
>> down some ideas that could help fixing the problem (although I don't
>> think it's a long-term scenario, rather a workaround) :
>>
>> Considering you need the metadata of the flavor asked by user when
>> claiming a resource :
>> 1. Compute Manager knows request_spec when building an instance, so it
>> could pass out request_spec as paramater (optional or not) to
>> rt.instance_claim() in the claim context
>> 2. As ERT does, when claiming, it calls the ERT plugin .test() method
>> so with request_spec as extra parameter
>> 3. On the ERT side, a plugin is responsible for calling (and caching)
>> all flavors metadata, or even only the key you need to compare
>> 4. Scheduler knows flavors metadata thanks to ERT plugin and blueprint
>> isolate-scheduler-db so it makes decisions on the same subset as the RT
>> 5. When claiming in ERT plugin test(), it extracts the flavor_id,
>> looks at the internal in-memory representation of flavors to get the
>> metadata, and calls the NUMA _test_numa_topology() method for checking
>> wrt this flavor metadata
>>
>>
>> The idea is that so filters and claims are doing separate decisions on
>> the same source of knowledge, here being ERT.
>>
>> -Sylvain
>>
>>
> 
> Just made a draft over my thoughts here :
> https://review.openstack.org/113936
> 

I have commented on the review itself in more detail, but in short -
this solution won't work for me, and thus in the general case as well, as

1) we need a way to persist the request data (and not make it a horrible
performance bottleneck), as caching won't cut it.

2) We also need a way to use it in every periodic task run.

None of this is only ERT problem, they are problems of the RT itself,
but as I have said in the thread already - adding a plugin API on top of
something that is broken is just asking for all sorts of trouble.

Still leaning towards revert really - anything else would be short
sighted. I would be happy to help come up with a baseline of things to
fix before we can add it back, to avoid deferring it needlesly due to
scope creep.

N.

> -S
>>
>>
>>>>
>>>> However, I still see several issues with the current ERT
>>>> implementation,
>>>> but the one I am getting at here, and why I think we should revert it,
>>>> is that we are building a 3rd party plugin API that is tightly coupled
>>>> to an obviously flawed internal API (RT and Claims).
>>>>
>>>> We have no policy AFIK about what guarantees we provide to 3rd party
>>>> plugins, but if we are going to be breaking them all the time, or in
>>>> this case providing very limited usefulness - I see little value in the
>>>> current implementation of ERT, and see issues with it staying, which
>>>> means future work will be based on it.
>>>>
>>>> N.
>>>
>>> *This* is to me IMHO the biggest problem with ERT : if we say that we
>>> externalize an API for 3rd-party plugins, we need to make sure that
>>> everything is handled.
>>>
>>> That said, instead of reverting the whole patch, could we just
>>> consider to only accept changes that wouldn't require user-facing
>>> metrics ?
>>> The existing VCPU implementation still sounds good to me, so we can
>>> just consider a clear communication on what is acceptable and what
>>> not (ie. a docstring in the plugin API or so., plus -2/-1 reviews if
>>> needed)
>>>
>>> -Sylvain
>>>
>>>>> Thanks,
>>>>> -Sylvain
>>>>>
>>>>>> Thanks all,
>>>>>>
>>>>>> Nikola
>>>>>>
>>>>>> [1]
>>>>>> https://blueprints.launchpad.net/nova/+spec/virt-driver-numa-placement
>>>>>>
>>>>>> [2] https://review.openstack.org/#/c/109643/
>>>>>> [3] https://review.openstack.org/#/c/111782/
>>>>>> [4] https://review.openstack.org/#/c/89893
>>>>>>
>>>>>> _______________________________________________
>>>>>> OpenStack-dev mailing list
>>>>>> OpenStack-dev at lists.openstack.org
>>>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>>> [2.1]
>>>>> https://github.com/openstack/nova/blob/master/nova/compute/resources/__init__.py#L75
>>>>>
>>>>>
>>>>> [2.2]
>>>>> https://github.com/openstack/nova/blob/master/nova/compute/claims.py#L134
>>>>>
>>>>> [2.3] https://review.openstack.org/112578
>>>>> [2.4] https://review.openstack.org/113373
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> OpenStack-dev mailing list
>>>>> OpenStack-dev at lists.openstack.org
>>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>>
>>>> _______________________________________________
>>>> OpenStack-dev mailing list
>>>> OpenStack-dev at lists.openstack.org
>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>>
>>> _______________________________________________
>>> OpenStack-dev mailing list
>>> OpenStack-dev at lists.openstack.org
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev