[ILM ] Fix logic for showing/hiding recommended allocation on Cloud #90592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…moved tests over from legacy test folder
jloleysens
added
release_note:fix
Feature:ILM
v8.0.0
Team:Kibana Management
|
|
|
@elasticmachine merge upstream |
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@jloleysens Can we add the 7.11.1 label and backport this to 7.11 too? Let's also change the release note label to |
cjcenizal
reviewed
Feb 8, 2021
There was a problem hiding this comment.
Thanks for jumping on this @jloleysens! I had some questions about the tests. I'm also not clear on why we've added the logic regarding pre-v8 and post-v8. This adds additional complexity, and I'm not clear on the benefits. Is there an advantage to this approach over implementing the desired v8 behavior when we're closer to shipping v8?
| const { component } = testBed; | ||
| component.update(); | ||
| }); | ||
| test('hiding default, recommended option', async () => { |
There was a problem hiding this comment.
Nit: can we change "hiding" to "removes"? I think the present tense makes the sentence a bit more grammatically sound. And I think "removes" is a slightly more accurate verb, since I think we typically use "hide" and "show" to refer to things the user can do with the UI but in this case we're taking an option off the table entirely.
| httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy('my_policy')]); | ||
| httpRequestsMockHelpers.setListNodes({ | ||
| nodesByAttributes: { test: ['123'] }, | ||
| // On cloud, even if there are data_* roles set, the default, recommended allocation option should not |
There was a problem hiding this comment.
Is this correct? Shouldn't this state that all options should be available, even if the cluster is using the deprecated data role config?
There was a problem hiding this comment.
Yes, as I understand it, this is the fix we want to make. The current check for: onCloud && usingDeprecatedConfig && !hasNodeRoles was too restrictive for removing the recommended option. The fix here, according to the feedback we got from Roy, is to hide the recommended option if we detect deprecated config on cloud: onCloud && usingDeprecatedConfig. I'll add some emphasis to the comment and try to clarify.
Let me know if this is incorrect!
| const { component } = testBed; | ||
| component.update(); | ||
| }); | ||
| test('hiding default, recommended option', async () => { |
There was a problem hiding this comment.
Should this be "offers default, recommended option"?
| cloud?: CloudSetup | ||
| ): UnmountCallback => { | ||
| render( | ||
| <I18nContext> | ||
| <KibanaContextProvider services={{ cloud, breadcrumbService, license }}> | ||
| <KibanaContextProvider | ||
| services={{ cloud, breadcrumbService, license, stackVersion: '7.12.0' }} |
There was a problem hiding this comment.
Was this hard-coded stack version intentional? I think we want this to reference the stackVersion parameter right?
There was a problem hiding this comment.
No, this was not intentional 😅, the result of testing in code. Great catch! Will fix.
| } = useKibana(); | ||
|
|
||
| const isPreV8 = useMemo(() => { | ||
| return semver.lt(stackVersion, V_8_0_0); |
There was a problem hiding this comment.
Minor nit: I don't think we gain anything by extracting the V_8_0_0 variable since it's only used in one place, and the meaning of the value is just as self-evident as the var name. How about inlining the value?
There was a problem hiding this comment.
Sure, this was perhaps premature optimisation, let's go with the string literal inline.
| const { component } = testBed; | ||
| component.update(); | ||
| }); | ||
| test('defaults searchable snapshot to true on cloud', async () => { |
There was a problem hiding this comment.
This is just a new test, right? We're not testing behavior added in this PR?
There was a problem hiding this comment.
Right, the "cloud" tests were migrated over from the legacy "components" folder.
| httpRequestsMockHelpers.setListNodes({ | ||
| isUsingDeprecatedDataRoleConfig: false, | ||
| nodesByAttributes: { test: ['123'] }, | ||
| nodesByRoles: { data: ['123'] }, |
There was a problem hiding this comment.
Can we order these similarly to those on line 681, to make it easier to compare them?
httpRequestsMockHelpers.setListNodes({
nodesByAttributes: { test: ['123'] },
nodesByRoles: { data: ['123'] },
isUsingDeprecatedDataRoleConfig: false,
});| isUsingDeprecatedDataRoleConfig: false, | ||
| nodesByAttributes: { test: ['123'] }, | ||
| nodesByRoles: { data: ['123'] }, | ||
| describe('using data role config', () => { |
There was a problem hiding this comment.
I found it difficult to discern the difference between the tests on line 798 and 810 and the tests on 703 and 741. I think it would be easier to discern the difference if we created a stronger contrast between their contexts. Could we change this description to "using node roles"? Is that an accurate description? If so, I would find it easier to contrast that against the description on line 676 of "using legacy data role config".
| expect(find('customDataAllocationOption').exists()).toBeTruthy(); | ||
| expect(find('noneDataAllocationOption').exists()).toBeTruthy(); | ||
| // We should not be showing the call-to-action for users to activate data tiers in cloud | ||
| expect(find('cloudDataTierCallout').exists()).toBeFalsy(); |
There was a problem hiding this comment.
Should we make the opposite assertion inside the "on cloud using legacy data role config pre v8" tests?
| expect(request.phases.cold.actions.searchable_snapshot.snapshot_repository).toEqual( | ||
| 'found-snapshots' | ||
| ); | ||
| test('should show cloud notice when cold tier nodes do not exist', async () => { |
There was a problem hiding this comment.
Should we have complementary assertions in the "on cloud using legacy data role config pre v8" tests?
jloleysens
added
release_note:skip
…y specific to the migration of a deployment on cloud
|
@elasticmachine merge upstream |
|
@cjcenizal I have updated PR description with more info and screenshots. I think I have addressed your feedback, would you mind taking another look? |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
cjcenizal
reviewed
Feb 19, 2021
| return ( | ||
| <EuiCallOut title={i18nTexts.title} data-test-subj="cloudDataTierCallout"> | ||
| {i18nTexts.body} | ||
| |
There was a problem hiding this comment.
This is a non-breaking space, which can interfere with the way words naturally break at the end of a line and wrap to the next line. In most cases we just want a regular breaking space, which would mean replacing this with {' '}.
There was a problem hiding this comment.
The use of here was intentional, but I see your point that it is unnecessary.
| return ( | ||
| <EuiCallOut title={i18nTexts.title} data-test-subj="cloudMissingColdTierCallout"> | ||
| {i18nTexts.body} | ||
| |
| @@ -106,6 +107,14 @@ export const DataTierAllocationField: FunctionComponent<Props> = ({ phase, descr | |||
| </> | |||
| ); | |||
| } | |||
| if (isUsingDeprecatedDataRoleConfig) { | |||
There was a problem hiding this comment.
Should this also check if we're on Cloud?
if (isCloudEnabled && isUsingDeprecatedDataRoleConfig) {
/* snip */
}| const hasNoNodesWithNodeRole = !nodesByRoles.data_cold?.length; | ||
|
|
||
| if (isUsingNodeRolesAllocation && hasNoNodesWithNodeRole) { | ||
| if (hasDataNodeRoles && hasNoNodesWithNodeRole) { |
There was a problem hiding this comment.
This code is so important to the UX and contains many conditions that can be difficult for the reader to untangle. I think the description of the PR is rich with information, but we lose a lot of that in the code itself. What do you think about adding comments to provide this info in context? To me, the most useful information is a) the intended UX and b) a human-readable summary of the conditions under consideration. For example:
switch (allocationType) {
case 'node_roles':
/**
* We'll drive Cloud users to add a cold tier to their deployment if there are no nodes
* with the cold node role.
*/
if (isCloudEnabled && phase === 'cold') {
const hasNoNodesWithNodeRole = !nodesByRoles.data_cold?.length;
if (hasDataNodeRoles && hasNoNodesWithNodeRole) {
// Tell cloud users they can deploy nodes on cloud.
return (
<>
<EuiSpacer size="s" />
<MissingColdTierCallout linkToCloudDeployment={cloudDeploymentUrl} />
</>
);
}
}
/**
* <Decribe intention and conditions here>
*/
const allocationNodeRole = getAvailableNodeRoleForPhase(phase, nodesByRoles);
if (
allocationNodeRole === 'none' ||
!isNodeRoleFirstPreference(phase, allocationNodeRole)
) {
return (
<>
<EuiSpacer size="s" />
<DefaultAllocationNotice phase={phase} targetNodeRole={allocationNodeRole} />
</>
);
}
break;
case 'node_attrs':
/**
* <Decribe intention and conditions here>
*/
if (!hasNodeAttrs) {
return (
<>
<EuiSpacer size="s" />
<NoNodeAttributesWarning phase={phase} />
</>
);
}
/**
* <Decribe intention and conditions here>
*/
if (isUsingDeprecatedDataRoleConfig) {
return (
<>
<EuiSpacer size="s" />
<CloudDataTierCallout linkToCloudDeployment={cloudDeploymentUrl} />
</>
);
}
break;
default:
return null;
}
};There was a problem hiding this comment.
Great idea! I also did a slight refactor for additional clarity regarding the "DefaultAllocationNotice" component. This was rendering either a "notice" style callout, or "warning" style callout depending on the resolved nodes.
…ndition-for-hiding-recommded-allocation * 'master' of github.com:elastic/kibana: (117 commits) [coverage] ingest data in parallel (elastic#92074) [Lens] Drag and drop performance improvements (elastic#91641) A few more environment uiFilters fixes (elastic#92044) Enabling Uptime and Dashboard a11y test (elastic#91017) [Security Solution][Detections] Adds more granular validation for nested fields (elastic#92041) [Security Solution] [Detections] add overflow-wrap for description (elastic#91945) [Security Solution] [Detections] do not truncate filename in value list table in modal (elastic#91952) Skip flaky apm test elastic#91673 (elastic#92065) [docker] Default server.name to hostname (elastic#90799) Use documentation link service for snapshot restore (elastic#91596) [Security Solution] Clearing up all jest errors and warnings (elastic#91740) Add `@kbn/analytics` to UI Shared Deps (elastic#91810) [7.12][Telemetry] Add missing fields for security telemetry (elastic#91920) [Security Solution] Adds cypress-pipe (elastic#91550) [ML] Fix event rate chart annotation position (elastic#91899) [APM] Break down error table api removing the sparklines (elastic#89138) docs: update dependencies table bug (elastic#91964) [Time to Visualize] Stay in Edit Mode After Dashboard Quicksave (elastic#91729) Unskip Search Sessions Management UI test (elastic#90110) [Fleet] Handle long text in agent details page (elastic#91776) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx
- Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles
|
@elasticmachine merge upstream |
|
@cjcenizal I think I have addressed all of your feedback, do you think you could take another look? |
|
Thank you so much for addressing my feedback @jloleysens! I won't be able to review this for a few days, so to prevent possible merge conflicts from popping up we might want to speed up the merge process by finding another reviewer to take over for me. 😬 |
|
@elasticmachine merge upstream |
sebelga
approved these changes
Feb 25, 2021
There was a problem hiding this comment.
LGTM! Great work @jloleysens ! Tested locally and it works as expected. As we discussed offline we will need to test this in Cloud environment.
jloleysens
deleted the
ilm/update-condition-for-hiding-recommded-allocation
branch
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 25, 2021
…lastic#90592) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 25, 2021
…lastic#90592) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 25, 2021
…lastic#90592) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/reactive_form/node_allocation.test.ts
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 25, 2021
…tiple-searchable-snapshot-actions * 'master' of github.com:elastic/kibana: [Rollup] Fix use of undefined value in JS import (elastic#92791) [ILM] Fix replicas not showing (elastic#92782) [Event Log] Extended README.md with the documentation for a REST API and Start plugin contract. (elastic#92562) [XY] Enables page reload toast for the legacyChartsLibrary setting (elastic#92811) [Security Solution][Case] Improve hooks (elastic#89580) [Security Solution] Update wordings and breadcrumb for timelines page (elastic#90809) [Security Solution] Replace EUI theme with mocks in jest suites (elastic#92462) docs: ✏️ use correct heading level (elastic#92806) [ILM ] Fix logic for showing/hiding recommended allocation on Cloud (elastic#90592) [Security Solution][Detections] Pull gap detection logic out in preparation for sharing between rule types (elastic#91966) [core.savedObjects] Remove _shard_doc tiebreaker since ES now adds it automatically. (elastic#92295) docs: ✏️ fix links in embeddable plugin readme (elastic#92778) # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
jloleysens
added a commit
that referenced
this pull request
Feb 25, 2021
…90592) (#92829) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
that referenced
this pull request
Feb 25, 2021
…90592) (#92828) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens
added a commit
that referenced
this pull request
Feb 25, 2021
…Cloud (#90592) (#92852) * [ILM ] Fix logic for showing/hiding recommended allocation on Cloud (#90592) * updated logic for hiding recommended allocation options on cloud and moved tests over from legacy test folder * added version check and tests for version check to enable pre v8 behaviour * implement feedback to make tests more legible, fix test names and minor refactors * added additional callout for data tier state, also added some new copy specific to the migration of a deployment on cloud * remove unused stackVersion context value * address windows max path length constraint * - Fix botched conflict resolution! - Addressed PR feedback, updated data allocation field for readability; added comments and refactored default allocation notice and warning - Added one more test case for on cloud; when to show the call to migrate to node roles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/reactive_form/node_allocation.test.ts * remove async describe * update component integration for 7.11 branch * remove legacy, duplicate test * remove unused variable
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/transform/editing·ts.transform editing edit transform with pivot configuration updates the transform and displays it correctly in the job listStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/dashboard/dashboard_filtering·ts.dashboard app using current data dashboard filtering disabling a filter unfilters the data on "before all" hook for "pie charts"Standard OutStack TraceKibana Pipeline / general / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_index·ts.detection engine api security and spaces enabled create_index t1_analyst should NOT be able to create a signal index when it has not been created yet. Should return a 403 and error that the user is unauthorizedStandard OutStack Traceand 2 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to logic originally implemented in #81455
Summary
On cloud, hide the "X nodes (recommended)" option when
data: trueis selected. Previously we only hid this option when legacydata: truewas detected AND no newdata_*roles were present. This PR is limited to fixing this logic and updating tests to hide the "recommended" option when legacy, deprecateddata:trueconfig is in use on cloud.Requirements matrix
Please note: this matrix reflects node types (deprecated
data:trueconfig) and using node roles without the presence of autoscaling. The case where autoscaling is on still needs to be addressed as a separate piece of work.data:trueScreenshots
Deployment >= 7.10 and < 8.0 AND deprecated config
We show a callout telling users they can migrate to the data tiers on cloud. This callout will show when a user is using deprecated config and has selected "custom". If they have no node attributes, the missing node attributes callout will be shown instead, but this should never be the case on cloud.
Deployment >= 7.10 and < 8.0 AND node role config
Warm tier callout
Cold tier callout (different because cold tier has slider)
How to test
0.1. You can set node roles using:
yarn es -E node.roles=data_hot,data_content,masterkibana.dev.yml:yarn es snapshot -E node.data=true. Do steps 2-4 again.Checklist