api: Adding support for 'api extensions' #121
Conversation
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
8eaf092
to
a54d5c9
Compare
|
/canonical/self-hosted-runners/run-workflows a54d5c9 |
|
Overall LGTM, I feel much better about this feature being part of the microcluster rather than standalone implementation in microovn. |
|
@mkalcok exactly. The dependency upgrade shouldn't be too problematic, there will be only one thing to change in h.OnBootstrap = ovn.Bootstrapwould become h.PreBootstrap = ovn.Bootstrapor h.PostBootstrap = ovn.BootstrapNot entirely sure, but I'm more confident about the |
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
a54d5c9
to
b30bb71
Compare
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
b30bb71
to
4a8e57d
Compare
gabrielmougard
changed the title
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
4a8e57d
to
899b41d
Compare
|
@mkalcok updated |
|
/canonical/self-hosted-runners/run-workflows 899b41d |
|
@mkalcok the TLS system test is failing here. I don't quite understand why.. Do you have an idea (I must admit I running out of ideas)? |
|
@gabrielmougard Originally I thought it's pretty simple case of bad bash syntax that's missing a space between expression and closing then i realized that you didn't change any tests and the error is coming from the |
| @@ -29,7 +29,7 @@ func regenerateCaPut(s *state.State, r *http.Request) response.Response { | |||
| responseData := types.NewRegenerateCaResponse() | |||
|
|
|||
| // Check that this is the initial node that received the request and recreate new CA certificate | |||
| if !client.IsForwardedRequest(r) { | |||
| if !client.IsNotification(r) { | |||
There was a problem hiding this comment.
@gabrielmougard So I checked and the problem is not with BATS. Command microovn certificates regenerate-ca indeed does not work. It fails with
Error: command failed: failed to generate new CA: Put "http://control.socket/1.0/ca": context deadline exceeded
as the CI shows.
I also noticed that it absolutely obliterates the CPU 😆. So perhaps this change from IsForwardedRequest to IsNotification is not working as expected.
The way this command is supposed to work is that the original node that receives the request from client, forwards the request to the rest of the nodes in the cluster. This condition is then used to distinguish whether the node received direct request from client (and should forward requests to all other cluster members), or it's the forwarded message and the node should just do its own thing.
The high CPU consumption suggests to me that even the servers that received the forwarded message then try to forward it again to everyone else, creating kind of a death spiral.
But that's just a guess. I didn't dive too deep into it.
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
899b41d
to
eaf8922
Compare
|
@mkalcok can we re run the tests please? |
|
/canonical/self-hosted-runners/run-workflows eaf8922 |
|
@gabrielmougard I wonder about the upgrade test failure. Is it possible that changes to |
|
@mkalcok that is very possible. We introduced a change in the SQL extension mechanism in MicroCluster canonical/microcluster#94 @masnax do you know how this would work with MicroOVN custom SQL updates? |
|
Hmm..I tried slapping this piece of code after the snap upgrade because it was enough previously when the upgrade required microovn db schema upgrade. However it doesn't seem that the cluster recovers withing 30 seconds. You probably know more about the microcluster schema upgrades than I do. Could you try manually
In any case, even after it gets resolved, since this change will introduce schema upgrade, we won't be able to backport it to Tangentially (also cc @fnordahl) If this becomes part of the
|
This is a consequence of the fact that the introduction of API extensions is itself a schema extension. To give an example, imagine 3 nodes.
So you get into a situation where all 3 nodes are waiting for each other. After 30s the loop repeats so So because the schema update that introduces API updates is part of the same update that increments the number of API updates, the update process takes at least 30s in this case. |
|
Thanks for the detailed explanation @masnax. I believe we've gotten ourselves into a sticky situation. I believe this comes from microovn/microovn/ovn/start.go Lines 47 to 51 in 2524d48 and the sbConnect is just an empty string. What I find interesting though is that the function that's supposed to fetch the sbConnectmicroovn/microovn/ovn/environment.go Line 71 in 2524d48 is connecting to the database, but it's not failing. It's just returning empty strings. |
Crap, this was my bad. I forgot to set Actually microcluster should probably set that for each internal schema update since any external table we don't know about could be referencing any of our's. |
|
@gabrielmougard I have 2 PRs up in microcluster, canonical/microcluster#123 and canonical/microcluster#122, which should fix the issues detected here. |
|
@masnax thanks! I'll have a look |
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
eaf8922
to
226bb44
Compare
Signed-off-by: Gabriel Mougard <[email protected]>
Signed-off-by: Gabriel Mougard <[email protected]> microovn/cmd/microovnd: Pass the MicroOVN extensions map to the MicroCluster initialization process. Signed-off-by: Gabriel Mougard <[email protected]>
gabrielmougard
force-pushed
the
feat/microovn-extensions
branch
from
c910da2
to
fdfc65c
Compare
|
@mkalcok I just ran the full |
|
Thanks @masnax for your MicroCluster PRs ! |
|
/canonical/self-hosted-runners/run-workflows fdfc65c |
|
@gabrielmougard reason for the failing upgrade tests seems to be that the cluster needs a bit more time to converge after internal schema upgrade. Adding here microovn/tests/test_helper/bats/upgrade.bats Lines 31 to 33 in 3f0c609 should solve the issue. |
Signed-off-by: Gabriel Mougard <[email protected]>
|
@mkalcok can you re-run the CI? I added the |
|
/canonical/self-hosted-runners/run-workflows e7c67e4 |
@masnax seamless and painless upgrades are very important to our users, and as @mkalcok already pointed out we rely on microcluster to manage upgrade of the payload. Our highest priority for this release is to make the upgrade process bullet proof, ensuring minimal data path downtime and keeping the end user informed (ref #130). As far as I understand this PR effectively makes the microcluster unavailable for an extended period of time, without any means of informing the user of why or what to do. This does not come across as great UX and conflicts with our main goal for this release, can the schema/extension migration process be improved in microcluster? |
|
@fnordahl Sorry about that, I'm not sure what's happening with the upgrade process here. There was an issue earlier with the upgrade process taking a long time, but this was fixed (by this pr) Running an upgrade locally appears instantaneous on my end, though I haven't tried it with MicroOVN's testsuite personally. I'll give it a run and let you know my findings. |
That's excellent, thank you for looking into it! FWIW; I did a manual test just before posting the previous comment, and the cluster appeared unresponsive until all nodes were upgraded. I think that is our main issue because we really need the cluster and its CLI to be responsive throughout the upgrade process to guide our users. |
In this case it is absolutely necessary for a cluster member to enter a very restricted state if it encounters an upgrade that changes its database schema. This is because we can't risk applying a change to the schema until we are sure that all cluster members expect the same upgrade. If any one system applies the upgrade blindly, then very abruptly all running systems will become unable to properly read from or write to the database. An additional risk is the possibility of conflicting upgrades occurring on different cluster members. The upgrade process works as follows: after encountering a new schema upgrade, that cluster member enters a waiting state which restricts access to the database and API until it receives a notification from the final cluster member to receive the upgrade. Only at this point is the upgrade actually applied, and the database and API become open for regular access. Until this point, any non-upgraded cluster member will continue to function. One added benefit here is that since schema upgrades are non-backwards compatible, we maintain the freedom to revert the upgrade until it is applied on the final system, since nothing will have been committed until that point. I can think of these ways to keep a user informed of the upgrade process and overall cluster status going forward:
As for the slowness, I've narrowed it down to two components:
|
|
Thank you for the detailed insights @masnax. What would be a risks/downsides of keeping the older version of |
|
@mkalcok Well, if we want to go in that direction, I don't see a clean way for a node in the cluster to know what features are enabled in a MicroOVN node. On the MicroCloud side (client querying the MicroOVN service), we could try our luck and call an API endpoint that might or might not be present in MicroOVN to expose MicroOVN's API extensions... If the endpoint exposing the extensions is there, that's great and we can read the extensions and proceed (or not) with our logic in MicroCloud. If it turns out that de deployed MicroOVN is too old and doesn't have such an endpoint (which will return in an I agree with @masnax, there are a couple of improvements we can work on to make the UX better during the upgrade. But ultimately, I don't think there is a way around upgrading the schema (and bumping the version of MicroCluster) without introducing these very risky consistency issues that could even corrupt the MicroOVN networking setup.. |
|
We would welcome an improved schema conversion process. If it is possible to get that done before our release, great, if not we need to be creative and think tactical if you want this included in the release. |
|
@gabrielmougard my suggestion/question was more towards whether it's possible to implement the API feature without the schema change. Looking at this PR, there's no real change to the schema, it just changed because you bumped the Would it be possible to add this API without bumping the |
|
Naively, I would say yes it is of course possible. Adding a new API endpoint in MicroOVN for exposing its API extensions will suffice. We'll then have to find a creative way in the client side in Microcloud to ensure everything is correct on all the MicroOVN nodes. I let @masnax correct me if I say something wrong. |
|
What role does the upgraded |
|
RE merge conflicts: microovn/tests/test_helper/setup_teardown/upgrade.bash Lines 58 to 60 in ce15d25 (btw, it's sufficient to run it in single container. No need to run it in every container ) |
|
@mkalcok Just like the schema upgrade, the set of API extension must be something all cluster members need to agree on. It is effectively a record of the behaviour of the API. If any one cluster member unilaterally changes its API, then cluster-wide behaviour will be inconsistent and possibly broken. For that to work, we need to record the API extensions of all cluster members in the global database so any one member can compare what set it expects to what all other cluster members (even currently offline ones) expect, and be instantly aware if a change happens on another system. This means a change to the schema. So I don't think there is a way to add such a feature without reimplementing a mechanism that works in a similar way to the schema upgrade mechanism, because the API would be (and currently is) unstable when a single system runs So maybe it's best to say that the real feature here is the addition and utilisation of a way to coordinate non-backwards compatible upgrades across the cluster. It would be great to know what your requirements are for such a feature. Would the steps I outlined above be sufficient for moving this forward? That is:
|
This is actually dangerous because of the bug I mentioned earlier:
While some members might report all cluster members are Ideally, there should always be a check after cluster formation that reports all nodes are no longer in a FWIW, if the upgrade occurs after all systems are fully reachable and set up, the total down time is only about a second more than the total down time for a restart of all systems. |
I see, thank you. I didn't fully realized how it affects the cluster list.
We are trying to facilitate hassle-free upgrade of the underlying OVN cluster which, coincidentally, also involves schema upgrade of a clustered database 😆. We hold back the schema upgrade until every node in the cluster expects same version of the database. We don't want to leave the user blind during this process. For example, when 2/3 nodes in the cluster are upgraded, running This lets user know that schema upgrade from You touched on the issue of inconsistent APIs, in this case we deal with it here by assuming that anything that returns I think that if the current error message
That would be something we'd be happy with |
|
@mkalcok That all sounds reasonable to me, thank you :) It shouldn't be hard to incorporate some cluster database elements into that One concern I have is about the cluster error message. There may be very many cluster members so reporting a list would require including some data structure in the error metadata. Our principle has been to keep the error messages as simple strings for the most part without additional metadata that needs to be checked for and parsed. This is to prevent every error returned to the CLI needing to be checked for various types of metadata, and then transformed into a human-readable representation. This isn't a hard rule or anything, but as an alternative do you think it would suffice to simply report a summary in the error message rather than include all cluster member statuses, of which there may be very many? As in simply report something like this: And then more detailed information can be available through the aforementioned Connected to all of this, one thing we arrived at internally is that microcluster doesn't understand the difference between a restart or a reload of the daemon, so that the underlying OVN service can continue to run while the daemon is reloaded for a snap refresh. LXD follows this approach for snap refreshes to ensure instances remain online during a database upgrade, since the actual instances shouldn't care about that. I'm unsure if MicroOVN has some internal reloading mechanism for snap refreshes? Perhaps we can formalize this in microcluster so that the daemon can detect if the intention is to simply reload, and if so we won't necessarily run the This would mean that during a microcluster-level schema upgrade, the underlying OVN service can continue to run. I'm not sure how relevant this will be to the next MicroOVN version since it appears to me that you are also performing some upgrades of OVN itself, so I'm not sure if that entails downtime anyway. |
This sounds very reasonable to me. General error message can be terse and instruct user to run Regarding the refresh vs restart: MicroOVN does not implement any |
Pending resolution of discussion in canonical#121. Signed-off-by: Frode Nordahl <[email protected]>
Pre-requisite PR: canonical/microcluster#86
We'll need a system to centralize the extensions (understand optional features that other MicroCluster-backed services might use) of different MicroCluster-based services.
We propose to centralize the extensions in the MicroCluster database, where a service like MicroOVN in this case, writes its OVN related extensions when being bootstrapped.
This will be needed by #113 and canonical/microcloud#245 to check that the deployed MicroOVN used by MicroCloud, supports custom ip encapsulation for an OVN Geneve tunnel.