Deprecation logs indexing is enabled by default. Backport(#78991) by pgomulka · Pull Request #79035 · elastic/elasticsearch

pgomulka · 2021-10-13T08:07:37Z

Changing the default for deprecation log indexing to be true.
This commit also overrides this default to tests where a deprecation
data stream would interfere - because it uses index template, it would
not be possible to delete with _index_template/*.
The overrides should be removed when #78850 is done.

closes #76292
backport #78991

Changing the default for deprecation log indexing to be true. This commit also overrides this default to tests where a deprecation data stream would interfere - because it uses index template, it would not be possible to delete with _index_template/*. The overrides should be removed when elastic#78850 is done. closes elastic#76292

…tion_indexing_by_default

pgomulka · 2021-10-14T15:34:38Z

@elasticmachine run elasticsearch-ci/part-1

pgomulka · 2021-10-14T15:54:22Z

build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java

        commonNodeConfig(node, null, null);
        // We need to translate these settings there as there's no support to do per version config for testclusters yet
+
+        if (node.getTestDistribution().equals(TestDistribution.DEFAULT)) {


interestingly I did not have to do this in master - I did not see mixed-cluster rolling-upgrade test failures.
Without this change - the rolling-upgrade were failing because when node upgraded to v7.16 the setting was set in the upgraded node config.
but it should be? it is being set in comonNodeConfig..

I thought you made this changed in #79226 as well though?

yes - but the #79226 is just a follow up. I added it there as a precaution - I have not seen this failing in master yet

pgomulka · 2021-10-14T15:55:58Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Archives.java

        Installation installation = Installation.ofArchive(sh, distribution, fullInstallPath);
        ServerUtils.disableGeoIpDownloader(installation);
-
+        if (Platforms.WINDOWS) {


this is really worrying, but I gave up for now.
the Windows was resulting with error

Unexpected exit code (expected 0, got 1) for script: C:\tmp\elasticsearch\bin\elasticsearch-service.bat stop

I am pretty sure that it emitted deprecation warnings (security licence related), but why would it fail the stop script?
https://gradle-enterprise.elastic.co/s/ccdeyjv34nm6m/tests/:qa:os:destructiveDistroTest.default-windows-archive/org.elasticsearch.packaging.test.WindowsServiceTests/test33JavaChanged
this test is always failing when deprecation log indexing is enabled

Looking at Bootstrap, we have extra handling for stopping gracefully on Windows, which involves the Node explicitly closing all components and plugins. This could result in something logging a deprecation warning while the node was in the middle of shutting down, which then causes the bulk processor to explode, and take out the node, causing a non-zero exit code.

We could amend DeprecationIndexingComponent#doStop to also cancel the bulk processor. BulkProcessordoesn't have acancel()method at the moment, but we could add one. It would be likeclose(or more accuratelyawaitClose()`), except it wouldn't flush any pending actions.

What do you think?

I think this is a good idea, we definitely need some more hardening around shutdown, data stream deletion
this test specifically failed with this in logs

[2021-10-14T10:59:33,612][ERROR][o.e.x.d.l.DeprecationIndexingComponent] [ELASTICSEARCH-C] Bulk write of deprecation logs encountered some failures: [[gS91fnwBfMC4zzZUEPlp NodeClosedException[node closed {ELASTICSEARCH-C}{wjutwiy2RCKty_vlySBv9g}{RJuxwdfbQrqSFK9-27Tzzw}{127.0.0.1}{127.0.0.1:9300}{cdfhilmrstw}{ml.machine_memory=103075397632, xpack.installed=true, transform.node=true, ml.max_open_jobs=512, ml.max_jvm_size=1073741824}]]]

but I have seen errors when a data stream was deleted (together with indices but a bulk processor still attempted to index documents.

Can you create a followup issue to track this down/improve it?

pgomulka · 2021-10-14T15:56:53Z

...re/src/main/resources/org/elasticsearch/xpack/deprecation/deprecation-indexing-settings.json

    "settings": {
      "index": {
        "hidden" : true,
+        "auto_expand_replicas" : "0-1",


also interesting I did not have to set this on master. In 7.x I got some failures complaining about not able to allocate replica in testing

Again, isn't this being set in #79226?

yes but https://github.com/elastic/elasticsearch/pull/79226/files is against master
this PR is a backport and it would fail in CI without this change. Somehow master CI was able to pass

pugnascotia · 2021-10-15T13:32:00Z

...ion/qa/rest/src/javaRestTest/java/org/elasticsearch/xpack/deprecation/DeprecationHttpIT.java

-        final boolean transientValue = jsonNode.at("/transient/cluster.deprecation_indexing.enabled").asBoolean();
-        assertTrue(transientValue);
+        final boolean defaultValue = jsonNode.at("/defaults/cluster.deprecation_indexing.enabled").asBoolean();
+        assertTrue(defaultValue);


Isn't the default value of this setting always true now? i.e. this assertion isn't useful any more?

there is a possibility that a test in another class (there is ML test in this module) executes first and pollutes the cluster (overrides this setting)

pugnascotia · 2021-10-15T13:33:44Z

...ation/qa/rest/src/javaRestTest/java/org/elasticsearch/xpack/deprecation/MlDeprecationIT.java


+    @After
+    public void resetFeatures() throws IOException {
+        Response response = adminClient().performRequest(new Request("POST", "/_features/_reset"));


Why do we need this now?

previously on master there were a lot of failures after #78991 was merged (somehow the CI did not fail..)
The fix was to use _features/_reset api which do not trigger deprecation warning.
This was merged in master as a follow up fix - #79071

rjernst

LGTM

rjernst · 2021-10-18T16:11:01Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Archives.java

        Installation installation = Installation.ofArchive(sh, distribution, fullInstallPath);
        ServerUtils.disableGeoIpDownloader(installation);
-
+        if (Platforms.WINDOWS) {


Can you create a followup issue to track this down/improve it?

pgomulka added the backport label Oct 13, 2021

pgomulka self-assigned this Oct 13, 2021

elasticsearchmachine added the v7.16.0 label Oct 13, 2021

pgomulka changed the title ~~Deprecation logs indexing is enabled by default~~ Deprecation logs indexing is enabled by default. Backport(#78991) Oct 13, 2021

pgomulka added 12 commits October 13, 2021 11:40

disable ml for test

e687f4c

Merge remote-tracking branch 'upstream/7.x' into bp/7x_enable_depreca…

8942fcb

…tion_indexing_by_default

reset feature api in deprecation module

66952b1

Merge remote-tracking branch 'upstream/7.x' into bp/7x_enable_depreca…

780d579

…tion_indexing_by_default

code style

2adb976

remove types usage and add autoexpand replica

bb926fe

fix server utils

16065db

disable for bwctest

cbe076d

disable for mixed cluster test

e657862

settings for rolling upgrades

b4ac7b4

add more debug to windows test

6ab2398

disable deprecation log indexing for windows

5f7ba49

pgomulka commented Oct 14, 2021

View reviewed changes

pgomulka requested review from pugnascotia and rjernst October 14, 2021 15:57

pgomulka mentioned this pull request Oct 15, 2021

Set Auto expand replica on deprecation log data stream #79226

Merged

pugnascotia reviewed Oct 15, 2021

View reviewed changes

rjernst approved these changes Oct 18, 2021

View reviewed changes

pgomulka mentioned this pull request Oct 18, 2021

Investigate shut down problems with deprecation log indexing #79375

Closed

pgomulka merged commit e25cf1a into elastic:7.x Oct 18, 2021

jakelandis mentioned this pull request Oct 21, 2021

Improve deprecation message for settings #79666

Closed

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

pgomulka commented Oct 13, 2021 • edited by pugnascotia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka commented Oct 14, 2021

Uh oh!

pgomulka Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pgomulka commented Oct 13, 2021 •

edited by pugnascotia

Loading

pgomulka Oct 14, 2021 •

edited

Loading

pgomulka Oct 14, 2021 •

edited

Loading

pugnascotia Oct 15, 2021 •

edited

Loading