SAP: Introduce external scheduler filter

[auhlig]

SAP: Introduce an external scheduler filter

This allows us to rely on an external http service to do the filtering,
on a potentially more complex rule-set.
Instead of filtering each host one-by-one, we want to filter them in
a single requests and therefor override the BaseFilter.filter_all

[joker-at-work]

implement using BaseFilter

I think it should be documented (at least in the commit message), why we do not use the usual approach of BaseHostFilter.

[fwiesel]

Could you also rewrite the history to a single commit please?

[fwiesel]

Sorry for the piece-meal: Could you also please prefix the commit title with "SAP: "?

[fwiesel]

And maybe a slightly more elaborate commit-message as @joker-at-work already asked for. I'd suggest:

SAP: Introduce an external scheduler filter

This allows us to rely on an external http service to do the filtering,
on a potentially more complex rule-set.
Instead of filtering each host one-by-one, we want to filter them in
a single requests and therefor override the `BaseFilter.filter_all`

[fwiesel]

Maybe better: spec_obj.obj_to_primitive()

[auhlig]

Thanks!
I did it explicitly now for vcpus, memory.
Deriving this from the object results in some bad json which might make it easier here, but harder on the receiver.

[fwiesel]

The problem is, now you have only a subset of the fields, and no version-negotiation.
You can still pick out just the fields you want on the receiver side on from oslo_versionedobjects json.
I agree, it is harder to do that from there, but I am a big fan of working with the tools/libraries as they are intended.

[fwiesel]

As to the work on the other side: Since zed, there is a class-method to_json_schema, which will
drop you the schema, which in turn could be used to generate the full object.

Here would be one of such tools: https://github.com/omissis/go-jsonschema

I agree, doing all that is a bit much to start with, but using here spec_obj.obj_to_primitive() opens the path for that, and the usage of other serializers (protobuf?). Parsing "manually" the admittedly ugly json for that is in my mind a small price to pay for that.

[fwiesel]

Not that I will block this change because of that.

[fwiesel]

Personally, I still would prefer the oslo_versionedobject serialization, but I am okay with this change.

[auhlig]

Personally, I still would prefer the oslo_versionedobject serialization, but I am okay with this change.

I tried that. However, the nested python structures aren't serialized correctly. Thus I implemented the current solution.

[joker-at-work]

afaics, this comment doesn't contain any content. What's the intent?

[joker-at-work]

oslo_log not being part of this project (Nova) should go to another import block (the one where requests is)

[joker-at-work]

I think this does nothing. I tried with this code:

def a():
  try:
      for i in range(7):
        if i == 3:
          raise Exception
        yield i
  except Exception:
    return list(range(7))

breakpoint()

$ python3 /tmp/bla.py 
--Return--
> /tmp/bla.py(10)<module>()->None
-> breakpoint()
(Pdb) p a()
<generator object a at 0x7e701c04d4d0>
(Pdb) p list(a())
[0, 1, 2]

Since a function containing yield, not matter if it's called or not, is a Generator, it doesn't have a proper return-value - afaik.

Any return is broken, also the first one allowing to skip the filter:

def a():
  return ["early"]
  try:
      for i in range(7):
        if i == 3:
          raise Exception
        yield i
  except Exception:
    return list(range(7))

breakpoint()

$ python3 /tmp/bla.py 
--Return--
> /tmp/bla.py(11)<module>()->None
-> breakpoint()
(Pdb) p list(a())
[]
(Pdb) 

Am I holding it wrong? Shouldn't the test for "the filter returns everything if it's disabled" have found this?

[joker-at-work]

Since you have all the hosts in a list already and are not reading from the request anymore, you might as well return a list instead of yielding, i.e. return [obj for obj in filter_obj_list if obj.host in valid_hosts]

[joker-at-work]

I think this comment doesn't add anything helpful as the next line starts with LOG.error and thus easily conveys the same information.

[joker-at-work]

This is missing at least 3 tests:
a) we're disabled
b) we're enabled, but the api is unreachable (mock_post.side_effect = requests.RequestException() or something)
c) we're enabled, the api returns an empty reply
d) we're enabled, the api returns some error code (500? 503?) (is this this same as b? we could make sure we catch a broad-enough exception from requests)

[joker-at-work]

Other test classes, mainly because they run multiple tests, instantiate their filter in __init__() already.