Add HA support for the Azure storage backend #8464
Conversation
|
@calvn, thanks for adding the @jefferai , looking at git commit history, it appears you were the reviewer for the few |
7de4bc8 to
3b61812
Compare
|
@calvn - it has been a year, any suggestion on how to get this reviewed and merged? Thanks. |
3b61812 to
287dd05
Compare
|
Any news for that? |
| } | ||
| conds.IfMatch = r.etag // CAS mechanism | ||
| } else { | ||
| // IfNoneMatch: "*" = perform the operation iff the resource does not exist |
There was a problem hiding this comment.
Just spotted a small typo.
| // IfNoneMatch: "*" = perform the operation iff the resource does not exist | |
| // IfNoneMatch: "*" = perform the operation if the resource does not exist |
There was a problem hiding this comment.
this is short for "if and only if"
| blob := l.backend.container.NewBlobURL(l.key) | ||
| bProp, err := blob.GetProperties(context.Background(), azblob.BlobAccessConditions{}) | ||
| if err != nil { | ||
| if bProp != nil && bProp.StatusCode() == http.StatusNotFound { |
There was a problem hiding this comment.
I did some testing with this PR and had some trouble at bootstrapping. The absence of a lock is treated as error: The vault instances always yielded messages like lock: attempt lock: write lock: failed to read blob properties for "core/lock":, followed by a 404 error.
It seems that this line here is intended to treat 404s differently than other errors, as these 404s are part of normal operation, as one Vault instance has to create the lock initially (I imagine).
When doing a little bit of debugging, I found that bProp was nil also for 404s though, hence not satisfying this if clause, causing effectively a deadlock with aforementioned log output.
Unfortunately, as of now, I cannot provide a solution, though.
|
@sding3 I just added support for testing in CI using Azurite in Docker (#12057), would you mind merging in main and adding a call to ExerciseHABackend? This won't obviate the need for real testing using Azure, since it's a different implementation, but it would at least give us some protection against regressions and some feedback in CI. Merging in main will probably also fix those failing tests you're seeing. |
Borrowed heavily from gcs_ha.go, and implemented using analogous features from Azure blob storage.
0f5e081 to
7d0f598
Compare
I was previously pointing the two HA backends at two different azurite backends, of course that didn't work. Fixed that by pointing them at the same azurite backend.
| var conds azblob.BlobAccessConditions | ||
|
|
||
| // Read the record | ||
| r, err := l.get(ctx) |
There was a problem hiding this comment.
This seems like a race - someone could acquire a lock in between our get and our update, and then we'd blow it away. I don't know enough Azure to offer good advice here, but I think you should try to eliminate the get if possible, instead relying solely on the conds to protect the update.
There was a problem hiding this comment.
The outcome of the get informs how the access condition should be built. The access condition influences the behavior of the subsequent write.
If the lock file doesn't already exist, the cond will be If-None-Match: *, which means that the file must not exist in order for the subsequent write to succeed.
If the lock file already exists, the cond will be If-Match: {eTag_of_lock_file}, which means that the file must exist and has not been modified for the subsequent write to succeed.
There was a problem hiding this comment.
How about instead of doing the get, you take note of the Etag in the BlockBLobUploadResponse you get back from the blob.Upload call? Then the renews can make use of that for their IfMatch condition, and if you don't have an Etag (because this is your first writeLock call, i.e. it's not a renewal) you use IfNoneMatch:* as before?
As to the timestamp check below, maybe instead of that, use IfUnmodifiedSince in your update?
These two changes would save you a roundtrip and also eliminate my race concern.
There was a problem hiding this comment.
Thinking about it a bit more I don't think my suggestion above quite works. Let me try again.
For the initial case, we want to grab the lock unless there's a lock that has been recently updated. So for that initial case, maybe the only condition needed is IfUnmodifiedSince=(now-ttl). But I don't know how IfUnmodifiedSince behaves if the blob doesn't exist, so maybe we also need If-None-match:*. But the docs say that these conditions are ANDed together whereas we really want an OR, and I don't really understand the table they give describing the combined behaviour here: https://docs.microsoft.com/en-us/rest/api/storageservices/specifying-conditional-headers-for-blob-service-operations.
One option in the initial case if we can't find a way to express the desired condition: first try to update with If-None-Match:*, and if that fails attempt it with IfUnmodifiedSince. Or the other way around, whichever.
For the renew case, we want to grab the lock if we still own it. So there the only condition needed is IfMatch = (etag returned by our Upload). We don't really care about the ttl in this case.
There was a problem hiding this comment.
Thanks for suggesting these alternate methods.
The current implementation does not have a race condition. Further, the current implementation is consistent with the way gcs implements it. gcs is an analogous storage technology to azure blob.
|
I'm sorry that this has been ignored for so long. Thank you hanging in there. This isn't quite a review yet, just some initial thoughts. I'll have to enlist the help of someone who knows Azure better to validate the parts that actually talk to Azure. I regret not having gotten around yet to writing some docs to guide storage backend authors, because I recognize the model you used for this, and I prefer a slightly different one. Back when HA support was being added to the PostgreSQL backend someone submitted a PR much like this one, and I found it a bit hard to understand. I came up with a slightly simpler approach that I think could work here too. The idea is to get rid fo the "watch" goroutine, since it's not really necessary: we can use renewal failure as our mechanism to detect when we've lost the lock, assuming the renewal operation is constrained to not update the lock record when someone else has a valid lock (which should already be the case). I also don't really like the Anyway, I don't want to impose this model on you. You've already done a lot of hard work here, and been extremely patient with us. But if you're amenable have a look at physical/postgresql and see what you think. |
|
@ncabatoff - thanks for pointing me to the Azurite testing set up. It's done well. I like how I could simply run As for your other suggestion about implementing the HA support in a similar fashion as the PostgreSQL backend, my current inkling is that I should approach it in the same manner as how |
This implementation borrows heavily from the gcs HA implementation. The CAS mechanism is realized through Azure storage conditional access headers and ETag.
Partially addresses #4576.