Fix running Kibana Platform migrations in development

[joshdover]

Summary

This solves a problem where migrations were failing in development for saved object types registered by plugins:

  log   [14:43:33.439] [fatal][root] { Error: Document "7a694860-6cfc-11ea-ba22-77a4b53bec34" has property "index-pattern" which belongs to
a more recent version of Kibana (7.6.0).
    at find (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/document_migrator.ts:351:18)
    at Array.find (<anonymous>)
    at nextUnmigratedProp (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/document_migrator.ts:337:21)
    at applyMigrations (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/document_migrator.ts:262:18)
    at DocumentMigrator.transformDoc (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/document_migrator.ts:244:9)
    at migrateDoc (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/document_migrator.ts:155:17)
    at map (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/migrate_raw_docs.ts:46:12)
    at Array.map (<anonymous>)
    at migrateRawDocs (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/migrate_raw_docs.ts:40:18)
    at migrateSourceToDest (C:\gitRepositories\kibana\src\core\server\saved_objects\migrations\core/index_migrator.ts:198:7)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  data:

This was failing because the migrations were being run in the master dev process where plugins are not initialized, so the migrations were missing for types registered by Kibana Platform plugins.

This change ensures that migrations are only ran on the actual server process where plugins are initialized.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility, including a check against IE11

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[joshdover]

cliArgs.optimize was the wrong option to be listening for. This CLI arg is only specified when running kibana to only run the optimizer and then exit. It is not set when running the optimizer in dev. However, in both cases, plugin initialization is disabled which is really the case where we shouldn't run migrations.

[mshustov]

is this flag isDevClusterMaster already used when calculating pluginsInitialized ? https://github.com/elastic/kibana/pull/61325/files#diff-725c498a267472754f01cff2baa3901bR110

[joshdover]

No there's a --plugin.initialized config option that is used (optimizer also specifies this flag)

[joshdover]

I think we may actually be able to drop the --migrations.skip flag completely and only look at --plugins.initialize.

@rudolf Are there any cases where we would want to specify one of these but not the other? I did not find any in the codebase.

[rudolf]

@rudolf Are there any cases where we would want to specify one of these but not the other? I did not find any in the codebase.

We use it for some tests when ES isn't available which would otherwise block migrations, like

kibana/src/cli/serve/integration_tests/invalid_config.test.ts

Line 40 in ba9a6c9

['src/cli', '--config', INVALID_CONFIG_PATH, '--migrations.skip=true'],

Not sure if we can use plugins.initialize for those as we probably want plugins to be enabled during those tests.

Edit: realized after a closer look that this specific test doesn't require plugins to be initialized, but the use case is probably still valid

[rudolf]

Just had a look through our existing migrations.skip occurrences:

Redundant, can be removed:

kibana/tasks/config/run.js

Line 62 in a2c002d

'--migrations.skip=true',

Can be replaced with plugins.initialize=false

kibana/src/cli/serve/integration_tests/invalid_config.test.ts

Line 40 in ba9a6c9

['src/cli', '--config', INVALID_CONFIG_PATH, '--migrations.skip=true'],

This test spins up ES so not necessary to skip migrations. It might not be necessary to start ES for this test, if that's the case and we want to speed up the test, this would be the only case where skip migrations is useful:

kibana/packages/kbn-plugin-generator/integration_tests/generate_plugin.test.js

Line 106 in be0a4c4

'--migrations.skip=true',

[rudolf]

Left a comment of some occurrences of migrations.skip that we could clean up, but doesn't have to block this PR.

[joshdover]

I'm opting to keep migrations.skip for now until we're sure there isn't a valid use case. I can imagine other test scenarios where we may want this.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 5713fda

History

• 💚 Build #36313 succeeded 5713fda
• 💔 Build #36292 failed 09b3594
• 💔 Build #36282 failed 274f17873cd63bd560e669d0428794fb3de86251

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

[pgayvallet]

Probably low impact, but we now have the config SO type that is registered by core, not plugins. The type would be usable but not migrated with PR changes.

[joshdover]

Probably the correct way to solve is to have the processes that need to run Core only run setup and extract whatever information they need.