Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-400060: Sm feature intersection #6069

Merged

Conversation

Vincent-lau
Copy link
Contributor

There are two parts to this PR:

  1. only declare a feature as a public sm feature when it is advertised by all SMs in the pool
  2. reject a host from joining the pool if it does not have a compatible set of SM features with the current pool

Now I did not create a new class just for SM features as I feel having a decidated class for something that is supposed to be quite a temporary state (as this is only supposed to happen during an upgrade) is an overkill. But I am open to suggestions.

@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch 2 times, most recently from 10e58fa to 5e7eec5 Compare October 22, 2024 13:27
@@ -4994,11 +4994,21 @@ module SM = struct
, "capabilities of the SM plugin, with capability version \
numbers"
)
; ( Changed
, "24.34.0"
Copy link
Contributor Author

@Vincent-lau Vincent-lau Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This probably needs to be changed before actually being merged

@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch 2 times, most recently from 8b0f6a2 to 6bce2b4 Compare October 22, 2024 15:10
ocaml/xapi/xapi_pool.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_pool.ml Outdated Show resolved Hide resolved
@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch 2 times, most recently from 74b67dc to 45e1578 Compare November 5, 2024 15:40
@rdn32
Copy link

rdn32 commented Nov 5, 2024

I had a couple of questions:

  • With the call to Storage_access.on_xapi_start moved from dbsync_master.ml to dbsync_slave.ml, what happens with a single stand-alone host? Does the call happen nonetheless?
  • In update_from_query_result, once a feature is listed as pending on all hosts, then it's just a new feature - not pending any more. So shouldn't something update the record of pending features?

@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch from 45e1578 to e700ae7 Compare November 5, 2024 16:28
@robhoes
Copy link
Member

robhoes commented Nov 5, 2024

With the call to Storage_access.on_xapi_start moved from dbsync_master.ml to dbsync_slave.ml, what happens with a single stand-alone host? Does the call happen nonetheless?

Yes, dbsync_slave is execute on any host when it starts, both coordinator and other hosts. And a standalone host is simply a coordinator of a pool of one host.

In update_from_query_result, once a feature is listed as pending on all hosts, then it's just a new feature - not pending any more. So shouldn't something update the record of pending features?

Yes, I think the way is works is that a host does remove its pending features, but only after it restarts again? That may be alright for the purpose. The DB field is not public.

@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch from bc526a0 to 9e179d6 Compare November 6, 2024 14:26
@Vincent-lau
Copy link
Contributor Author

In update_from_query_result, once a feature is listed as pending on all hosts, then it's just a new feature - not pending any more. So shouldn't something update the record of pending features?

Yes, I think the way is works is that a host does remove its pending features, but only after it restarts again? That may be alright for the purpose. The DB field is not public.

I think that was a valid point, it does not get removed after the feature is moved from pending to non-pending, and gets stored in the db forever. I have added some code to remove it once all hosts know this feature.

@@ -51,6 +51,8 @@ let prototyped_of_field = function
Some "22.26.0"
| "VTPM", "persistence_backend" ->
Some "22.26.0"
| "SM", "host_pending_features" ->
Some "24.36.0-next"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be 24.37.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be changed to 24.37.0 whenever we tag and release 24.37.0

@Vincent-lau Vincent-lau force-pushed the private/shul2/sm-feature-check2 branch from 9455d31 to 2ec0ac6 Compare November 7, 2024 11:13
@Vincent-lau Vincent-lau added this pull request to the merge queue Nov 7, 2024
Merged via the queue into xapi-project:master with commit 670d811 Nov 7, 2024
15 checks passed
`host_pending_features` represents the features that are available on
some of the hosts in the pool, but not all of them. Note the way this
field is initialised in the `SM.create` code means that it will only
contain new features that appear during upgrade. This means a feature
that is added into `SM.features` during creation will stay there even if
it is not available on all hosts. But we should not end up in this
situation in the first place.

Also change the meaning of `Sm.features` to be pool-wide.

Signed-off-by: Vincent Liu <[email protected]>
NEW Sm features that are found during an upgrde will now only be
available when they are available on all of the hosts. Add logic to
keep track of features that are only availabe on some of the hosts in
the pool, and declare them in `Sm.feature` only when all of the hosts
have declared this.

Also move `Storage_access.on_xapi_start` to `dbsync_slave` as this needs
to be run on all hosts for each sm to get a chance to say what features
they have.

Signed-off-by: Vincent Liu <[email protected]>
Implement a new `assert_sm_features_compatiable` in pre_join_checks so
that if the joining host does not have compatible sm features, it will
be denied entry into the pool.

Signed-off-by: Vincent Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants