Skip to content

Deprecate disabling basic-license features #54816

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

Conversation

williamrandolph
Copy link
Contributor

We believe there's no longer a need to be able to disable basic-license features completely using the xpack.*.enabled settings. If users don't want to use those features, they simply don't need to use them. Having such features always available lets us build more complex features that assume basic-license features are present.

This commit deprecates settings of the form "xpack.*.enabled" for basic-license features, excluding "security", which is a special case.

Meta issue: #54745

We believe there's no longer a need to be able to disable basic-license
features completely using the "xpack.*.enabled" settings. If users don't
want to use those features, they simply don't need to use them. Having
such features always available lets us build more complex features that
assume basic-license features are present.

This commit deprecates settings of the form "xpack.*.enabled" for
basic-license features, excluding "security", which is a special case.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Plugins)

@mark-vieira
Copy link
Contributor

I'm wondering then now how x-pack:qa:rolling-upgrade-basic differs from x-pack:qa:rolling-upgrade if we are now making basic feature enabled by default. Can we ditch this test suite?

@williamrandolph
Copy link
Contributor Author

williamrandolph commented Apr 6, 2020

@mark-vieira

Can we ditch this test suite?

Good question. The issue I ran into was that some tests were failing because of deprecation warnings in setup and teardown, so I brute-force removed all of the deprecated settings from the test configurations. Because we need to preserve test coverage for deprecated but not removed options, I don't think this is something that I actually want to do at this stage; instead, I will probably put those settings back where they were and find a way to ignore the relevant deprecation warnings in the test setup and teardown.

  • restore removed settings and handle deprecation warnings elsewhere

However, we are planning to make the breaking change of removing those settings altogether on the master branch and I will remove one of those test tasks when I make the breaking changes.

I should probably put the WIP tag on this PR.

@mark-vieira
Copy link
Contributor

However, we are planning to make the breaking change of removing those settings altogether on the master branch and I will remove one of those test tasks when I make the breaking changes.

Yes, we need to keep the coverage if it's just deprecated but if this is slated for removal in 8.0 then I think we can ditch this project in master. @rjernst does this seem reasonable? I'm not completely sure what we are testing in terms of licensing here that needs to stick around or if it's even related to this PR.

Here's the tests specific to this project.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I think the question of x-pack:qa:rolling-upgrade-basic vs x-pack:qa:rolling-upgrade is orthogonal to this issue. We should look at that separately. From what I remember the upgrade-basic test was added when we opened x-pack, but it really has nothing to do with whether the basic licensed features have the ability to be disabled.

The Watcher REST integration tests had been running with ILM disabled.
With that setting's deprecation, it seems that some index creation and
deletion operations will throw deprecation warnings. These deprecation
warnings don't seem to have to do with the purposes of the tests, so we
will just change the settings in build.gradle in order to avoid having
to catch them all.
Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

Generally looks good, just a few things that should be tweaked.

@williamrandolph
Copy link
Contributor Author

@debadair Thank you for the feedback and fixes!

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

LGTM

@williamrandolph
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/2

@williamrandolph
Copy link
Contributor Author

I'm running checkPart2 a few times on a cloud instance so I can be sure that the changes here don't slow down the test suite.

@williamrandolph
Copy link
Contributor Author

After letting checkPart2 run on a debian-9 cloud instance, I've got timings of 50:20, 51:30, and 51:34 for this branch. Meanwhile, the same instance ran checkPart2 on master in 52:44. While this is a pretty small sample size, it leads me to believe that these changes aren't going to slow down our test suite too much.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks good. The main nit is there seems to be several qa tests that still have monitoring disabled.

@williamrandolph williamrandolph merged commit 92c8a73 into elastic:master Apr 17, 2020
@williamrandolph williamrandolph deleted the deprecate-disabling-basic-features branch April 17, 2020 13:19
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Apr 17, 2020
We believe there's no longer a need to be able to disable basic-license
features completely using the "xpack.*.enabled" settings. If users don't
want to use those features, they simply don't need to use them. Having
such features always available lets us build more complex features that
assume basic-license features are present.

This commit deprecates settings of the form "xpack.*.enabled" for
basic-license features, excluding "security", which is a special case.
It also removes deprecated settings from integration tests and unit
tests where they're not directly relevant; e.g. monitoring and ILM are
no longer disabled in many integration tests.
williamrandolph added a commit that referenced this pull request Apr 17, 2020
We believe there's no longer a need to be able to disable basic-license
features completely using the "xpack.*.enabled" settings. If users don't
want to use those features, they simply don't need to use them. Having
such features always available lets us build more complex features that
assume basic-license features are present.

This commit deprecates settings of the form "xpack.*.enabled" for
basic-license features, excluding "security", which is a special case.
It also removes deprecated settings from integration tests and unit
tests where they're not directly relevant; e.g. monitoring and ILM are
no longer disabled in many integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants