Skip to content

Conversation

@williamrandolph
Copy link
Contributor

@williamrandolph williamrandolph commented Apr 22, 2020

In #54816, we deprecated a group of toggle settings for basic license level features. In #55416 (in review), we are turning most of those settings into no-ops, i.e., having them in your settings file won't throw an error, but they won't do anything at all. I left monitoring and ILM for separate PRs because turning them into no-ops affects logic in other modules.

This PR turns xpack.ilm.enabled into a no-op. It removes logic and tests in xpack core, ML, and Watcher. The latter two modules in particular had to switch behavior based on the enablement or disablement of ILM, which is the kind of thing we're trying to eliminate with this effort; ILM should be a basic "building block" for higher level modules.

Meta issue: #54745

  • documentation

@williamrandolph williamrandolph added :Core/Infra/Plugins Plugin API and infrastructure >deprecation labels Apr 22, 2020
@elasticmachine
Copy link
Collaborator

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

@williamrandolph
Copy link
Contributor Author

@elasticmachine test this please

@williamrandolph
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/2

1 similar comment
@williamrandolph
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/2

@williamrandolph williamrandolph requested a review from rjernst April 23, 2020 15:54
@williamrandolph williamrandolph marked this pull request as ready for review April 23, 2020 15:54
@williamrandolph
Copy link
Contributor Author

williamrandolph commented Apr 23, 2020

I think I ought to add a new Watcher-specific setting that replaces how the old xpack.ilm.enabled setting was used in the watcher module. This would allow users who are managing Watcher indices with Curator to keep doing what they're already doing.

  • Introduce Watcher setting to not use ILM.

templatesToUse = Arrays.asList(
ANOMALY_DETECTION_RESULTS_TEMPLATE,
ilmEnabled ? ANOMALY_DETECTION_STATE_TEMPLATE : ANOMALY_DETECTION_STATE_TEMPLATE_NO_ILM,
ANOMALY_DETECTION_STATE_TEMPLATE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove ANOMALY_DETECTION_STATE_TEMPLATE_NO_ILM and STATS_TEMPLATE_NO_ILM now?

Copy link
Contributor Author

@williamrandolph williamrandolph Apr 23, 2020

Choose a reason for hiding this comment

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

I will reach out to someone from ML to see if this change is OK, but I would guess that we will be able to remove those templates.

  • get feedback from someone on ML for removing NO_ILM templates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dimitris-athanasiou and @przemekwitek have confirmed that these templates can be removed.

Choose a reason for hiding this comment

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

(Przemek from ML team): I think it is ok to remove the non-ILM versions.
Once only ILM versions exist, it would be good to inline the variables into the .json files but if you don't feel like doing it, I can do it as a followup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@przemekwitek Do you mean like this?

    private static IndexTemplateConfig stateTemplate() {
        Map<String, String> variables = new HashMap<>();
        variables.put(VERSION_ID_PATTERN, String.valueOf(Version.CURRENT.id));
        variables.put("xpack.ml.index.lifecycle.name", ML_SIZE_BASED_ILM_POLICY_NAME);
        variables.put("xpack.ml.index.rollover_alias.name", AnomalyDetectorsIndex.jobStateIndexWriteAlias());

        return new IndexTemplateConfig(AnomalyDetectorsIndexFields.STATE_INDEX_PREFIX,
            ANOMALY_DETECTION_PATH + "state_index_template.json",
            Version.CURRENT.id, VERSION_PATTERN,
            variables);
    }

…with this in the template json file?

  "settings" : {
    "index" : {
      "auto_expand_replicas" : "0-1",
      "hidden": true
    },
    "index.lifecycle.name": "${xpack.ml.index.lifecycle.name}",
    "index.lifecycle.rollover_alias": "${xpack.ml.index.rollover_alias.name}"
  },

Or should the actual values be inlined as well?

Choose a reason for hiding this comment

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

Any of these 2 solutions work for me.
It's most important for me to get rid of syntax tokens (like comma ",") which were just a hack to allow enabled/disabled duality.
The first one is more flexible so my preference goes there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a commit with the first, more flexible strategy. Thanks for the feedback!

Copy link

@przemekwitek przemekwitek Apr 24, 2020

Choose a reason for hiding this comment

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

Could you make the variable name the same for both templates?
I.e. xpack.ml.index.lifecycle.name and xpack.ml.index.lifecycle.rollover_alias for both state and stats templates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This is done.

Choose a reason for hiding this comment

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

Thank you!

Copy link

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

templatesToUse = Arrays.asList(
ANOMALY_DETECTION_RESULTS_TEMPLATE,
ilmEnabled ? ANOMALY_DETECTION_STATE_TEMPLATE : ANOMALY_DETECTION_STATE_TEMPLATE_NO_ILM,
ANOMALY_DETECTION_STATE_TEMPLATE,

Choose a reason for hiding this comment

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

Thank you!

@williamrandolph
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/bwc

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.

Thanks for adding the fallback to the old setting, so that users do not suddenly break if they were using curator externally for watcher management. LGTM.

@williamrandolph williamrandolph merged commit c4b11e4 into elastic:master Apr 29, 2020
@williamrandolph williamrandolph deleted the disabled-ilm-is-no-op branch April 29, 2020 19:44
@williamrandolph
Copy link
Contributor Author

Rats. Github lost the commit message I wrote and used the generated commit summary message instead. Here's what I wanted to use:

This commit makes the 'xpack.ilm.enabled' setting into a no-op for all
purposes except for Watcher index management. ILM is an important building
block for higher level services, so, along with other basic license features,
we want it to always be available in the default distribution.

Since users might use a tool other than ILM to manage Watcher indices, we
introduce a new 'xpack.watcher.use_ilm_index_management' setting. If this
setting is not set explicitly, it will fall back to the value of
'xpack.ilm.enabled', meaning that this will not be a breaking change for our
users.

This commit *does* make ILM mandatory for ML by removing the "NO_ILM"
templates for ML indices.

williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Apr 29, 2020
* Make xpack.ilm.enabled setting a no-op

* Add watcher setting to not use ILM

* Update documentation for no-op setting

* Remove NO_ILM ml index templates

* Remove unneeded setting from test setup

* Inline variable definitions for ML templates

* Use identical parameter names in templates

* New ILM/watcher setting falls back to old setting

* Add fallback unit test for watcher/ilm setting
williamrandolph added a commit that referenced this pull request Apr 30, 2020
* Make xpack.ilm.enabled setting a no-op

* Add watcher setting to not use ILM

* Update documentation for no-op setting

* Remove NO_ILM ml index templates

* Remove unneeded setting from test setup

* Inline variable definitions for ML templates

* Use identical parameter names in templates

* New ILM/watcher setting falls back to old setting

* Add fallback unit test for watcher/ilm setting
williamrandolph added a commit that referenced this pull request Jun 4, 2020
In #55592 and #55416, we deprecated the settings for enabling and disabling
basic license features and turned those settings into no-ops. Since doing so,
we've had feedback that this change may not give users enough time to cleanly
switch from non-ILM index management tools to ILM. If two index managers
operate simultaneously, results could be strange and difficult to
reconstruct. We don't know of any cases where SLM will cause a problem, but we
are restoring that setting as well, to be on the safe side.

This PR is not a strict commit reversion. First, we are keeping the new
xpack.watcher.use_ilm_index_management setting, introduced when
xpack.ilm.enabled was made a no-op, so that users can begin migrating to using
it. Second, the SLM setting was modified in the same commit as a group of other
settings, so I have taken just the changes relating to SLM.
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Jun 4, 2020
In elastic#55592 and elastic#55416, we deprecated the settings for enabling and disabling
basic license features and turned those settings into no-ops. Since doing so,
we've had feedback that this change may not give users enough time to cleanly
switch from non-ILM index management tools to ILM. If two index managers
operate simultaneously, results could be strange and difficult to
reconstruct. We don't know of any cases where SLM will cause a problem, but we
are restoring that setting as well, to be on the safe side.

This PR is not a strict commit reversion. First, we are keeping the new
xpack.watcher.use_ilm_index_management setting, introduced when
xpack.ilm.enabled was made a no-op, so that users can begin migrating to using
it. Second, the SLM setting was modified in the same commit as a group of other
settings, so I have taken just the changes relating to SLM.
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.

6 participants