Skip to content

remove deps on data plugin#121955

Closed
Dosant wants to merge 16 commits intoelastic:mainfrom
Dosant:d/2021-12-23-data-deps
Closed

remove deps on data plugin#121955
Dosant wants to merge 16 commits intoelastic:mainfrom
Dosant:d/2021-12-23-data-deps

Conversation

@Dosant
Copy link
Copy Markdown
Contributor

@Dosant Dosant commented Dec 23, 2021

Summary

This pr highlights a blocker for removing data_enhanced plugin in #122075

To remove data_enhanced, we will need data plugin optionally depend on security plugin.
After fixing some circular dependency CI starts flaky failing on commit where security becomes a dependency of data plugin: df7151a

The culprit is not clear yet.

Here you can see how I had 3 successful runs before adding security as dependency and then a failure after adding it in: df7151a

Screen Shot 2022-01-06 at 16 47 25

Some observations:

  1. One of the ofter failing tests is status page should display the server status. Kibana reports Yellow status when Green is expected. After adding more logging I saw that taskManager is causing Yellow status.
    image
    logs
  2. Ofter failing tests look like Kibana or a bundle is not loaded properly
    image
    logs
  3. Some of security / reporting are often failing, for example: Reporting Functional Tests with Security enabled Security with reporting_user built-in role Dashboard: Download CSV file does not allow user that does not have reporting privileges. It fails on page load with timeout. logs. It is also often SAML / OIDC related tests that are likely to fail
  4. Sometimes the failure is: error: Setup lifecycle of "infra" plugin wasn't completed in 10sec. Consider disabling the plugin and re-start. I guess this just highlights that a bundle wasn't loaded on time
  5. Often flaky test pass by second try. Some of them fail even on second try, but with slightly different error.

What we've tried:

  1. There was and idea that plugin topology changed, making plugins resolve in different order highlighting some bugs. But this isn't the case. The list of plugins that core reports on start up is unchanged
  2. Tried to reproduce locally. Both with dev and prod build. No realiable reproduction were found.
  3. Tried to make security plugin a dependency of a different OSS plugin (discover) - didn't reproduce the issue. So issue is reproduced on data but not on discover

UPDATE 1

In #123220 we've added debug logs for security and taskManager
We've noticed that, for example, in a very simple test:

const { access_token: accessToken } = await es.security.getToken({
        body: {
          grant_type: 'password',
          username: 'elastic',
          password: 'changeme',
        },
      });

      await supertest
        .get('/internal/security/me')
        .set('kbn-xsrf', 'true')
        .set('authorization', `Bearer ${accessToken}`)
        .expect(200);

Call to es takes around 30 seconds!!

The test fails because the token expires before it can continue.
logs

Update 2

We've tried adding a delay between es + kibana start and tests run: 2da369f
🟢 🟢 Tests started passing! 🟢 🟢
This means there is clearly some instablity in the beginning causing kibana / elasticsearch slow to respond and tests to fail

@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Dec 23, 2021

@elasticmachine merge upstream

@Dosant Dosant force-pushed the d/2021-12-23-data-deps branch 3 times, most recently from 04c47cc to 72f9983 Compare December 29, 2021 11:40
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Dec 30, 2021

@elasticmachine merge upstream

@Dosant Dosant force-pushed the d/2021-12-23-data-deps branch 2 times, most recently from b846d09 to c19eb90 Compare January 5, 2022 12:09
@Dosant Dosant force-pushed the d/2021-12-23-data-deps branch from c19eb90 to c45f61c Compare January 5, 2022 13:12
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Jan 5, 2022

@elasticmachine merge upstream

@Dosant Dosant force-pushed the d/2021-12-23-data-deps branch from 15ae512 to 883cc99 Compare January 5, 2022 16:04
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Jan 6, 2022

@elasticmachine merge upstream

@Dosant Dosant mentioned this pull request Jan 6, 2022
5 tasks
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Jan 6, 2022

buildkite, test this

@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Jan 13, 2022

@elasticmachine merge upstream

@afharo
Copy link
Copy Markdown
Member

afharo commented Jan 18, 2022

@elasticmachine merge upstream

@afharo
Copy link
Copy Markdown
Member

afharo commented Feb 2, 2022

FYI: I merged main into this branch and resolved the conflicts in hopes that #121628 made this better. My local tests are passing now, so hopefully CI does as well.

@afharo afharo mentioned this pull request Feb 4, 2022
1 task
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Mar 23, 2022

@elasticmachine merge upstream

@jportner jportner self-requested a review March 23, 2022 14:11
@afharo
Copy link
Copy Markdown
Member

afharo commented Mar 29, 2022

Updating from main after #128324 was merged to see if it improves anything. That PR fixed an event-loop delay due to status-dependency processing, which happens a lot during start-up.

@afharo
Copy link
Copy Markdown
Member

afharo commented Mar 29, 2022

@elasticmachine merge upstream

@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Mar 29, 2022

buildkite, test this

@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Mar 29, 2022

buildkite, test this

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
customIntegrations 17 22 +5

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
customIntegrations 6.1KB 10.0KB +3.9KB
security 50.2KB 50.2KB -5.0B
total +3.9KB
Unknown metric groups

ESLint disabled line counts

id before after diff
customIntegrations 2 3 +1

Total ESLint disabled count

id before after diff
customIntegrations 2 3 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Dosant Dosant closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants