configure_add() method should get called only once during glance server initialization

[Abhishek Kekane]

The configure_add() method is called each time during image create, update or delete api calls, which leads to a performance issue. Ideally configuration related activities should be done only once during server initialization.
This is applicable to all stores.

[Zhi Yan Liu (lzy-dev)]

Interesting, Abhishek can you explain where you see this wrong behaviour logic in code? thanks. IMO this is an invalid report.

[Abhishek Kekane (abhishek-kekane)]

Hi Zhi Yan Liu,

Consider filesystem store,
get_store_from_scheme() method initializes store object each time create or update image api is triggered.
https://github.com/openstack/glance/blob/master/glance/store/__init__.py#L212.

Each time store object is initialized, it gives a call to configure_add() method (url below).
https://github.com/openstack/glance/blob/master/glance/store/base.py#L55

configure_add() does following things, which need not be repeated every time a call to create or update image api is triggered. Ideally this information should get only ones when the store is initialized, as it will not be changed after server is started.

- checks if CONF.filesystem_store_datadir is specified in glance config, if not raise exception
- checks if CONF.filesystem_store_datadir path exists, if not create the directory at that path.

[Zhi Yan Liu (lzy-dev)]

@Abhishek, you are right. I think think the key at here https://github.com/openstack/glance/blob/master/glance/store/__init__.py#L179 . But I'm thinking your proposal probably is not a good way to go: to cache a store driver instance and reuse it in next calls when upper layer code needed.

We can add one line code easily to saving a driver object for reusing at https://github.com/openstack/glance/blob/master/glance/store/__init__.py#179, but it will make a potential limitation on store drivers, which is all store drivers must be implemented as a stateless class, all driver's interfaces must be implemented as a reentrant function (existing and new dirvers in future). I'm not sure all existing interfaces are all meet this requirement, but I believe this limitation will make driver's interface be implemented more like "functional programing" style but OOP/D. So from this angle I still think this ia an invalid bug since I consider this limitation is overkill, but I agree this is a good point to make us notice.

I think the key to determine whether this behaviour is a real problem in real deplyment is: how long/effort will be spent on store driver's configure() and configure_add() method, so did you see some bottlenecks for hanlde request on existing drivers? in your above case do you think "CONF.filesystem_store_datadir" and "CONF.filesystem_store_datadir" checking will make an unacceptable delay for api request?

If its answer is yes then, IMO we can extract a new method on store driver, and call that in create_stores() only at first create time. And in future calls, like in get_store_from_scheme(), we (driver's __init__()) will not call that again.

if its answer is no then, IMO to make enough notice to reviews, cores specially, and let's pay more thinking on this point when we implement/review new driver patch, we will raise a -1/-2 up if driver's __init__() execution may cause a long dalay.

[Zhi Yan Liu (lzy-dev)]

@Abhishek, there is a critical typo in my above posting sorry, it should be:

But I'm thinking **the** proposal probably is not a good way to go: to cache a store driver instance and reuse it in next calls when upper layer code needed.

[Erno Kuvaja (jokke)]

Looks like this bug is related with Bug: #1315237 that Arnaud is working on. Would it be possible to get some collaboration happening between and possibly hit them both with single strike?

[Zhi Yan Liu (lzy-dev)]

I believe Arnaud's one (bug #1315237) is different than this, since in order to make glance functions work, store driver needs to be configured correctly for each driver instance, so configure_add/configure() always need to be triggered.

And I still think my above comment works for glance/us, btw.

Btw, if it's needed (needs verification/some numbers), probably we can separate RO and RW logic from store driver against different kind of api request, for example we can move the necessary code stuff from configure() to configure_add() which only be used for store.add() operations, then change store.__init__.py and driver base class to only call particular configure function for relevant request: for RW request, we can only call store.configure() instead of both of them.

[Arnaud Legendre (arnaudleg)]

@Abhishek -- Thank you for creating this bug!! I was about to create it. You have my full support to fix it.

The bug that I fixed (bug #1315237) is indeed different: it is specific to Glance startup and do not change the behavior of glance after g-api is started.

@Zhi Yan, this bug is the source of very important performance degradation, especially for stores that have *long* running operations in configure or configure_add.
For example, with the VMware store, for every API call, we currently recreate a soap session which might take a couple of seconds or even more with a very bad network. This is totally unacceptable. Also, based on the fact that we are not reusing the session created, we can hit the vCenter limit of number of sessions opened (since they are not cleaned up correctly at this point), which is also pretty bad... I created the patch to renew the session expecting to work on a patch to keep the session around (which is this bug).

@Zhi Yan "Btw, if it's needed (needs verification/some numbers), probably we can separate RO and RW logic from store driver against different kind of api request, for example we can move the necessary code stuff from configure() to configure_add() which only be used for store.add() operations, then change store.__init__.py"
>> I don't think this will solve the problem. Again, for the VMware store, the session created should be reused for each API call. I don't think having to keep a session reference is something too particular to make it an exception.
From my POV, the problem is not the fact that we are configuring too many time, but more that we should be able to keep the state of the stores once they are created!
I think creating one instance for each store and keep it around will definitely achieve this, but I agree with Zhi, we might face race conditions...

[Arnaud Legendre (arnaudleg)]

Also, I think we should bump the priority of this bug.

[Flavio Percoco (flaper87)]

This bug is now invalid as the glance_store library doesn't do this anymore.