From 671320d4f52bdb948e5cfeef27a90c3c97e9157d Mon Sep 17 00:00:00 2001 From: Anton Gilgur <4970083+agilgur5@users.noreply.github.com> Date: Tue, 23 Apr 2024 00:53:26 -0400 Subject: [PATCH] refactor: simplify `getPodName` and make consistent with back-end (#12964) Signed-off-by: Anton Gilgur --- ui/src/app/shared/pod-name.test.ts | 78 ++++++++----------- ui/src/app/shared/pod-name.ts | 41 +++++----- .../workflow-details/workflow-details.tsx | 18 +---- .../workflow-logs-viewer.tsx | 9 +-- .../workflow-node-info/workflow-node-info.tsx | 10 +-- workflow/util/pod_name.go | 3 +- 6 files changed, 59 insertions(+), 100 deletions(-) diff --git a/ui/src/app/shared/pod-name.test.ts b/ui/src/app/shared/pod-name.test.ts index 31de1ba52a41..4d7cd928a647 100644 --- a/ui/src/app/shared/pod-name.test.ts +++ b/ui/src/app/shared/pod-name.test.ts @@ -1,4 +1,6 @@ -import {Inputs, MemoizationStatus, NodePhase, NodeStatus, NodeType, Outputs, RetryStrategy} from '../../models'; +import {NodeStatus, Workflow} from '../../models'; +import {ANNOTATION_KEY_POD_NAME_VERSION} from './annotations'; + import {createFNVHash, ensurePodNamePrefixLength, getPodName, getTemplateNameFromNode, k8sNamingHashLength, maxK8sResourceNameLength, POD_NAME_V1, POD_NAME_V2} from './pod-name'; describe('pod names', () => { @@ -9,9 +11,6 @@ describe('pod names', () => { }); // note: the below is intended to be equivalent to the server-side Go code in workflow/util/pod_name_test.go - const nodeName = 'nodename'; - const nodeID = '1'; - const shortWfName = 'wfname'; const shortTemplateName = 'templatename'; @@ -29,58 +28,44 @@ describe('pod names', () => { }); test('getPodName', () => { - const v1podName = nodeID; - const v2podName = `${shortWfName}-${shortTemplateName}-${createFNVHash(nodeName)}`; - expect(getPodName(shortWfName, nodeName, shortTemplateName, nodeID, POD_NAME_V2)).toEqual(v2podName); - expect(getPodName(shortWfName, nodeName, shortTemplateName, nodeID, POD_NAME_V1)).toEqual(v1podName); - expect(getPodName(shortWfName, nodeName, shortTemplateName, nodeID, '')).toEqual(v2podName); - expect(getPodName(shortWfName, nodeName, shortTemplateName, nodeID, undefined)).toEqual(v2podName); + const node = { + name: 'nodename', + id: '1', + templateName: shortTemplateName + } as unknown as NodeStatus; + const wf = { + metadata: { + name: shortWfName, + annotations: { + [ANNOTATION_KEY_POD_NAME_VERSION]: POD_NAME_V1 + } + } + } as unknown as Workflow; + + const v1podName = node.id; + const v2podName = `${shortWfName}-${shortTemplateName}-${createFNVHash(node.name)}`; + + expect(getPodName(wf, node)).toEqual(v1podName); + wf.metadata.annotations[ANNOTATION_KEY_POD_NAME_VERSION] = POD_NAME_V2; + expect(getPodName(wf, node)).toEqual(v2podName); + wf.metadata.annotations[ANNOTATION_KEY_POD_NAME_VERSION] = ''; + expect(getPodName(wf, node)).toEqual(v2podName); + delete wf.metadata.annotations; + expect(getPodName(wf, node)).toEqual(v2podName); - const name = getPodName(longWfName, nodeName, longTemplateName, nodeID, POD_NAME_V2); + wf.metadata.name = longWfName; + node.templateName = longTemplateName; + const name = getPodName(wf, node); expect(name.length).toEqual(maxK8sResourceNameLength); }); test('getTemplateNameFromNode', () => { // case: no template ref or template name // expect fallback to empty string - const nodeType: NodeType = 'Pod'; - const nodePhase: NodePhase = 'Succeeded'; - const retryStrategy: RetryStrategy = {}; - const outputs: Outputs = {}; - const inputs: Inputs = {}; - const memoizationStatus: MemoizationStatus = { - hit: false, - key: 'key', - cacheName: 'cache' - }; - - const node: NodeStatus = { - id: 'patch-processing-pipeline-ksp78-1623891970', - name: 'patch-processing-pipeline-ksp78.retriable-map-authoring-initializer', - displayName: 'retriable-map-authoring-initializer', - type: nodeType, - templateScope: 'local/', - phase: nodePhase, - boundaryID: '', - message: '', - startedAt: '', - finishedAt: '', - podIP: '', - daemoned: false, - retryStrategy, - outputs, - children: [], - outboundNodes: [], - templateName: '', - inputs, - hostNodeName: '', - memoizationStatus - }; - + const node = {} as unknown as NodeStatus; expect(getTemplateNameFromNode(node)).toEqual(''); // case: template ref defined but no template name defined - // expect to return templateRef.template node.templateRef = { name: 'test-template-name', template: 'test-template-template' @@ -88,7 +73,6 @@ describe('pod names', () => { expect(getTemplateNameFromNode(node)).toEqual(node.templateRef.template); // case: template name defined - // expect to return templateName node.templateName = 'test-template'; expect(getTemplateNameFromNode(node)).toEqual(node.templateName); }); diff --git a/ui/src/app/shared/pod-name.ts b/ui/src/app/shared/pod-name.ts index c365e9e032ee..d4dd1ac85f81 100644 --- a/ui/src/app/shared/pod-name.ts +++ b/ui/src/app/shared/pod-name.ts @@ -1,33 +1,40 @@ -import {NodeStatus} from '../../models'; +import {NodeStatus, Workflow} from '../../models'; +import {ANNOTATION_KEY_POD_NAME_VERSION} from './annotations'; export const POD_NAME_V1 = 'v1'; export const POD_NAME_V2 = 'v2'; export const maxK8sResourceNameLength = 253; export const k8sNamingHashLength = 10; +const maxPrefixLength = maxK8sResourceNameLength - k8sNamingHashLength; // getPodName returns a deterministic pod name // In case templateName is not defined or that version is explicitly set to POD_NAME_V1, it will return the nodeID (v1) // In other cases it will return a combination of workflow name, template name, and a hash (v2) // note: this is intended to be equivalent to the server-side Go code in workflow/util/pod_name.go -export function getPodName(workflowName: string, nodeName: string, templateName: string, nodeID: string, version: string): string { - if (version !== POD_NAME_V1 && templateName !== '') { - if (workflowName === nodeName) { - return workflowName; - } +export function getPodName(workflow: Workflow, node: NodeStatus): string { + const version = workflow.metadata?.annotations?.[ANNOTATION_KEY_POD_NAME_VERSION]; + if (version === POD_NAME_V1) { + return node.id; + } - const prefix = ensurePodNamePrefixLength(`${workflowName}-${templateName}`); + const workflowName = workflow.metadata.name; + if (workflowName === node.name) { + return workflowName; + } - const hash = createFNVHash(nodeName); - return `${prefix}-${hash}`; + const templateName = getTemplateNameFromNode(node); + let prefix = workflowName; + if (templateName) { + prefix += `-${templateName}`; } + prefix = ensurePodNamePrefixLength(prefix); - return nodeID; + const hash = createFNVHash(node.name); + return `${prefix}-${hash}`; } export function ensurePodNamePrefixLength(prefix: string): string { - const maxPrefixLength = maxK8sResourceNameLength - k8sNamingHashLength; - if (prefix.length > maxPrefixLength - 1) { return prefix.substring(0, maxPrefixLength - 1); } @@ -48,14 +55,6 @@ export function createFNVHash(input: string): number { } export function getTemplateNameFromNode(node: NodeStatus): string { - if (node.templateName && node.templateName !== '') { - return node.templateName; - } - // fall back to v1 pod names if no templateName or templateRef defined - if (node?.templateRef === undefined || node?.templateRef.template === '') { - return ''; - } - - return node.templateRef.template; + return node.templateName || node.templateRef?.template || ''; } diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index ee35494005c4..e6727ec2199d 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -4,8 +4,7 @@ import * as React from 'react'; import {useContext, useEffect, useRef, useState} from 'react'; import {RouteComponentProps} from 'react-router'; -import {archivalStatus, ArtifactRepository, execSpec, isArchivedWorkflow, isWorkflowInCluster, Link, NodeStatus, Parameter, Workflow} from '../../../../models'; -import {ANNOTATION_KEY_POD_NAME_VERSION} from '../../../shared/annotations'; +import {archivalStatus, ArtifactRepository, execSpec, isArchivedWorkflow, isWorkflowInCluster, Link, Parameter, Workflow} from '../../../../models'; import {artifactRepoHasLocation, findArtifact} from '../../../shared/artifacts'; import {uiUrl} from '../../../shared/base'; import {CostOptimisationNudge} from '../../../shared/components/cost-optimisation-nudge'; @@ -17,7 +16,7 @@ import {useCollectEvent} from '../../../shared/use-collect-event'; import {hasArtifactGCError, hasWarningConditionBadge} from '../../../shared/conditions-panel'; import {Context} from '../../../shared/context'; import {historyUrl} from '../../../shared/history'; -import {getPodName, getTemplateNameFromNode} from '../../../shared/pod-name'; +import {getPodName} from '../../../shared/pod-name'; import {RetryWatch} from '../../../shared/retry-watch'; import {services} from '../../../shared/services'; import {getResolvedTemplates} from '../../../shared/template-resolution'; @@ -475,18 +474,7 @@ export function WorkflowDetails({history, location, match}: RouteComponentProps< }); } - function ensurePodName(wf: Workflow, node: NodeStatus, nodeID: string): string { - if (workflow && node) { - const annotations = workflow.metadata.annotations || {}; - const version = annotations[ANNOTATION_KEY_POD_NAME_VERSION]; - const templateName = getTemplateNameFromNode(node); - return getPodName(wf.metadata.name, node.name, templateName, node.id, version); - } - - return nodeID; - } - - const podName = ensurePodName(workflow, selectedNode, nodeId); + const podName = workflow && selectedNode ? getPodName(workflow, selectedNode) : nodeId; const archived = isArchivedWorkflow(workflow); diff --git a/ui/src/app/workflows/components/workflow-logs-viewer/workflow-logs-viewer.tsx b/ui/src/app/workflows/components/workflow-logs-viewer/workflow-logs-viewer.tsx index ddf750baa001..1359e3a40e66 100644 --- a/ui/src/app/workflows/components/workflow-logs-viewer/workflow-logs-viewer.tsx +++ b/ui/src/app/workflows/components/workflow-logs-viewer/workflow-logs-viewer.tsx @@ -6,14 +6,13 @@ import {Observable} from 'rxjs'; import {map, publishReplay, refCount} from 'rxjs/operators'; import * as models from '../../../../models'; import {execSpec} from '../../../../models'; -import {ANNOTATION_KEY_POD_NAME_VERSION} from '../../../shared/annotations'; import {Button} from '../../../shared/components/button'; import {ErrorNotice} from '../../../shared/components/error-notice'; import {InfoIcon, WarningIcon} from '../../../shared/components/fa-icons'; import {Links} from '../../../shared/components/links'; import {Context} from '../../../shared/context'; import {useLocalStorage} from '../../../shared/use-local-storage'; -import {getPodName, getTemplateNameFromNode} from '../../../shared/pod-name'; +import {getPodName} from '../../../shared/pod-name'; import {ScopedLocalStorage} from '../../../shared/scoped-local-storage'; import {services} from '../../../shared/services'; import {FullHeightLogsViewer} from './full-height-logs-viewer'; @@ -157,9 +156,6 @@ export function WorkflowLogsViewer({workflow, initialNodeId, initialPodName, con return () => clearTimeout(x); }, [logFilter]); - const annotations = workflow.metadata.annotations || {}; - const podNameVersion = annotations[ANNOTATION_KEY_POD_NAME_VERSION]; - // map pod names to corresponding node IDs const podNamesToNodeIDs = new Map(); const podNames = [{value: '', label: 'All'}].concat( @@ -167,8 +163,7 @@ export function WorkflowLogsViewer({workflow, initialNodeId, initialPodName, con .filter(x => x.type === 'Pod') .map(targetNode => { const {name, id, displayName} = targetNode; - const templateName = getTemplateNameFromNode(targetNode); - const targetPodName = getPodName(workflow.metadata.name, name, templateName, id, podNameVersion); + const targetPodName = getPodName(workflow, targetNode); podNamesToNodeIDs.set(targetPodName, id); return {value: targetPodName, label: (displayName || name) + ' (' + targetPodName + ')'}; }) diff --git a/ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx b/ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx index a2fe4379304f..19c9113a59e8 100644 --- a/ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx +++ b/ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx @@ -5,7 +5,6 @@ import {useState} from 'react'; import * as models from '../../../../models'; import {Artifact, NodeStatus, Workflow} from '../../../../models'; -import {ANNOTATION_KEY_POD_NAME_VERSION} from '../../../shared/annotations'; import {Button} from '../../../shared/components/button'; import {ClipboardText} from '../../../shared/components/clipboard-text'; import {DurationPanel} from '../../../shared/components/duration-panel'; @@ -13,7 +12,7 @@ import {InlineTable} from '../../../shared/components/inline-table/inline-table' import {Links} from '../../../shared/components/links'; import {Phase} from '../../../shared/components/phase'; import {Timestamp} from '../../../shared/components/timestamp'; -import {getPodName, getTemplateNameFromNode} from '../../../shared/pod-name'; +import {getPodName} from '../../../shared/pod-name'; import {ResourcesDuration} from '../../../shared/resources-duration'; import {services} from '../../../shared/services'; import {getResolvedTemplates} from '../../../shared/template-resolution'; @@ -100,12 +99,7 @@ function DisplayWorkflowTime(props: {date: Date | string | number}) { function WorkflowNodeSummary(props: Props) { const {workflow, node} = props; - - const annotations = workflow.metadata.annotations || {}; - const version = annotations[ANNOTATION_KEY_POD_NAME_VERSION]; - const templateName = getTemplateNameFromNode(node); - - const podName = getPodName(workflow.metadata.name, node.name, templateName, node.id, version); + const podName = getPodName(workflow, node); const attributes = [ {title: 'NAME', value: }, diff --git a/workflow/util/pod_name.go b/workflow/util/pod_name.go index fceecf0b9b7f..b230d3777cc5 100644 --- a/workflow/util/pod_name.go +++ b/workflow/util/pod_name.go @@ -13,6 +13,7 @@ import ( const ( maxK8sResourceNameLength = 253 k8sNamingHashLength = 10 + maxPrefixLength = maxK8sResourceNameLength - k8sNamingHashLength ) // PodNameVersion stores which type of pod names should be used. @@ -70,8 +71,6 @@ func GeneratePodName(workflowName, nodeName, templateName, nodeID string, versio } func ensurePodNamePrefixLength(prefix string) string { - maxPrefixLength := maxK8sResourceNameLength - k8sNamingHashLength - if len(prefix) > maxPrefixLength-1 { return prefix[0 : maxPrefixLength-1] }