-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ILM ] Fix logic for showing/hiding recommended allocation on Cloud #90592
[ILM ] Fix logic for showing/hiding recommended allocation on Cloud #90592
Conversation
…moved tests over from legacy test folder
|
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a new test, right? We're not testing behavior added in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have complementary assertions in the "on cloud using legacy data role config pre v8" tests?
…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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Tested locally, code LGTM! Just had a few suggestions. I'll review the tests after you address the merge conflicts.
return ( | ||
<EuiCallOut title={i18nTexts.title} data-test-subj="cloudDataTierCallout"> | ||
{i18nTexts.body} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -106,6 +107,14 @@ export const DataTierAllocationField: FunctionComponent<Props> = ({ phase, descr | |||
</> | |||
); | |||
} | |||
if (isUsingDeprecatedDataRoleConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also check if we're on Cloud?
if (isCloudEnabled && isUsingDeprecatedDataRoleConfig) {
/* snip */
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
const hasNoNodesWithNodeRole = !nodesByRoles.data_cold?.length; | ||
|
||
if (isUsingNodeRolesAllocation && hasNoNodesWithNodeRole) { | ||
if (hasDataNodeRoles && hasNoNodesWithNodeRole) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work @jloleysens ! Tested locally and it works as expected. As we discussed offline we will need to test this in Cloud environment.
…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>
…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>
…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
…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
…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>
…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>
…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 Out
Stack Trace
Kibana 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 Out
Stack Trace
Kibana 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 Out
Stack Trace
and 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: |
Relates to logic originally implemented in #81455
Summary
On cloud, hide the "X nodes (recommended)" option when
data: true
is selected. Previously we only hid this option when legacydata: true
was 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:true
config is in use on cloud.Requirements matrix
Please note: this matrix reflects node types (deprecated
data:true
config) 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:true
Screenshots
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,master
kibana.dev.yml
:yarn es snapshot -E node.data=true
. Do steps 2-4 again.Checklist