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

Do not lock on reads of XPackLicenseState #52492

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

Tim-Brooks
Copy link
Contributor

XPackLicenseState reads to necessary to validate a number of cluster
operations. This reads occasionally occur on transport threads which
should not be blocked. Currently we sychronize when reading. However,
this is unecessary as only a single piece of state is updateable. This
commit makes this state volatile and removes the locking.

@Tim-Brooks Tim-Brooks added :Security/License License functionality for commercial features backport v8.0.0 v7.7.0 labels Feb 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/License)

@Tim-Brooks
Copy link
Contributor Author

@jasontedor @jaymode - This back port required a number of additional changes, please someone review to spot check for errors.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

final OperationMode mode = status.mode;
return mode != OperationMode.BASIC && mode != OperationMode.MISSING;
});

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

return currentStatus.active && isMachineLearningAllowedForOperationMode(currentStatus.mode);
public boolean isMachineLearningAllowed() {
return checkAgainstStatus(status -> status.active && isMachineLearningAllowedForOperationMode(status.mode));

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

// Should work on all active licenses
return localStatus.active;
return checkAgainstStatus(status -> status.active);

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

mode == OperationMode.TRIAL || mode == OperationMode.BASIC;
});


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

mode == OperationMode.TRIAL || mode == OperationMode.BASIC;
});

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@ywangd
Copy link
Member

ywangd commented Feb 18, 2020

@tbrooks8 I merged some change to this file in master and your one was on top it. But I haven’t backported my changes yet. The PR #52115 is ready, but my intentions was to bundle the it with a few more refactoring. Not sure if you had to solve many conflicts with your backporting. I wonder whether it would be better to have my backport go through first to maintain consistency with original work?
@tvernum what do you think?

@Tim-Brooks
Copy link
Contributor Author

@ywangd - I will wait until you merge.

@ywangd
Copy link
Member

ywangd commented Feb 18, 2020

Thanks. I'll get it sorted out asap

@ywangd
Copy link
Member

ywangd commented Feb 21, 2020

@tbrooks8 I have merged my backport. Thanks a lot for coordinating on this.

@ywangd
Copy link
Member

ywangd commented Feb 25, 2020

@tbrooks8 Merge of #52115 caused some code conflicts for your PR. I am happy to assist in resovling them. Please let me know how I can help. Thanks.

XPackLicenseState reads to necessary to validate a number of cluster
operations. This reads occasionally occur on transport threads which
should not be blocked. Currently we sychronize when reading. However,
this is unecessary as only a single piece of state is updateable. This
commit makes this state volatile and removes the locking.
@Tim-Brooks Tim-Brooks force-pushed the backport_xpackstate_locking branch from 63d87a8 to 48ee863 Compare February 25, 2020 21:39
@Tim-Brooks
Copy link
Contributor Author

@ywangd I just cherry-picked from master again and it was clean this time.

@Tim-Brooks Tim-Brooks merged commit 6669e53 into elastic:7.x Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport :Security/License License functionality for commercial features v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants