Add interruptible override to launch forms #chore (#417)

* Added interruptible override for executions Workflows and tasks can now have their interruptible flag set for a single execution Added indication about interruptible override to execution metadata * Interruptible override now includes indeterminate state Defaults to indeterminate state, allowing for executions to have their interruptible setting overriden to and Added text indicator for override status to checkbox label * Use released @flyteorg/flyteidl package * Added extra tests Signed-off-by: Nick Müller <[email protected]>
flyteorg · May 3, 2022 · 919e90c · 919e90c
1 parent 0590d1c
commit 919e90c
Show file tree

Hide file tree

Showing 18 changed files with 591 additions and 24 deletions.
diff --git a/package.json b/package.json
@@ -80,7 +80,7 @@
         "@commitlint/cli": "^8.3.5",
         "@commitlint/config-conventional": "^8.3.4",
         "@date-io/moment": "1.3.9",
-        "@flyteorg/flyteidl": "0.24.11",
+        "@flyteorg/flyteidl": "1.1.0",
         "@material-ui/core": "^4.0.0",
         "@material-ui/icons": "^4.0.0",
         "@material-ui/pickers": "^3.2.2",

diff --git a/src/components/Executions/ExecutionDetails/ExecutionMetadataExtra.tsx b/src/components/Executions/ExecutionDetails/ExecutionMetadataExtra.tsx
@@ -6,6 +6,7 @@ import { Execution } from 'models/Execution/types';
 import * as React from 'react';
 import { getLaunchPlan } from 'models/Launch/api';
 import { LaunchPlanSpec } from 'models/Launch/types';
+import { dashedValueString } from 'common/constants';
 import { ExecutionMetadataLabels } from './constants';
 
 const useStyles = makeStyles((theme: Theme) => {
@@ -39,6 +40,7 @@ export const ExecutionMetadataExtra: React.FC<{
     maxParallelism,
     rawOutputDataConfig,
     securityContext,
+    interruptible,
   } = execution.spec;
 
   const [launchPlanSpec, setLaunchPlanSpec] = React.useState<Partial<LaunchPlanSpec>>({});
@@ -67,6 +69,10 @@ export const ExecutionMetadataExtra: React.FC<{
       label: ExecutionMetadataLabels.parallelism,
       value: maxParallelism,
     },
+    {
+      label: ExecutionMetadataLabels.interruptible,
+      value: interruptible ? (interruptible.value ? 'true' : 'false') : dashedValueString,
+    },
   ];
 
   return (

diff --git a/src/components/Executions/ExecutionDetails/RelaunchExecutionForm.tsx b/src/components/Executions/ExecutionDetails/RelaunchExecutionForm.tsx
@@ -35,6 +35,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
             annotations,
             authRole,
             securityContext,
+            interruptible,
           },
         } = execution;
 
@@ -59,6 +60,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
           annotations,
           authRole,
           securityContext,
+          interruptible,
         };
       },
     },
@@ -74,7 +76,7 @@ function useRelaunchTaskFormState({ execution }: RelaunchExecutionFormProps) {
       defaultValue: {} as TaskInitialLaunchParameters,
       doFetch: async (execution) => {
         const {
-          spec: { authRole, launchPlan: taskId },
+          spec: { authRole, launchPlan: taskId, interruptible },
         } = execution;
         const task = await apiContext.getTask(taskId);
         const inputDefinitions = getTaskInputs(task);
@@ -85,7 +87,7 @@ function useRelaunchTaskFormState({ execution }: RelaunchExecutionFormProps) {
           },
           apiContext,
         );
-        return { authRole, values, taskId };
+        return { authRole, values, taskId, interruptible };
       },
     },
     execution,

diff --git a/src/components/Executions/ExecutionDetails/constants.ts b/src/components/Executions/ExecutionDetails/constants.ts
@@ -10,6 +10,7 @@ export enum ExecutionMetadataLabels {
   rawOutputPrefix = 'Raw Output Prefix',
   parallelism = 'Parallelism',
   securityContextDefault = 'default',
+  interruptible = 'Interruptible override',
 }
 
 export const tabs = {

diff --git a/src/components/Executions/ExecutionDetails/test/ExecutionMetadata.test.tsx b/src/components/Executions/ExecutionDetails/test/ExecutionMetadata.test.tsx
@@ -1,5 +1,6 @@
 import { render } from '@testing-library/react';
 import { dashedValueString } from 'common/constants';
+import { Protobuf } from 'flyteidl';
 import { Execution, WorkflowExecutionIdentifier } from 'models/Execution/types';
 import { createMockExecution } from 'models/__mocks__/executionsData';
 import * as React from 'react';
@@ -11,6 +12,7 @@ import { ExecutionMetadata } from '../ExecutionMetadata';
 const clusterTestId = `metadata-${ExecutionMetadataLabels.cluster}`;
 const startTimeTestId = `metadata-${ExecutionMetadataLabels.time}`;
 const durationTestId = `metadata-${ExecutionMetadataLabels.duration}`;
+const interruptibleTestId = `metadata-${ExecutionMetadataLabels.interruptible}`;
 
 jest.mock('models/Launch/api', () => ({
   getLaunchPlan: jest.fn(() => Promise.resolve({ spec: {} })),
@@ -75,4 +77,22 @@ describe('ExecutionMetadata', () => {
       Routes.ExecutionDetails.makeUrl(referenceExecution),
     );
   });
+
+  it('shows true if execution was marked as interruptible', () => {
+    execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
+    const { getByTestId } = renderMetadata();
+    expect(getByTestId(interruptibleTestId)).toHaveTextContent('true');
+  });
+
+  it('shows false if execution was not marked as interruptible', () => {
+    execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
+    const { getByTestId } = renderMetadata();
+    expect(getByTestId(interruptibleTestId)).toHaveTextContent('false');
+  });
+
+  it('shows dashes if no interruptible value is found in execution spec', () => {
+    delete execution.spec.interruptible;
+    const { getByTestId } = renderMetadata();
+    expect(getByTestId(interruptibleTestId)).toHaveTextContent(dashedValueString);
+  });
 });
diff --git a/src/components/Executions/ExecutionDetails/test/RelaunchExecutionForm.test.tsx b/src/components/Executions/ExecutionDetails/test/RelaunchExecutionForm.test.tsx
@@ -9,7 +9,7 @@ import {
 } from 'components/Launch/LaunchForm/utils';
 import { mockSimpleVariables } from 'components/Launch/LaunchForm/__mocks__/mockInputs';
 import { primitiveLiteral } from 'components/Launch/LaunchForm/__mocks__/utils';
-import { Admin } from 'flyteidl';
+import { Admin, Protobuf } from 'flyteidl';
 import { LiteralMap, ResourceType, Variable } from 'models/Common/types';
 import { getExecutionData } from 'models/Execution/api';
 import { Execution, ExecutionData } from 'models/Execution/types';
@@ -155,6 +155,39 @@ describe('RelaunchExecutionForm', () => {
         }),
       });
     });
+
+    it('should not set interruptible value if not provided', async () => {
+      delete execution.spec.interruptible;
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: undefined,
+        }),
+      });
+    });
+
+    it('should have correct interruptible value if override is enabled', async () => {
+      execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: Protobuf.BoolValue.create({ value: true }),
+        }),
+      });
+    });
+
+    it('should have correct interruptible value if override is disabled', async () => {
+      execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: Protobuf.BoolValue.create({ value: false }),
+        }),
+      });
+    });
   });
 
   describe('Launch form with full inputs', () => {
@@ -256,5 +289,38 @@ describe('RelaunchExecutionForm', () => {
         }),
       });
     });
+
+    it('should not set interruptible value if not provided', async () => {
+      delete execution.spec.interruptible;
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: undefined,
+        }),
+      });
+    });
+
+    it('should have correct interruptible value if override is enabled', async () => {
+      execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: Protobuf.BoolValue.create({ value: true }),
+        }),
+      });
+    });
+
+    it('should have correct interruptible value if override is disabled', async () => {
+      execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
+      const { getByText } = renderForm();
+      await waitFor(() => expect(getByText(mockContentString)));
+      checkLaunchFormProps({
+        initialParameters: expect.objectContaining({
+          interruptible: Protobuf.BoolValue.create({ value: false }),
+        }),
+      });
+    });
   });
 });
diff --git a/src/components/Launch/LaunchForm/LaunchInterruptibleInput.tsx b/src/components/Launch/LaunchForm/LaunchInterruptibleInput.tsx
@@ -0,0 +1,112 @@
+import { makeStyles, Theme, Typography } from '@material-ui/core';
+import FormControlLabel from '@material-ui/core/FormControlLabel';
+import Checkbox from '@material-ui/core/Checkbox';
+import * as React from 'react';
+import { Protobuf } from 'flyteidl';
+import { useStyles } from './styles';
+import { LaunchInterruptibleInputRef } from './types';
+import { formStrings } from './constants';
+
+export const useInterruptibleStyles = makeStyles((theme: Theme) => ({
+  labelIndeterminate: {
+    color: theme.palette.grey[500],
+  },
+}));
+
+const isValueValid = (value: any) => {
+  return value !== undefined && value !== null;
+};
+
+interface LaunchInterruptibleInputProps {
+  initialValue?: Protobuf.IBoolValue | null;
+}
+
+export const LaunchInterruptibleInputImpl: React.ForwardRefRenderFunction<
+  LaunchInterruptibleInputRef,
+  LaunchInterruptibleInputProps
+> = (props, ref) => {
+  // interruptible stores the override to enable/disable the setting for an execution
+  const [interruptible, setInterruptible] = React.useState(false);
+  // indeterminate tracks whether the interruptible flag is unspecified/indeterminate (true) or an override has been selected (false)
+  const [indeterminate, setIndeterminate] = React.useState(true);
+
+  React.useEffect(() => {
+    if (isValueValid(props.initialValue) && isValueValid(props.initialValue!.value)) {
+      setInterruptible(() => props.initialValue!.value!);
+      setIndeterminate(() => false);
+    } else {
+      setInterruptible(() => false);
+      setIndeterminate(() => true);
+    }
+  }, [props.initialValue?.value]);
+
+  const handleInputChange = React.useCallback(() => {
+    if (indeterminate) {
+      setInterruptible(() => true);
+      setIndeterminate(() => false);
+    } else if (interruptible) {
+      setInterruptible(() => false);
+      setIndeterminate(() => false);
+    } else {
+      setInterruptible(() => false);
+      setIndeterminate(() => true);
+    }
+  }, [interruptible, indeterminate]);
+
+  React.useImperativeHandle(
+    ref,
+    () => ({
+      getValue: () => {
+        if (indeterminate) {
+          return null;
+        }
+
+        return Protobuf.BoolValue.create({ value: interruptible });
+      },
+      validate: () => true,
+    }),
+    [interruptible, indeterminate],
+  );
+
+  const styles = useStyles();
+  const interruptibleStyles = useInterruptibleStyles();
+
+  const getInterruptibleLabel = () => {
+    if (indeterminate) {
+      return (
+        <Typography
+          className={interruptibleStyles.labelIndeterminate}
+        >{`${formStrings.interruptible} (no override)`}</Typography>
+      );
+    } else if (interruptible) {
+      return <Typography>{`${formStrings.interruptible} (enabled)`}</Typography>;
+    }
+    return <Typography>{`${formStrings.interruptible} (disabled)`}</Typography>;
+  };
+
+  return (
+    <section>
+      <header className={styles.sectionHeader}>
+        <Typography variant="h6">Override interruptible flag</Typography>
+        <Typography variant="body2">
+          Overrides the interruptible flag of a workflow for a single execution, allowing it to be
+          forced on or off. If no value was selected, the workflow's default will be used.
+        </Typography>
+      </header>
+      <section title={formStrings.interruptible}>
+        <FormControlLabel
+          control={
+            <Checkbox
+              checked={interruptible}
+              indeterminate={indeterminate}
+              onChange={handleInputChange}
+            />
+          }
+          label={getInterruptibleLabel()}
+        />
+      </section>
+    </section>
+  );
+};
+
+export const LaunchInterruptibleInput = React.forwardRef(LaunchInterruptibleInputImpl);
diff --git a/src/components/Launch/LaunchForm/LaunchTaskForm.tsx b/src/components/Launch/LaunchForm/LaunchTaskForm.tsx
@@ -7,6 +7,7 @@ import { LaunchFormHeader } from './LaunchFormHeader';
 import { LaunchFormInputs } from './LaunchFormInputs';
 import { LaunchState } from './launchMachine';
 import { LaunchRoleInput } from './LaunchRoleInput';
+import { LaunchInterruptibleInput } from './LaunchInterruptibleInput';
 import { SearchableSelector } from './SearchableSelector';
 import { useStyles } from './styles';
 import { BaseInterpretedLaunchState, BaseLaunchService, LaunchTaskFormProps } from './types';
@@ -15,8 +16,14 @@ import { isEnterInputsState } from './utils';
 
 /** Renders the form for initiating a Launch request based on a Task */
 export const LaunchTaskForm: React.FC<LaunchTaskFormProps> = (props) => {
-  const { formInputsRef, roleInputRef, state, service, taskSourceSelectorState } =
-    useLaunchTaskFormState(props);
+  const {
+    formInputsRef,
+    roleInputRef,
+    interruptibleInputRef,
+    state,
+    service,
+    taskSourceSelectorState,
+  } = useLaunchTaskFormState(props);
   const styles = useStyles();
   const baseState = state as BaseInterpretedLaunchState;
   const baseService = service as BaseLaunchService;
@@ -63,6 +70,10 @@ export const LaunchTaskForm: React.FC<LaunchTaskFormProps> = (props) => {
           />
         ) : null}
         <LaunchFormInputs key={formKey} ref={formInputsRef} state={baseState} variant="task" />
+        <LaunchInterruptibleInput
+          initialValue={state.context.interruptible}
+          ref={interruptibleInputRef}
+        />
       </DialogContent>
       <LaunchFormActions state={baseState} service={baseService} onClose={props.onClose} />
     </>

diff --git a/src/components/Launch/LaunchForm/LaunchWorkflowForm.tsx b/src/components/Launch/LaunchForm/LaunchWorkflowForm.tsx
@@ -13,13 +13,15 @@ import { useLaunchWorkflowFormState } from './useLaunchWorkflowFormState';
 import { isEnterInputsState } from './utils';
 import { LaunchRoleInput } from './LaunchRoleInput';
 import { LaunchFormAdvancedInputs } from './LaunchFormAdvancedInputs';
+import { LaunchInterruptibleInput } from './LaunchInterruptibleInput';
 
 /** Renders the form for initiating a Launch request based on a Workflow */
 export const LaunchWorkflowForm: React.FC<LaunchWorkflowFormProps> = (props) => {
   const {
     formInputsRef,
     roleInputRef,
     advancedOptionsRef,
+    interruptibleInputRef,
     state,
     service,
     workflowSourceSelectorState,
@@ -105,6 +107,10 @@ export const LaunchWorkflowForm: React.FC<LaunchWorkflowFormProps> = (props) =>
               />
             ) : null}
             <LaunchFormAdvancedInputs ref={advancedOptionsRef} state={state} />
+            <LaunchInterruptibleInput
+              initialValue={state.context.interruptible}
+              ref={interruptibleInputRef}
+            />
           </AccordionDetails>
         </Accordion>
       </DialogContent>

diff --git a/src/components/Launch/LaunchForm/constants.ts b/src/components/Launch/LaunchForm/constants.ts
@@ -10,6 +10,7 @@ export const formStrings = {
   title: 'Create New Execution',
   workflowVersion: 'Workflow Version',
   launchPlan: 'Launch Plan',
+  interruptible: 'Interruptible',
 };
 
 export const AuthRoleStrings: { [k in AuthRoleTypes]: AuthRoleMeta } = {