Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ The version in which this object type is being converted to a multi-namespace ty
<b>Signature:</b>

```typescript
convertToMultiNamespaceTypeVersion?: string;
readonly convertToMultiNamespaceTypeVersion?: string;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ logger instance to be used by the migration handler
<b>Signature:</b>

```typescript
log: SavedObjectsMigrationLogger;
readonly log: SavedObjectsMigrationLogger;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ The migration version that this migration function is defined for
<b>Signature:</b>

```typescript
migrationVersion: string;
readonly migrationVersion: string;
```
Original file line number Diff line number Diff line change
Expand Up @@ -661,13 +661,14 @@ function wrapWithTry(
migrationFn: SavedObjectMigrationFn,
log: Logger
) {
const context = Object.freeze({
log: new MigrationLogger(log),
migrationVersion: version,
convertToMultiNamespaceTypeVersion: type.convertToMultiNamespaceTypeVersion,
});

return function tryTransformDoc(doc: SavedObjectUnsanitizedDoc) {
try {
const context = {
log: new MigrationLogger(log),
migrationVersion: version,
convertToMultiNamespaceTypeVersion: type.convertToMultiNamespaceTypeVersion,
};
const result = migrationFn(doc, context);

// A basic sanity check to help migration authors detect basic errors
Expand Down
6 changes: 3 additions & 3 deletions src/core/server/saved_objects/migrations/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ export interface SavedObjectMigrationContext {
/**
* logger instance to be used by the migration handler
*/
log: SavedObjectsMigrationLogger;
readonly log: SavedObjectsMigrationLogger;
/**
* The migration version that this migration function is defined for
*/
migrationVersion: string;
readonly migrationVersion: string;
/**
* The version in which this object type is being converted to a multi-namespace type
*/
convertToMultiNamespaceTypeVersion?: string;
readonly convertToMultiNamespaceTypeVersion?: string;
}

/**
Expand Down
25 changes: 25 additions & 0 deletions src/core/server/saved_objects/migrationsv2/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,31 @@ describe('migrations v2 model', () => {
});

describe('model transitions from', () => {
it('transition returns new state', () => {
const initState: State = {
...baseState,
controlState: 'INIT',
currentAlias: '.kibana',
versionAlias: '.kibana_7.11.0',
versionIndex: '.kibana_7.11.0_001',
};

const res: ResponseType<'INIT'> = Either.right({
'.kibana_7.11.0_001': {
aliases: {
'.kibana': {},
'.kibana_7.11.0': {},
},
mappings: {
properties: {},
},
settings: {},
},
});
const newState = model(initState, res);
expect(newState).not.toBe(initState);
});

describe('INIT', () => {
const initState: State = {
...baseState,
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/saved_objects/migrationsv2/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { gt, valid } from 'semver';
import * as Either from 'fp-ts/lib/Either';
import * as Option from 'fp-ts/lib/Option';
import { cloneDeep } from 'lodash';

import { AliasAction, FetchIndexResponse, isLeftTypeof, RetryableEsClientError } from './actions';
import { AllActionStates, InitState, State } from './types';
import { IndexMapping } from '../mappings';
Expand Down Expand Up @@ -187,7 +187,7 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
// control state using:
// `const res = resW as ResponseType<typeof stateP.controlState>;`

let stateP: State = cloneDeep(currentState);
let stateP: State = currentState;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we sure our state workflow is pure enough for this to not have any side effect? (should be but still asking)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This might be a red herring:

https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrationsv2/migrations_state_action_machine.ts#L50

We are risking that any step might call stateP.logs.push(). So far, I found that we are using everywhere stateP.logs = [...stateP.logs, {...}], so we should be fine...

Copy link
Copy Markdown
Contributor Author

@mshustov mshustov May 11, 2021

Choose a reason for hiding this comment

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

Are we sure our state workflow is pure enough for this to not have any side effect? (should be but still asking)

@pgayvallet I'm not quite sure cloneDeep usage is justified here.
Mutations made during model execution will be returned from the model and State will be cloned with mutations during the next model call.
The only difference is the OldState mutation won't affect NewState shape. But AFAIK SavedObjectMigrationFn is the only valid case for mutations and we already clone the doc during migrateAndConvert call.


I'm fine to roll back the change. In this case, we will pay the penalty of deep cloning all the SO several times. Plus additional work for GC.

We are risking that any step might call stateP.logs.push()

@afharo Can stateP.logs.push be called with the current implementation? I think so. stateP.logs is not an immutable array, State is not frozen object (not sure why, btw)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure cloneDeep usage is justified here.

After a quick look, I don't think either. Also we're supposed to have unit tests covering these things, so I think we're fine keeping the change.


// Handle retryable_es_client_errors. Other left values need to be handled
// by the control state specific code below.
Expand Down
5 changes: 3 additions & 2 deletions src/core/server/saved_objects/migrationsv2/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ export interface LegacyDeleteState extends LegacyBaseState {
readonly controlState: 'LEGACY_DELETE';
}

export type State =
export type State = Readonly<
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

note: It's not RecursiveReadonly<...> but we can add it to make sure State is not mutated

| FatalState
| InitState
| DoneState
Expand Down Expand Up @@ -411,7 +411,8 @@ export type State =
| LegacySetWriteBlockState
| LegacyReindexState
| LegacyReindexWaitForTaskState
| LegacyDeleteState;
| LegacyDeleteState
>;

export type AllControlStates = State['controlState'];
/**
Expand Down
6 changes: 3 additions & 3 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2152,9 +2152,9 @@ export interface SavedObjectExportBaseOptions {

// @public
export interface SavedObjectMigrationContext {
convertToMultiNamespaceTypeVersion?: string;
log: SavedObjectsMigrationLogger;
migrationVersion: string;
readonly convertToMultiNamespaceTypeVersion?: string;
readonly log: SavedObjectsMigrationLogger;
readonly migrationVersion: string;
}

// @public
Expand Down
5 changes: 2 additions & 3 deletions src/plugins/dashboard/common/saved_dashboard_references.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import semverSatisfies from 'semver/functions/satisfies';
import Semver from 'semver';
import { SavedObjectAttributes, SavedObjectReference } from '../../../core/types';
import { DashboardContainerStateWithType, DashboardPanelState } from './types';
import { EmbeddablePersistableStateService } from '../../embeddable/common/types';
Expand All @@ -24,7 +23,7 @@ export interface SavedObjectAttributesAndReferences {
}

const isPre730Panel = (panel: Record<string, string>): boolean => {
return 'version' in panel ? semverSatisfies(panel.version, '<7.3') : true;
return 'version' in panel ? Semver.gt('7.3.0', panel.version) : true;
Copy link
Copy Markdown
Contributor Author

@mshustov mshustov May 11, 2021

Choose a reason for hiding this comment

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

noticed that migration spent about 7.4sec here, with these changes, migration is 0.3sec. There are other semverSatisfies usages in dashboard plugin, so it's up to code-owners whether to refactor all the remaining places.

before:
Screenshot 2021-05-11 at 14 55 09

after:
Screenshot 2021-05-11 at 14 50 37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not surprising given that <7.3 needed to be parsed and interpreted.

};

function dashboardAttributesToState(
Expand Down