[Graph] Make Graph saved object share-capable #111404

mbondyra · 2021-09-07T14:49:58Z

To be merged preferably after: #109617
Fixes #105812

Step 4 of https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-step-4

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

Step 4 of https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-faq-changing-object-ids

elasticmachine · 2021-09-08T12:55:04Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jportner · 2021-09-08T14:01:18Z

x-pack/plugins/graph/server/saved_objects/migrations.ts

@@ -88,4 +88,5 @@ export const graphMigrations = {
    doc.references = [];
    return doc;
  },
+  '8.0.0': (doc: SavedObjectUnsanitizedDoc<any>) => doc, // no-op, bump migration version to satisfy integration tests after adding convertToMultiNamespaceTypeVersion to graph_workspace SO


This bit shouldn't be necessary, and I don't think it will fix the integration tests.

Sorry I should have probably clarified, specifying convertToMultiNamespaceTypeVersion causes Core to add a special migration under the hood, so any Graph objects created will automatically have a migrationVersion of 8.0.0.
I think the problem in this case is the test data is actually malformed, it happened to work before but now it does not, so the test data needs to be fixed.

I tried this locally and it did fix the test... I'll try to see what you mean by malformed data. Btw I based this PR on https://github.com/elastic/kibana/pull/110386/files, how come it is helpful there and not here?

I tried this locally and it did fix the test... I'll try to see what you mean by malformed data.

Sorry, I got this PR for Graph confused with the other one for Lens #111403 (review)
So ignore what I said about malformed data!

Btw I based this PR on https://github.com/elastic/kibana/pull/110386/files, how come it is helpful there and not here?

That PR is for Alerting, which uses encrypted saved objects. They need a no-op migration function that will decrypt and re-encrypt the object (Step 5 of the Sharing Saved Objects dev guide). We had to do it that way because of the way encryption is handled in a plugin, not in Core.

Anyway, you adding a no-op migration here doesn't hurt anything, but it shouldn't be needed to pass CI. If it is needed, that might be indicative of a Core bug that I need to fix.
What exactly was the test failure?

Here's the build failure: https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/151533/testReport/junit/Jest%20Integration%20Tests/src_core_server_saved_objects_migrationsv2_integration_tests/Kibana_Pipeline___general___migration_v2_migrating_from_the_same_Kibana_version_that_used_v1_migrations_migrates_the_documents_to_the_highest_version/

It's reproducible locally. Without my changes in migrations file the version for graph_workspace stays on the last explicit migration (7.11).

The actual version is equal 8.0.0 (correct) and the expectedVersion is wrong. That's because the expected one is read for all saved objects here: https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_v1.test.ts#L116 from type.migrations property.

Ohh, okay, thanks for clarifying.
So yeah, I think in this case the core integration test needs to be changed, you shouldn't have to add this no-op to pass the test 😄

If you can wait a day or two, I'd be happy to add the Core changes and test them out locally. We can merge them in this PR if you're OK with that.

Sure, it can wait till 8.0.0! 😀

Okay, it appears to be after working hours for you so I took the liberty of just pushing the changes to this branch to get CI started, I hope you don't mind 🙂

This reverts commit 04d2f49.

This reverts commit 7b0a74d.

The existing tests incorrectly asserted an object's `migrationVersion` solely based on the registered type's `migration` field; in reality, the `convertToMultiNamespaceTypeVersion` field is also used when determining an object's `migrationVersion`. This commit simply updates the test to reflect that.

pgayvallet · 2021-09-10T09:10:16Z

x-pack/plugins/graph/server/saved_objects/graph_workspace.ts

+  namespaceType: 'multiple-isolated',
+  convertToMultiNamespaceTypeVersion: '8.0.0',


@jportner we don't need to adapt any call to use resolve instead of get for this specific SO type?

This is done separately in this PR: #109617

kertal · 2021-09-10T09:45:51Z

src/core/server/saved_objects/migrationsv2/integration_tests/migration_from_v1.test.ts

+        if (migrations || convertToMultiNamespaceTypeVersion) {
+          const migrationsMap = typeof migrations === 'function' ? migrations() : migrations;
+          const migrationsKeys = migrationsMap ? Object.keys(migrationsMap) : [];
+          if (convertToMultiNamespaceTypeVersion) {
+            // Setting this option registers a conversion migration that is reflected in the object's `migrationVersions` field
+            migrationsKeys.push(convertToMultiNamespaceTypeVersion);
+          }


QQ: so you're checking for convertToMultiNamespaceTypeVersion 2 times. if think the first is redundant?

convertToMultiNamespaceTypeVersion will impact the object's migrationVersion regardless of whether a migrations object/function is defined or not.

To rephrase: the union of the object type definition's migrations and convertToMultiNamespaceTypeVersion determines the resulting objects' migrationVersion.

@kertal in 122 line it's || operation so all good :)

dimaanj

LGTM, do not have objections in terms of Grpah. Code review only.

mbondyra · 2021-09-12T21:02:57Z

@elasticmachine merge upstream

kibanamachine · 2021-09-12T23:17:58Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #152313 succeeded 11c905d
💚 Build #151793 succeeded 7b0a74d
💔 Build #151533 failed 5aa9464

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

kertal

LGTM 👍 , code review, didn't test

…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (120 commits) [TSVB] Support custom field format (elastic#101245) [VisEditors] Add code ownership to the functional tests (elastic#111680) [Lens] Make Lens saved object share-capable (elastic#111403) [Graph] Make Graph saved object share-capable (elastic#111404) [Stack Monitoring] Add breadcrumb support (elastic#111850) Update Jira Cloud to use OAuth2.0 (elastic#111493) Show warning message when attempting to create an APM alert in stack management (elastic#111781) Skip suite blocking ES snapshot promotion (elastic#111907) Respect `auth_provider_hint` if session is not authenticated. (elastic#111521) Added in 'Responses' field in alert telemetry & updated test (elastic#111892) [Usage collection] refactor cloud detector collector (elastic#110439) Make classnames a shared dep (elastic#111636) Fix link to e2e tests in APM testing.md (elastic#111869) [Security Solution] Add host.os.name.caseless mapping and runtime field (elastic#111455) [APM] Removes the beta label from APM tutorial (elastic#111499) (elastic#111828) [RAC] [Observability] Expand Observability alerts page functional tests (elastic#111297) Fix extra white space on the alert table whe page size is 50 or 100 (elastic#111568) [Metrics UI] Add Inventory Timeline open/close state to context and URL state (elastic#111034) [Graph] Switch to SavedObjectClient.resolve (elastic#109617) [APM] Adding lambda icon (elastic#111834) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap

mbondyra added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) Feature:Graph Graph application feature labels Sep 7, 2021

[graph] Make Graph saved object share-capable

6d46130

Step 4 of https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-faq-changing-object-ids

mbondyra force-pushed the graph/make_so_sharecapable branch from 5d35bfc to 6d46130 Compare September 8, 2021 12:44

mbondyra added 2 commits September 8, 2021 14:45

bump migration version to fix the test

04d2f49

test

7b0a74d

mbondyra marked this pull request as ready for review September 8, 2021 12:55

mbondyra requested a review from a team as a code owner September 8, 2021 12:55

mbondyra requested a review from jportner September 8, 2021 12:57

kertal requested a review from dimaanj September 8, 2021 13:02

jportner reviewed Sep 8, 2021

View reviewed changes

mbondyra added the backport:skip This commit does not require backporting label Sep 8, 2021

jportner self-requested a review September 9, 2021 16:29

jportner added 4 commits September 9, 2021 12:53

Merge branch 'master' into pr/mbondyra/111404

d442f3e

Revert "bump migration version to fix the test"

3c87b53

This reverts commit 04d2f49.

Revert "test"

471e0ef

This reverts commit 7b0a74d.

mbondyra requested a review from a team as a code owner September 9, 2021 17:00

pgayvallet reviewed Sep 10, 2021

View reviewed changes

kertal reviewed Sep 10, 2021

View reviewed changes

jportner removed their request for review September 10, 2021 14:28

dimaanj approved these changes Sep 10, 2021

View reviewed changes

pgayvallet approved these changes Sep 10, 2021

View reviewed changes

Merge branch 'master' into graph/make_so_sharecapable

f475e06

kertal self-requested a review September 13, 2021 05:20

kertal approved these changes Sep 13, 2021

View reviewed changes

mbondyra merged commit f9f9070 into elastic:master Sep 13, 2021

mbondyra deleted the graph/make_so_sharecapable branch September 13, 2021 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Graph] Make Graph saved object share-capable #111404

[Graph] Make Graph saved object share-capable #111404

mbondyra commented Sep 7, 2021 •

edited

Loading

elasticmachine commented Sep 8, 2021

jportner Sep 8, 2021

mbondyra Sep 8, 2021

jportner Sep 8, 2021 •

edited

Loading

mbondyra Sep 8, 2021 •

edited

Loading

jportner Sep 8, 2021

mbondyra Sep 8, 2021

jportner Sep 9, 2021

pgayvallet Sep 10, 2021

mbondyra Sep 10, 2021

kertal Sep 10, 2021 •

edited

Loading

jportner Sep 10, 2021

mbondyra Sep 10, 2021

dimaanj left a comment

mbondyra commented Sep 12, 2021

kibanamachine commented Sep 12, 2021

kertal left a comment •

edited

Loading

		namespaceType: 'multiple-isolated',
		convertToMultiNamespaceTypeVersion: '8.0.0',

[Graph] Make Graph saved object share-capable #111404

[Graph] Make Graph saved object share-capable #111404

Conversation

mbondyra commented Sep 7, 2021 • edited Loading

Checklist

Risk Matrix

For maintainers

elasticmachine commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

mbondyra Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaanj left a comment

Choose a reason for hiding this comment

mbondyra commented Sep 12, 2021

kibanamachine commented Sep 12, 2021

💚 Build Succeeded

Metrics [docs]

History

kertal left a comment • edited Loading

Choose a reason for hiding this comment

mbondyra commented Sep 7, 2021 •

edited

Loading

jportner Sep 8, 2021 •

edited

Loading

mbondyra Sep 8, 2021 •

edited

Loading

kertal Sep 10, 2021 •

edited

Loading

kertal left a comment •

edited

Loading