Simplify ml license checking with XpackLicenseState internals #52684
Conversation
ywangd
added
>enhancement
:ml
|
Pinging @elastic/es-security (:Security/License) |
|
Pinging @elastic/ml-core (:ml) |
|
|
||
| // catch the rest, if the license is active and is at least the required model license | ||
| return licenseState.isAllowedByLicense(licenseLevel, true, false); | ||
| return licenseState.isAllowedByLicense(licenseLevel); |
There was a problem hiding this comment.
isAllowedByLicense(OperationMode) calls isAllowedByLicense(minimumMode, true, true) which is different to the previous code.
There was a problem hiding this comment.
Thanks David. You are right that this call is different from the previous code. The difference is that the previous one does not allow trial license while the current one does. But their behaviours are actually not different when examined in the context of preceding code.
In the previous version, above this line, there is another check:
if (licenseState.isAllowedByLicense(License.OperationMode.PLATINUM)) {
return true;
}If the license is trial mode, it will be allowed already by the above check and will never reach the second check, i.e. it makes no difference whether the 2nd isAllowedByLicense is called with true or false for trial license, since the 1st check short-curcuits the logic for trial license.
With above being said, there is indeed a behaviour difference: previously platinum license is considered as highest and is guaranteed to be allowed. With this change, only Enterprise is guaranteed to be allowed. While writing this, I think we can achieve the same behaviour by change the code to:
return License.OperationMode.PLATINUM.compareTo(licenseState.getOperationMode()) <= 0
|| licenseState.isAllowedByLicense(licenseLevel);There was a problem hiding this comment.
Above suggest code snippet has a racing issue as the licenseState could change between two calls. To remove potential racing issue was a motivation for this PR. Sorry yet I suggested another one. A better one should be as the follows:
final License.OperationMode minimumMode =
License.OperationMode.PLATINUM.compareTo(licenseLevel) < 0
? License.OperationMode.PLATINUM : licenseLevel;
return licenseState.isAllowedByLicense(minimumMode);There was a problem hiding this comment.
Thanks for the explanation it makes sense, there is no need for the additional platinum level check what you have written is better for future proofing. Will you backport this to 7.7?
@grabowskit do you know why we did not deem a trial licence sufficient for model inference?
There was a problem hiding this comment.
Just noticed the v7.7.0 label is not applied. But yes I plan to backport this to 7.7.
There was a problem hiding this comment.
👍
There is an argument to backport the trail licence change to 7.6.1 as it would be considered a bug if that is not what we intended. Thanks for finding this
There was a problem hiding this comment.
If the intention is "trial license should always allow access", this bug does not really manifest itself in 7.6 since the the buggy code is never really triggered. If this is the case, I'd prefer to not backport this to 7.6.1 as it requires a number of other PRs.
But if the intention is to "block trial license for access", we do need fix it then. In this case, I'd prefer to have a separate PR to fix 7.6.1 specifically to avoid cascading backports.
@tvernum Do you wanna comment on this? Thanks
There was a problem hiding this comment.
If the intention is "trial license should always allow access"
A trial license should allow all platinum features to be used (see for example https://www.elastic.co/trialextension and https://www.elastic.co/guide/en/elasticsearch/reference/current/start-trial.html#_description_15). It's a mistake if this wasn't the case before.
There was a problem hiding this comment.
Thanks @droberts195 That's my understanding as well. In this case, the bug will never be triggered as it is short-circuited by its preceding check.
| return true; | ||
| } | ||
| ); | ||
| assertTrue(builder.setLicenseLevel(License.OperationMode.ENTERPRISE.description()).build().isAvailableWithLicense(licenseState)); |
There was a problem hiding this comment.
The licensing for inference models is not straightforward as different models can be licensed at different levels can we keep these assertions please so we know exactly what the behaviour is. The test that this code calls another piece of code is not sufficient. And for the gold tests below please
There was a problem hiding this comment.
These assertions were removed because the method to be tested was reduced to just a single line which simply makes a call to XPackLicenseState#isAllowedByLicense(licenseLevel). Since tests use mock object, there really was no other logic to be tested for this method.
But if we add some of the original logic back, e.g. the platinum license check as discussed above, the relevant tests are then become necessary again and need to be added back as well.
There was a problem hiding this comment.
I know it's a one liner and the mocking makes the test awkward but it may not always be a one-liner if someone refactors the function and I'd prefer to have the safety net of asserting the expected behaviour.
That said, the best kind of code is deleted code so why not remove the method completely and call XPackLicenseState#isAllowedByLicense() directly then the tests can go.
There was a problem hiding this comment.
This is a great suggestion. I updated the code accordingly.
There was a problem hiding this comment.
LGTM.
Can you wait for a sanity check from @benwtrent or @grabowskit regarding the trial licence change before merging please
Thanks David. No problem I'll wait for all necessary checks. Plus I'll be offline soon. So no rush. |
The original conditional did check for a trial license. see So, I don't think the original code had a bug at all. Consequently, I think it should only be backported to 7.7. Regardless, the change looks good to me. Simplifies things for future additional licenses. |
…c#52684) This change removes TrainedModelConfig#isAvailableWithLicense method with calls to XPackLicenseState#isAllowedByLicense. Please note there are subtle changes to the code logic. But they are the right changes: * Instead of Platinum license, Enterprise license nows guarantees availability. * No explicit check when the license requirement is basic. Since basic license is always available, this check is unnecessary. * Trial license is always allowed.
#52863) This change removes TrainedModelConfig#isAvailableWithLicense method with calls to XPackLicenseState#isAllowedByLicense. Please note there are subtle changes to the code logic. But they are the right changes: * Instead of Platinum license, Enterprise license nows guarantees availability. * No explicit check when the license requirement is basic. Since basic license is always available, this check is unnecessary. * Trial license is always allowed.
This change reduces
TrainedModelConfig#isAvailableWithLicensemethod to a single call toXPackLicenseState#isAllowedByLicense. It is made possible by the recent enhancements to license checking logic.Please note there are subtle change to the code logic. But they seem to be the right changes: