refactor: simplify getPodName and make consistent with back-end (ar…

…goproj#12964) Signed-off-by: Anton Gilgur <[email protected]>
zillow · Apr 23, 2024 · 671320d · 671320d
1 parent b1c3768
commit 671320d
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 100 deletions.
diff --git 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,66 +28,51 @@ 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'
         };
         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
@@ -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
@@ -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
@@ -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,18 +156,14 @@ 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<string, string>();
     const podNames = [{value: '', label: 'All'}].concat(
         Object.values(workflow.status.nodes || {})
             .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
@@ -5,15 +5,14 @@ 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';
 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: <ClipboardText text={props.node.name} />},

diff --git 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]
 	}