xcp-ng / xcp-ng-xapi-plugins

XCP-ng's specific XAPI plugins
GNU Affero General Public License v3.0
6 stars 9 forks source link

fix an incorrect behavior around the lock #30

Closed nraynaud closed 2 years ago

nraynaud commented 2 years ago
stormi commented 2 years ago

For your next commit --amend:

We formalized our commit message conventions at https://xcp-ng.org/docs/develprocess.html#commit-message-conventions

stormi commented 2 years ago

I'm sharing here the results of my "virtual" tests (I did not execute anything, I just followed the source code. Hopefully I did it right), as I did on mattermost.

Not giving up without trying to get the lock definitely seems the right move to me and should indeed solve the main issue.

image

In addition to the undefined attribute discussed above, I noted an unexpected change:

There's also a change that probably does not have any functional impact, but could probably be simplified:

Wescoeur commented 2 years ago
* Ideally, we'd not wait for 10s here, as there's no reason to wait for the lock when an update is already in progress.

Not easy, because we take the lock before reading the state in this commit. And if we read before, we can have the bad behavior fixed by this same commit: the lock content can be invalid if the host has been rebooted.

stormi commented 2 years ago

If the host has rebooted, the contents is stale indeed but it won't matter as we will be able to get the lock.

Wescoeur commented 2 years ago

True, we use a non-blocking flag. :+1: We can execute a trylock, and then check the state, and then we can wait or raise.

benjamreis commented 2 years ago

Closing this: it has been solved in https://github.com/xcp-ng/xcp-ng-xapi-plugins/pull/31