fix race between device remove and other ops #736
Conversation
|
Can one of the admins verify this patch? |
|
@centos-ci ok to test |
raghavendra-talur
force-pushed
the
fix-db-inconsistency
branch
from
64dec70
to
504b492
Compare
|
@lpabon @obnoxxx @MohamedAshiqrh This patch is ready for review. I have run unit tests and SmokeTests on the code. |
| v.gidRequested, v.Info.Id) | ||
| deviceEntry.Save(tx) | ||
| return nil | ||
| }) |
| float64(v.Info.Snapshot.Factor), | ||
| v.gidRequested, v.Info.Id) | ||
| err = db.Update(func(tx *bolt.Tx) error { | ||
| deviceEntry, _ := NewDeviceEntryFromId(tx, deviceId) |
There was a problem hiding this comment.
Maybe add a comment explaining why you re-fetch the device here?
| v.gidRequested, v.Info.Id) | ||
| deviceEntry.Save(tx) | ||
| return nil | ||
| }) | ||
|
|
There was a problem hiding this comment.
Now you re-fetched the device entry in the transaction, and changed it, you should re-fetch it outside? Otherwise the next operationon newDeviceEntry will operate on stale data?
| defer func() { | ||
| if e != nil { | ||
| db.Update(func(tx *bolt.Tx) error { | ||
| newDeviceEntry.StorageFree(newBrickEntry.TotalSize()) |
There was a problem hiding this comment.
newDeviceEntry is stale here, right?
| if err != nil { | ||
| logger.Err(err) | ||
| return err | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| logger.LogError("Replace brick operation succeeded but failed to update db") |
There was a problem hiding this comment.
message is not fully making sense. please reword...
There was a problem hiding this comment.
removed this line
| if err != nil { | ||
| logger.Err(err) | ||
| return err | ||
| } | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| logger.LogError("Replace brick operation succeeded but failed to update db") | ||
| logger.Err(err) |
There was a problem hiding this comment.
could we combine the two log calls?
| v.removeBrickFromDb(tx, oldBrickEntry) | ||
| err = v.Save(tx) | ||
| reReadVolEntry.BrickAdd(newBrickEntry.Id()) | ||
| reReadVolEntry.removeBrickFromDb(tx, oldBrickEntry) |
There was a problem hiding this comment.
Any possibility of an error on above 2 operations?
Yes, ran test 5 times, got the following list of brickNumbers 8,12,6,13,8 instead of 16. |
It is possible that a device gets more bricks added to it in a parallel volume create/expand operation when we are adding a brick to it as part of device remove opeation. What we save to disk should be the final state which is consequence of both the operations. Signed-off-by: Raghavendra Talur <rtalur@redhat.com>
Signed-off-by: Raghavendra Talur <rtalur@redhat.com>
raghavendra-talur
force-pushed
the
fix-db-inconsistency
branch
2 times, most recently
from
d333461
to
0e640de
Compare
| return err | ||
| } | ||
| newDeviceEntry.StorageFree(newBrickEntry.TotalSize()) | ||
| newDeviceEntry.Save(tx) |
There was a problem hiding this comment.
StorageFree returns error. shouldn't we catch it?
There was a problem hiding this comment.
Gosh, Save returns error, not StorageFree ... ;-)
There was a problem hiding this comment.
I am not entirely sure about this. I guess it might be ok in this case. Also, we are not fully consistent with this across the code base. Inclined to merge it like this and we can sort it out later in full generality..
|
LGTM |
It is possible that a device gets more bricks added to it in a parallel
volume create/expand operation when we are adding a brick to it as part
of device remove opeation. What we save to disk should be the final
state which is consequence of both the operations.
Signed-off-by: Raghavendra Talur rtalur@redhat.com