Skip to content
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.

Sign up for GitHub

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

[risk=low][no ticket] Logic changes for runtime cost display #8980

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[risk=low][no ticket] Logic changes for runtime cost display #8980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f910887
WIP
jmthibault79 Nov 19, 2024
fdd2788
derivePdFromAnalysisConfig
jmthibault79 Dec 3, 2024
7d6cb6a
runtime-cost changes
jmthibault79 Dec 3, 2024
fe660de
better
jmthibault79 Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions ui/src/app/components/apps-panel/runtime-cost.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ import * as React from 'react';
import { RuntimeStatus } from 'generated/fetch';

import { switchCase } from '@terra-ui-packages/core-utils';
import { toAnalysisConfig } from 'app/utils/analysis-config';
import { machineRunningCost, machineStorageCost } from 'app/utils/machines';
import {
ComputeType,
findMachineByName,
machineRunningCostPerHour,
machineStorageCostPerHour,
} from 'app/utils/machines';
import { formatUsd } from 'app/utils/numbers';
import { isVisible } from 'app/utils/runtime-utils';
import { runtimeDiskStore, runtimeStore, useStore } from 'app/utils/stores';
Expand All @@ -17,9 +21,29 @@ export const RuntimeCost = () => {
return null;
}

const analysisConfig = toAnalysisConfig(runtime, gcePersistentDisk);
const runningCost = formatUsd(machineRunningCost(analysisConfig));
const storageCost = formatUsd(machineStorageCost(analysisConfig));
const storageCostParams = {
dataprocConfig: runtime.dataprocConfig,
persistentDisk: gcePersistentDisk,
};

const machineType =
runtime.gceConfig?.machineType ??
runtime.gceWithPdConfig?.machineType ??
runtime.dataprocConfig.masterMachineType;

const runningCostParams = {
...storageCostParams,
computeType: runtime.dataprocConfig
? ComputeType.Dataproc
: ComputeType.Standard,
machine: findMachineByName(machineType),
gpuConfig:
// not available for dataproc
runtime.gceConfig?.gpuConfig ?? runtime.gceWithPdConfig?.gpuConfig,
};

const runningCost = formatUsd(machineRunningCostPerHour(runningCostParams));
const storageCost = formatUsd(machineStorageCostPerHour(storageCostParams));

// display running cost or stopped (storage) cost
// Error and Deleted statuses are not included because they're not "visible" [isVisible() = false]
Expand Down
6 changes: 3 additions & 3 deletions ui/src/app/components/common-env-conf-panels/confirm-delete-environment-with-pd.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { FlexRow } from 'app/components/flex';
import { ClrIcon } from 'app/components/icons';
import { RadioButton } from 'app/components/inputs';
import colors from 'app/styles/colors';
import { detachableDiskPricePerMonth } from 'app/utils/machines';
import { persistentDiskPricePerMonth } from 'app/utils/machines';
import { formatUsd } from 'app/utils/numbers';

import { BackupFilesHelpSection } from './backup-files-help-section';
Expand Down Expand Up @@ -80,7 +80,7 @@ export const ConfirmDeleteEnvironmentWithPD = ({
</p>
<p style={{ ...styles.confirmWarningText, gridColumn: 1, gridRow: 4 }}>
You will continue to incur persistent disk cost at{' '}
<b>{formatUsd(detachableDiskPricePerMonth(disk))}</b> per month. You
<b>{formatUsd(persistentDiskPricePerMonth(disk))}</b> per month. You
can delete your disk at any time via the {appType} configuration
panel.
</p>
Expand Down Expand Up @@ -178,7 +178,7 @@ export const ConfirmDeleteEnvironmentWithPD = ({
</p>
<p style={{ ...styles.confirmWarningText, gridColumn: 1, gridRow: 4 }}>
You will continue to incur persistent disk cost at{' '}
<b>{formatUsd(detachableDiskPricePerMonth(disk))}</b> per month.
<b>{formatUsd(persistentDiskPricePerMonth(disk))}</b> per month.
</p>
</div>
</div>
Expand Down
42 changes: 27 additions & 15 deletions ui/src/app/components/common-env-conf-panels/environment-cost-estimator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import { CSSProperties } from 'react';

import { cond } from '@terra-ui-packages/core-utils';
import { FlexColumn, FlexRow } from 'app/components/flex';
import { TooltipTrigger } from 'app/components/popups';
import colors from 'app/styles/colors';
import { reactStyles } from 'app/utils';
import { AnalysisConfig } from 'app/utils/analysis-config';
import {
detachableDiskPricePerMonth,
diskConfigPricePerMonth,
machineRunningCost,
derivePdFromAnalysisConfig,
machineRunningCostBreakdown,
machineStorageCost,
machineRunningCostPerHour,
machineStorageCostBreakdown,
machineStorageCostPerHour,
persistentDiskPricePerMonth,
RunningCost,
} from 'app/utils/machines';
import { formatUsd } from 'app/utils/numbers';

Expand Down Expand Up @@ -54,15 +54,29 @@ export const EnvironmentCostEstimator = ({
costTextColor = colors.accent,
style,
}: Props) => {
const { detachedDisk, diskConfig } = analysisConfig;
const { computeType, gpuConfig, machine, dataprocConfig } = analysisConfig;

// temp derive from analysisConfig
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my eventual goal is to stop using AnalysisConfig, but that's going to take some time

const persistentDisk = derivePdFromAnalysisConfig(analysisConfig);

const runningCostParams: RunningCost = {
dataprocConfig,
persistentDisk,
computeType,
gpuConfig,
machine,
};
const runningCost =
machineRunningCost(analysisConfig) +
machineRunningCostPerHour(runningCostParams) +
(isGKEApp ? GKE_APP_HOURLY_USD_COST_PER_WORKSPACE : 0);
const runningCostBreakdown = machineRunningCostBreakdown(analysisConfig);
const runningCostBreakdown = machineRunningCostBreakdown(runningCostParams);
const pausedCost =
machineStorageCost(analysisConfig) +
machineStorageCostPerHour({ dataprocConfig, persistentDisk }) +
(isGKEApp ? GKE_APP_HOURLY_USD_COST_PER_WORKSPACE : 0);
const pausedCostBreakdown = machineStorageCostBreakdown(analysisConfig);
const pausedCostBreakdown = machineStorageCostBreakdown({
dataprocConfig,
persistentDisk,
});

if (isGKEApp) {
const gkeBaseCost = `${formatUsd(
Expand All @@ -76,11 +90,9 @@ export const EnvironmentCostEstimator = ({
...styles.cost,
color: costTextColor,
};
const pdCost = cond(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good illustration of the complexity around how AnalysisConfig represents Persistent Disks.

  • if the diskConfig indicates that the disk is detachable, then the diskConfig represents a PD
  • else: the diskConfig represents a standard (non-persistent, non-detachable) disk
  • there may also be a detachedDisk. If so, that one is a persistent disk

My intention is to move the distinction between "detached" vs. otherwise to "persistent" vs. otherwise, which better reflects how we think about them today.

see the new derivePdFromAnalysisConfig() which captures this logic.

[diskConfig.detachable, () => diskConfigPricePerMonth(diskConfig)],
[!!detachedDisk, () => detachableDiskPricePerMonth(detachedDisk)],
() => 0
);
const pdCost = persistentDisk
? persistentDiskPricePerMonth(persistentDisk)
: 0;
return (
<FlexRow {...{ style }}>
<FlexColumn style={styles.costSection}>
Expand Down
1 change: 0 additions & 1 deletion ui/src/app/components/common-env-conf-panels/environment-informed-action-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ export const EnvironmentInformedActionPanel = ({
)}
<EnvironmentCostEstimator
{...{ analysisConfig }}
data-test-id='cost-estimator'
isGKEApp={appType !== UIAppType.JUPYTER}
style={costEstimatorStyle}
/>
Expand Down
18 changes: 12 additions & 6 deletions ui/src/app/components/runtime-configuration-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import { findCdrVersion } from 'app/utils/cdr-versions';
import {
ComputeType,
DATAPROC_MIN_DISK_SIZE_GB,
machineRunningCost,
derivePdFromAnalysisConfig,
machineRunningCostPerHour,
MIN_DISK_SIZE_GB,
} from 'app/utils/machines';
import {
Expand Down Expand Up @@ -197,7 +198,13 @@ export const getErrorsAndWarnings = ({
};

const runningCostErrors = validate(
{ currentRunningCost: machineRunningCost(analysisConfig) },
{
currentRunningCost: machineRunningCostPerHour({
...analysisConfig,
// temp derive from analysisConfig
persistentDisk: derivePdFromAnalysisConfig(analysisConfig),
}),
},
{
currentRunningCost: runningCostValidatorWithMessage(),
}
Expand Down Expand Up @@ -363,10 +370,9 @@ export const RuntimeConfigurationPanel = fp.flow(
(analysisConfig.computeType === ComputeType.Dataproc &&
!analysisConfig.diskConfig.detachable));

let runtimeCannotBeCreatedExplanation;
if (workspace.billingStatus !== BillingStatus.ACTIVE) {
runtimeCannotBeCreatedExplanation = BILLING_ACCOUNT_DISABLED_TOOLTIP;
}
const runtimeCannotBeCreatedExplanation =
workspace.billingStatus !== BillingStatus.ACTIVE &&
BILLING_ACCOUNT_DISABLED_TOOLTIP;

const runtimeCanBeUpdated =
runtimeCanBeCreated &&
Expand Down
6 changes: 6 additions & 0 deletions ui/src/app/components/runtime-configuration-panel/customize-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,15 @@ export const CustomizePanel = ({
gcePersistentDisk
);

const dataprocConfig = analysisConfig.dataprocConfig && {
...analysisConfig.dataprocConfig,
masterDiskSize: diskConfig.size,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was always correct to set this, but it was not needed because it's also captured in diskConfig. Now we need this because we use this value to calculate costs.

};

setAnalysisConfig({
...analysisConfig,
diskConfig,
dataprocConfig,
detachedDisk: diskConfig.detachable ? null : gcePersistentDisk,
});
}}
Expand Down
4 changes: 2 additions & 2 deletions ui/src/app/components/runtime-configuration-panel/offer-delete-disk-with-update.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { FlexRow } from 'app/components/flex';
import { ClrIcon } from 'app/components/icons';
import { RadioButton } from 'app/components/inputs';
import colors from 'app/styles/colors';
import { detachableDiskPricePerMonth } from 'app/utils/machines';
import { persistentDiskPricePerMonth } from 'app/utils/machines';
import { formatUsd } from 'app/utils/numbers';

const { useState, Fragment } = React;
Expand Down Expand Up @@ -69,7 +69,7 @@ export const OfferDeleteDiskWithUpdate = ({
Your disk will be saved for later and can be reattached when you
next configure a standard VM analysis environment. You will continue
to incur persistent disk cost at{' '}
<b>{formatUsd(detachableDiskPricePerMonth(disk))}</b> per month.
<b>{formatUsd(persistentDiskPricePerMonth(disk))}</b> per month.
</p>
</div>
<div style={styles.confirmWarning}>
Expand Down
46 changes: 4 additions & 42 deletions ui/src/app/utils/analysis-config.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {
DataprocConfig,
Disk,
DiskType,
GpuConfig,
Expand Down Expand Up @@ -813,19 +812,11 @@ describe(withAnalysisConfigDefaults.name, () => {
existingDiskName: null,
};

// yes it removes 3 fields. why?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this discrepancy

const expectedDataprocConfig: DataprocConfig = {
...inputConfig.dataprocConfig,
masterMachineType: undefined,
masterDiskSize: undefined,
numberOfWorkerLocalSSDs: undefined,
};

const outConfig = withAnalysisConfigDefaults(inputConfig, inputDisk);

expect(outConfig.computeType).toEqual(ComputeType.Dataproc);
expect(outConfig.diskConfig).toEqual(expectedDiskConfig);
expect(outConfig.dataprocConfig).toEqual(expectedDataprocConfig);
expect(outConfig.dataprocConfig).toEqual(inputConfig.dataprocConfig);
expect(outConfig.gpuConfig).toBeNull();
expect(outConfig.detachedDisk).toEqual(inputDisk);

Expand All @@ -839,48 +830,19 @@ describe(withAnalysisConfigDefaults.name, () => {
);
});

const replaceableFields = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all fields are now "replaceable" so we don't need this special logic

'numberOfWorkers',
'workerMachineType',
'workerDiskSize',
'numberOfPreemptibleWorkers',
];
it('replaces the replaceableFields with their defaults when dataprocConfig is missing', () => {
it('sets dataprocConfig to the preset default when it is missing', () => {
const inputConfig = {
...defaultAnalysisConfig,
computeType: ComputeType.Dataproc,
dataprocConfig: undefined,
};

const outConfig = withAnalysisConfigDefaults(inputConfig, undefined);

replaceableFields.forEach((field: string) =>
expect(outConfig.dataprocConfig[field]).toEqual(
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig[field]
)
expect(outConfig.dataprocConfig).toEqual(
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig
);
});

test.each(replaceableFields)(
"it replaces %s with the default when it's missing",
(field: string) => {
const inputConfig = {
...defaultAnalysisConfig,
computeType: ComputeType.Dataproc,
dataprocConfig: {
...defaultAnalysisConfig.dataprocConfig,
[field]: undefined,
},
};

const outConfig = withAnalysisConfigDefaults(inputConfig, undefined);

expect(outConfig.dataprocConfig[field]).toEqual(
runtimePresets().hailAnalysis.runtimeTemplate.dataprocConfig[field]
);
}
);

// same as Standard VM
it("should replace a missing diskConfig size with the persistent disk's when it exists", () => {
const inputConfig = {
Expand Down
6 changes: 6 additions & 0 deletions ui/src/app/utils/analysis-config.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,15 @@ export const withAnalysisConfigDefaults = (
dataprocConfig = {
numberOfWorkers:
dataprocConfig?.numberOfWorkers ?? defaults.numberOfWorkers,
masterMachineType:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, this did not set defaults for some fields. There was no clear reason for skipping these, and we do have defaults we can use for all of them. By adding them, we can simplify some logic.

dataprocConfig?.masterMachineType ?? defaults.masterMachineType,
masterDiskSize: dataprocConfig?.masterDiskSize ?? defaults.masterDiskSize,
workerMachineType:
dataprocConfig?.workerMachineType ?? defaults.workerMachineType,
workerDiskSize: dataprocConfig?.workerDiskSize ?? defaults.workerDiskSize,
numberOfWorkerLocalSSDs:
dataprocConfig?.numberOfWorkerLocalSSDs ??
defaults.numberOfWorkerLocalSSDs,
numberOfPreemptibleWorkers:
dataprocConfig?.numberOfPreemptibleWorkers ??
defaults.numberOfPreemptibleWorkers,
Expand Down
Loading