diff --git a/package.json b/package.json index 696563810..163bc7230 100644 --- a/package.json +++ b/package.json @@ -78,7 +78,7 @@ "@commitlint/cli": "^8.3.5", "@commitlint/config-conventional": "^8.3.4", "@date-io/moment": "1.3.9", - "@lyft/flyteidl": "^0.18.4", + "@lyft/flyteidl": "^0.18.9", "@material-ui/core": "^4.0.0", "@material-ui/icons": "^4.0.0", "@material-ui/pickers": "^3.2.2", diff --git a/src/components/Executions/Tables/NodeExecutionChildren.tsx b/src/components/Executions/Tables/NodeExecutionChildren.tsx index ee3bb32f5..6e5f52d59 100644 --- a/src/components/Executions/Tables/NodeExecutionChildren.tsx +++ b/src/components/Executions/Tables/NodeExecutionChildren.tsx @@ -40,13 +40,14 @@ export const NodeExecutionChildren: React.FC = ({ )); const key = `group-${name}`; return showNames ? ( -
+
0 }, tableStyles.borderBottom, tableStyles.childGroupLabel )} + title={name} style={childGroupLabelStyle} > = ({
{rows}
) : ( -
{rows}
+
+ {rows} +
); })} diff --git a/src/components/Executions/Tables/NodeExecutionRow.tsx b/src/components/Executions/Tables/NodeExecutionRow.tsx index ecafd66a8..5c98320ab 100644 --- a/src/components/Executions/Tables/NodeExecutionRow.tsx +++ b/src/components/Executions/Tables/NodeExecutionRow.tsx @@ -93,6 +93,7 @@ export const NodeExecutionRow: React.FC = ({ return (
{expanded ? : } diff --git a/src/components/Executions/Tables/constants.ts b/src/components/Executions/Tables/constants.ts index 31065a38d..689ecb876 100644 --- a/src/components/Executions/Tables/constants.ts +++ b/src/components/Executions/Tables/constants.ts @@ -15,3 +15,8 @@ export const nodeExecutionsTableColumnWidths = { phase: 150, startedAt: 200 }; + +export const titleStrings = { + expandRow: 'Expand row', + groupName: 'Group name' +}; diff --git a/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx b/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx index 791979630..9467f188f 100644 --- a/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx +++ b/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx @@ -1,4 +1,10 @@ -import { render, waitFor } from '@testing-library/react'; +import { + fireEvent, + getAllByRole, + getByTitle, + render, + waitFor +} from '@testing-library/react'; import { mockAPIContextValue } from 'components/data/__mocks__/apiContext'; import { APIContext, APIContextValue } from 'components/data/apiContext'; import { createMockExecutionEntities } from 'components/Executions/__mocks__/createMockExecutionEntities'; @@ -13,29 +19,39 @@ import { ExecutionDataCache } from 'components/Executions/types'; import { createExecutionDataCache } from 'components/Executions/useExecutionDataCache'; import { fetchStates } from 'components/hooks/types'; import { Core } from 'flyteidl'; +import { cloneDeep } from 'lodash'; import { + CompiledNode, + Execution, FilterOperationName, getTask, NodeExecution, + nodeExecutionQueryParams, RequestConfig, + TaskExecution, TaskNodeMetadata, + Workflow, WorkflowExecutionIdentifier } from 'models'; import { createMockExecution } from 'models/__mocks__/executionsData'; import { + createMockTaskExecutionForNodeExecution, createMockTaskExecutionsListResponse, mockExecution as mockTaskExecution } from 'models/Execution/__mocks__/mockTaskExecutionsData'; import { getExecution, + listNodeExecutions, listTaskExecutionChildren, listTaskExecutions } from 'models/Execution/api'; import { mockTasks } from 'models/Task/__mocks__/mockTaskData'; import * as React from 'react'; import { makeIdentifier } from 'test/modelUtils'; +import { obj } from 'test/utils'; import { Identifier } from 'typescript'; import { State } from 'xstate'; +import { titleStrings } from '../constants'; import { NodeExecutionsTable, NodeExecutionsTableProps @@ -48,6 +64,8 @@ describe('NodeExecutionsTable', () => { let dataCache: ExecutionDataCache; let requestConfig: RequestConfig; let mockNodeExecutions: NodeExecution[]; + let mockNodes: CompiledNode[]; + let mockWorkflow: Workflow; let mockGetExecution: jest.Mock>; let mockGetTask: jest.Mock>; let mockListTaskExecutions: jest.Mock { let mockListTaskExecutionChildren: jest.Mock>; + let mockListNodeExecutions: jest.Mock>; beforeEach(() => { const { + nodes, nodeExecutions, workflow, workflowExecution @@ -67,8 +89,11 @@ describe('NodeExecutionsTable', () => { nodeExecutionCount: 2 }); + mockNodes = nodes; mockNodeExecutions = nodeExecutions; + mockWorkflow = workflow; + mockListNodeExecutions = jest.fn().mockResolvedValue({ entities: [] }); mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] }); mockListTaskExecutionChildren = jest .fn() @@ -85,6 +110,7 @@ describe('NodeExecutionsTable', () => { apiContext = mockAPIContextValue({ getExecution: mockGetExecution, getTask: mockGetTask, + listNodeExecutions: mockListNodeExecutions, listTaskExecutions: mockListTaskExecutions, listTaskExecutionChildren: mockListTaskExecutionChildren }); @@ -103,7 +129,7 @@ describe('NodeExecutionsTable', () => { }; props = { - value: nodeExecutions, + value: mockNodeExecutions, lastError: null, state: State.from(fetchStates.LOADED), moreItemsAvailable: false, @@ -130,9 +156,7 @@ describe('NodeExecutionsTable', () => { const { queryAllByText } = renderTable(); await waitFor(() => {}); - const node = dataCache.getNodeForNodeExecution( - mockNodeExecutions[0].id - ); + const node = dataCache.getNodeForNodeExecution(mockNodeExecutions[0]); const taskId = node?.node.taskNode?.referenceId; expect(taskId).toBeDefined(); const task = dataCache.getTaskTemplate(taskId!); @@ -140,6 +164,190 @@ describe('NodeExecutionsTable', () => { expect(queryAllByText(task!.id.name)[0]).toBeInTheDocument(); }); + describe('for nodes with children', () => { + let parentNodeExecution: NodeExecution; + let childNodeExecutions: NodeExecution[]; + beforeEach(() => { + parentNodeExecution = mockNodeExecutions[0]; + }); + + const expandParentNode = async (container: HTMLElement) => { + const expander = await waitFor(() => + getByTitle(container, titleStrings.expandRow) + ); + fireEvent.click(expander); + return await waitFor(() => getAllByRole(container, 'list')); + }; + + describe('with isParentNode flag', () => { + beforeEach(() => { + const id = parentNodeExecution.id; + const { nodeId } = id; + childNodeExecutions = [ + { + ...parentNodeExecution, + id: { ...id, nodeId: `${nodeId}-child1` }, + metadata: { retryGroup: '0', specNodeId: nodeId } + }, + { + ...parentNodeExecution, + id: { ...id, nodeId: `${nodeId}-child2` }, + metadata: { retryGroup: '0', specNodeId: nodeId } + }, + { + ...parentNodeExecution, + id: { ...id, nodeId: `${nodeId}-child1` }, + metadata: { retryGroup: '1', specNodeId: nodeId } + }, + { + ...parentNodeExecution, + id: { ...id, nodeId: `${nodeId}-child2` }, + metadata: { retryGroup: '1', specNodeId: nodeId } + } + ]; + mockNodeExecutions[0].metadata = { isParentNode: true }; + mockListNodeExecutions.mockResolvedValue({ + entities: childNodeExecutions + }); + }); + + it('correctly fetches children', async () => { + const { getByText } = renderTable(); + await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId)); + expect(mockListNodeExecutions).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + params: { + [nodeExecutionQueryParams.parentNodeId]: + parentNodeExecution.id.nodeId + } + }) + ); + expect(mockListTaskExecutionChildren).not.toHaveBeenCalled(); + }); + + it('does not fetch children if flag is false', async () => { + mockNodeExecutions[0].metadata = { isParentNode: false }; + const { getByText } = renderTable(); + await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId)); + expect(mockListNodeExecutions).not.toHaveBeenCalled(); + expect(mockListTaskExecutionChildren).not.toHaveBeenCalled(); + }); + + it('correctly renders groups', async () => { + const { container } = renderTable(); + const childGroups = await expandParentNode(container); + expect(childGroups).toHaveLength(2); + }); + }); + + describe('without isParentNode flag, using taskNodeMetadata ', () => { + let taskExecutions: TaskExecution[]; + beforeEach(() => { + taskExecutions = [0, 1].map(retryAttempt => + createMockTaskExecutionForNodeExecution( + parentNodeExecution.id, + mockNodes[0], + retryAttempt, + { isParent: true } + ) + ); + childNodeExecutions = [ + { + ...parentNodeExecution + } + ]; + mockNodeExecutions = mockNodeExecutions.slice(0, 1); + // In the case of a non-isParent node execution, API should not + // return anything from the list endpoint + mockListNodeExecutions.mockResolvedValue({ entities: [] }); + mockListTaskExecutions.mockImplementation(async id => { + const entities = + id.nodeId === parentNodeExecution.id.nodeId + ? taskExecutions + : []; + return { entities }; + }); + mockListTaskExecutionChildren.mockResolvedValue({ + entities: childNodeExecutions + }); + }); + + it('correctly fetches children', async () => { + const { getByText } = renderTable(); + await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId)); + expect(mockListNodeExecutions).not.toHaveBeenCalled(); + expect(mockListTaskExecutionChildren).toHaveBeenCalledWith( + expect.objectContaining(taskExecutions[0].id), + expect.anything() + ); + }); + + it('correctly renders groups', async () => { + // We returned two task execution attempts, each with children + const { container } = renderTable(); + const childGroups = await expandParentNode(container); + expect(childGroups).toHaveLength(2); + }); + }); + + describe('without isParentNode flag, using workflowNodeMetadata', () => { + let childExecution: Execution; + let childNodeExecutions: NodeExecution[]; + beforeEach(() => { + childExecution = cloneDeep(executionContext.execution); + childExecution.id.name = 'childExecution'; + dataCache.insertExecution(childExecution); + dataCache.insertWorkflowExecutionReference( + childExecution.id, + mockWorkflow.id + ); + + childNodeExecutions = cloneDeep(mockNodeExecutions); + childNodeExecutions.forEach( + ne => (ne.id.executionId = childExecution.id) + ); + mockNodeExecutions[0].closure.workflowNodeMetadata = { + executionId: childExecution.id + }; + mockGetExecution.mockImplementation(async id => { + if (id.name !== childExecution.id.name) { + throw new Error( + `Unexpected call to getExecution with execution id: ${obj( + id + )}` + ); + } + return childExecution; + }); + mockListNodeExecutions.mockImplementation(async id => { + const entities = + id.name === childExecution.id.name + ? childNodeExecutions + : []; + return { entities }; + }); + }); + + it('correctly fetches children', async () => { + const { getByText } = renderTable(); + await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId)); + expect(mockListNodeExecutions).toHaveBeenCalledWith( + expect.objectContaining({ name: childExecution.id.name }), + expect.anything() + ); + }); + + it('correctly renders groups', async () => { + // We returned a single WF execution child, so there should only + // be one child group + const { container } = renderTable(); + const childGroups = await expandParentNode(container); + expect(childGroups).toHaveLength(1); + }); + }); + }); + it('requests child node executions using configuration from context', async () => { const { taskExecutions } = createMockTaskExecutionsListResponse(1); taskExecutions[0].isParent = true; diff --git a/src/components/Executions/TaskExecutionsList/utils.ts b/src/components/Executions/TaskExecutionsList/utils.ts index 407a29026..de4021769 100644 --- a/src/components/Executions/TaskExecutionsList/utils.ts +++ b/src/components/Executions/TaskExecutionsList/utils.ts @@ -13,7 +13,17 @@ export function getUniqueTaskExecutionName({ id }: TaskExecution) { return `${name}${suffix}`; } -export function formatRetryAttempt(attempt: number): string { +export function formatRetryAttempt( + attempt: number | string | undefined +): string { + let parsed = + typeof attempt === 'number' + ? attempt + : Number.parseInt(`${attempt}`, 10); + if (Number.isNaN(parsed)) { + parsed = 0; + } + // Retry attempts are zero-based, so incrementing before formatting - return `Attempt ${leftPaddedNumber(attempt + 1, 2)}`; + return `Attempt ${leftPaddedNumber(parsed + 1, 2)}`; } diff --git a/src/components/Executions/types.ts b/src/components/Executions/types.ts index a6e13f7d6..b0bc1b679 100644 --- a/src/components/Executions/types.ts +++ b/src/components/Executions/types.ts @@ -11,6 +11,7 @@ import { Execution, NodeExecution, NodeExecutionIdentifier, + NodeExecutionMetadata, TaskExecution, TaskExecutionIdentifier, WorkflowExecutionIdentifier @@ -47,6 +48,12 @@ export interface NodeInformation { node: CompiledNode; } +export interface ParentNodeExecution extends NodeExecution { + metadata: NodeExecutionMetadata & { + isParentNode: boolean; + }; +} + /** An interface combining a NodeExecution with data pulled from the * corresponding Workflow Node structure. */ @@ -73,12 +80,16 @@ export interface DetailedNodeExecutionGroup extends NodeExecutionGroup { export interface ExecutionDataCache { getNode(id: NodeId): GloballyUniqueNode | undefined; getNodeForNodeExecution( - nodeExecutionId: NodeExecutionIdentifier + nodeExecution: NodeExecution ): GloballyUniqueNode | null | undefined; getNodeExecutions( workflowExecutionId: WorkflowExecutionIdentifier, config: RequestConfig ): Promise; + getNodeExecutionsForParentNode( + id: NodeExecutionIdentifier, + config: RequestConfig + ): Promise; getTaskExecutions( nodeExecutionId: NodeExecutionIdentifier ): Promise; diff --git a/src/components/Executions/useChildNodeExecutions.ts b/src/components/Executions/useChildNodeExecutions.ts index 70cd4ad95..6b55313ea 100644 --- a/src/components/Executions/useChildNodeExecutions.ts +++ b/src/components/Executions/useChildNodeExecutions.ts @@ -3,6 +3,7 @@ import { useFetchableData } from 'components/hooks/useFetchableData'; import { isEqual } from 'lodash'; import { Execution, + FilterOperationName, NodeExecution, RequestConfig, TaskExecutionIdentifier, @@ -12,6 +13,7 @@ import { useContext } from 'react'; import { ExecutionContext, ExecutionDataCacheContext } from './contexts'; import { formatRetryAttempt } from './TaskExecutionsList/utils'; import { ExecutionDataCache, NodeExecutionGroup } from './types'; +import { hasParentNodeField } from './utils'; interface FetchGroupForTaskExecutionArgs { config: RequestConfig; @@ -109,6 +111,31 @@ async function fetchGroupsForWorkflowExecutionNode({ return group.nodeExecutions.length > 0 ? [group] : []; } +async function fetchGroupsForParentNodeExecution({ + config, + dataCache, + nodeExecution +}: FetchNodeExecutionGroupArgs): Promise { + const children = await dataCache.getNodeExecutionsForParentNode( + nodeExecution.id, + config + ); + const groupsByName = children.reduce>( + (out, child) => { + const retryAttempt = formatRetryAttempt(child.metadata?.retryGroup); + let group = out.get(retryAttempt); + if (!group) { + group = { name: retryAttempt, nodeExecutions: [] }; + out.set(retryAttempt, group); + } + group.nodeExecutions.push(child); + return out; + }, + new Map() + ); + return Array.from(groupsByName.values()); +} + export interface UseChildNodeExecutionsArgs { requestConfig: RequestConfig; nodeExecution: NodeExecution; @@ -136,9 +163,17 @@ export function useChildNodeExecutions({ nodeExecution: data }; - // Nested NodeExecutions will sometimes have `workflowNodeMetadata` that - // points to the parent WorkflowExecution. We're only interested in - // showing children if this node is a sub-workflow. + // Newer NodeExecution structures can directly indicate their parent + // status and have their children fetched in bulk. If the field + // is present but false, we can just return an empty list. + if (hasParentNodeField(nodeExecution)) { + return nodeExecution.metadata.isParentNode + ? fetchGroupsForParentNodeExecution(fetchArgs) + : []; + } + // Otherwise, we need to determine the type of the node and + // recursively fetch NodeExecutions for the corresponding Workflow + // or Task executions. if ( workflowNodeMetadata && !isEqual(workflowNodeMetadata.executionId, topExecution.id) diff --git a/src/components/Executions/useExecutionDataCache.ts b/src/components/Executions/useExecutionDataCache.ts index 325cf4af4..4108873fd 100644 --- a/src/components/Executions/useExecutionDataCache.ts +++ b/src/components/Executions/useExecutionDataCache.ts @@ -14,7 +14,9 @@ import { Execution, GloballyUniqueNode, Identifier, + NodeExecution, NodeExecutionIdentifier, + nodeExecutionQueryParams, NodeId, RequestConfig, TaskExecutionIdentifier, @@ -26,6 +28,7 @@ import { import { useState } from 'react'; import { ExecutionDataCache } from './types'; import { fetchTaskExecutions } from './useTaskExecutions'; +import { getNodeExecutionSpecId } from './utils'; function cacheItems( map: Map, @@ -94,10 +97,9 @@ export function createExecutionDataCache( return node; }; - const getNodeForNodeExecution = ({ - executionId, - nodeId - }: NodeExecutionIdentifier) => { + const getNodeForNodeExecution = (nodeExecution: NodeExecution) => { + const { executionId } = nodeExecution.id; + const nodeId = getNodeExecutionSpecId(nodeExecution); const workflowExecutionKey = getCacheKey(executionId); if (!workflowExecutionIdToWorkflowId.has(workflowExecutionKey)) { log.error( @@ -125,6 +127,34 @@ export function createExecutionDataCache( return nodeExecutions; }; + const getNodeExecutionsForParentNode = async ( + { executionId, nodeId }: NodeExecutionIdentifier, + config: RequestConfig + ) => { + const childrenPromise = fetchNodeExecutions( + { + config: { + ...config, + params: { + ...config.params, + [nodeExecutionQueryParams.parentNodeId]: nodeId + } + }, + id: executionId + }, + apiContext + ); + const workflowPromise = getWorkflowIdForWorkflowExecution( + executionId + ).then(workflowId => getWorkflow(workflowId)); + + const [children] = await Promise.all([ + childrenPromise, + workflowPromise + ]); + return children; + }; + const getTaskTemplate = (id: Identifier) => { const template = taskTemplatesById.get(getCacheKey(id)); if (template === undefined) { @@ -220,6 +250,7 @@ export function createExecutionDataCache( getNode, getNodeForNodeExecution, getNodeExecutions, + getNodeExecutionsForParentNode, getTaskExecutions, getTaskExecutionChildren, getTaskTemplate, diff --git a/src/components/Executions/utils.ts b/src/components/Executions/utils.ts index bc0c4d436..aa0172a00 100644 --- a/src/components/Executions/utils.ts +++ b/src/components/Executions/utils.ts @@ -32,7 +32,8 @@ import { DetailedNodeExecution, ExecutionDataCache, ExecutionPhaseConstants, - NodeExecutionDisplayType + NodeExecutionDisplayType, + ParentNodeExecution } from './types'; /** Given an execution phase, returns a set of constants (i.e. color, display @@ -109,14 +110,21 @@ export const taskExecutionIsTerminal = (taskExecution: TaskExecution) => taskExecution.closure && terminalTaskExecutionStates.includes(taskExecution.closure.phase); +export function getNodeExecutionSpecId(nodeExecution: NodeExecution): string { + return nodeExecution.metadata?.specNodeId || nodeExecution.id.nodeId; +} + /** Populates a NodeExecution with extended information read from an `ExecutionDataCache` */ export function populateNodeExecutionDetails( nodeExecution: NodeExecution, dataCache: ExecutionDataCache ) { - const { nodeId } = nodeExecution.id; + // Use `spec_node_id` if available to look up the node in the graph (needed to + // distinguish nodes in sub-workflow scenarios). But this may not exist, so + // fall back to id.nodeId in those cases. + const nodeId = getNodeExecutionSpecId(nodeExecution); const cacheKey = getCacheKey(nodeExecution.id); - const nodeInfo = dataCache.getNodeForNodeExecution(nodeExecution.id); + const nodeInfo = dataCache.getNodeForNodeExecution(nodeExecution); let displayId = nodeId; let displayType = NodeExecutionDisplayType.Unknown; @@ -220,6 +228,19 @@ function getExecutionTimingMS({ return { duration: durationMS, queued: queuedMS }; } +/** Indicates the presence of metadata for parent node status. Older executions + * may be missing this field and require additional API calls to determine if + * their are children. + */ +export function hasParentNodeField( + nodeExecution: NodeExecution +): nodeExecution is ParentNodeExecution { + return ( + nodeExecution.metadata != null && + nodeExecution.metadata.isParentNode != null + ); +} + /** Returns timing information (duration, queue time, ...) for a WorkflowExecution */ export function getWorkflowExecutionTimingMS(execution: Execution) { const { closure } = execution; diff --git a/src/models/AdminEntity/types.ts b/src/models/AdminEntity/types.ts index 1efaab9a0..8ed414514 100644 --- a/src/models/AdminEntity/types.ts +++ b/src/models/AdminEntity/types.ts @@ -71,7 +71,7 @@ export interface RequestConfig { data?: unknown; filter?: FilterOperationList; limit?: number; - params?: object; + params?: Record; token?: string; sort?: Sort; } diff --git a/src/models/Execution/constants.ts b/src/models/Execution/constants.ts index 1cfabb733..7cab0194f 100644 --- a/src/models/Execution/constants.ts +++ b/src/models/Execution/constants.ts @@ -30,4 +30,8 @@ export const executionSortFields = { startedAt: 'started_at' }; +export const nodeExecutionQueryParams = { + parentNodeId: 'uniqueParentId' +}; + export const defaultExecutionPrincipal = 'flyteconsole'; diff --git a/src/models/Execution/types.ts b/src/models/Execution/types.ts index 89244bd06..0ad9046f6 100644 --- a/src/models/Execution/types.ts +++ b/src/models/Execution/types.ts @@ -81,10 +81,17 @@ export interface NodeExecutionIdentifier extends Core.INodeExecutionIdentifier { executionId: WorkflowExecutionIdentifier; } +export interface NodeExecutionMetadata extends Admin.INodeExecutionMetaData { + retryGroup?: string; + isParentNode?: boolean; + specNodeId?: string; +} + export interface NodeExecution extends Admin.INodeExecution { id: NodeExecutionIdentifier; inputUri: string; closure: NodeExecutionClosure; + metadata?: NodeExecutionMetadata; } export interface NodeExecutionClosure extends Admin.INodeExecutionClosure { createdAt: Protobuf.ITimestamp; diff --git a/yarn.lock b/yarn.lock index cea135bc0..ced6c269a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1855,10 +1855,10 @@ dependencies: core-js "^2.5.7" -"@lyft/flyteidl@^0.18.4": - version "0.18.4" - resolved "https://registry.yarnpkg.com/@lyft/flyteidl/-/flyteidl-0.18.4.tgz#47177a33e082355a105f8276bfe15cd2388fcb4a" - integrity sha512-kQ40KN6ffUogPsQmaoLyVRvrg8tcxVaoVSDiGol4cnspeLxsB6728qBHzQY1oRcOeRBFCRASyImJrbbBVpNXeg== +"@lyft/flyteidl@^0.18.9": + version "0.18.9" + resolved "https://registry.yarnpkg.com/@lyft/flyteidl/-/flyteidl-0.18.9.tgz#e8c06a598329de052ca5b539c382749379a93ad7" + integrity sha512-BZCAiHpq+JMsd5u2m8UuedlL1E/CQZjDywXnOO27QmNzJv1zdcZ4QDkBU6ClCQQr/KOBszolyfRVY3N//3AG2Q== "@marionebl/sander@^0.6.0": version "0.6.1"