Skip to content

Conversation

@cplaursen
Copy link
Contributor

This commit addresses a fixme in idl/datamodel_pool.ml. The name pool_allowed_operations conflicts with field Pool.allowed_operations, so renaming for clarity.

As far as I can tell, this change does not impact functionality in any way.

@cplaursen
Copy link
Contributor Author

Forgot to bump minor schema version

@psafont
Copy link
Member

psafont commented Nov 12, 2025

I'm pretty sure changing an enum value in the IDL will break pools as soon as there's one host in the new version, and another in the old version: they won't be able to communicate with each other because they don't recognise each other's enum.

Please do test RPUs with this change and report back

( "pool_allowed_operations"
, (* FIXME: This should really be called `pool_operations`, to avoid confusion with the Pool.allowed_operations field *)
[
( "pool_operations"
Copy link
Member

Choose a reason for hiding this comment

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

It may seem that this string isn't actually used anywhere, at least by RPCs on the wire. But this may not be true for the language bindings in the SDK. So this change may break client code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does indeed change the name of the pool operations from (e.g. Java) PoolAllowedOperations to PoolOperations. Presumably there is no way around this, is there ever a time where we can change that name or do we just remove the fixme?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's not worth the possible fallout of changing this, so I'd just drop the comment.

@cplaursen
Copy link
Contributor Author

I'm pretty sure changing an enum value in the IDL will break pools as soon as there's one host in the new version, and another in the old version: they won't be able to communicate with each other because they don't recognise each other's enum.

Please do test RPUs with this change and report back

The changes passed an RPU test on XenRT - I'll have a look at whether there are any SDK changes.

@minglumlu
Copy link
Member

minglumlu commented Nov 13, 2025

The changes passed an RPU test on XenRT - I'll have a look at whether there are any SDK changes.

I think the potential problem is for the clients who are using an old SDK to communicate with the new XAPI with this change.
But I'm really not sure about this.

Signed-off-by: Christian Pardillo Laursen <christian.pardillolaursen@citrix.com>
@cplaursen
Copy link
Contributor Author

I'll be removing the comment instead - I'll open a pull request for that

@cplaursen cplaursen closed this Nov 13, 2025
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