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.

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
Changes from 2 commits
Commits
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
19 changes: 16 additions & 3 deletions ui/src/app/components/apps-panel/runtime-cost.tsx
Original file line number Diff line number Diff line change
@@ -4,7 +4,10 @@ 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 {
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';
@@ -18,8 +21,18 @@ export const RuntimeCost = () => {
}

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

// display running cost or stopped (storage) cost
// Error and Deleted statuses are not included because they're not "visible" [isVisible() = false]
Original file line number Diff line number Diff line change
@@ -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';
@@ -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>
@@ -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>
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import { CSSProperties } from 'react';

import { cond } from '@terra-ui-packages/core-utils';
import { PersistentDiskRequest } from 'generated/fetch';

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';

@@ -54,15 +56,31 @@ export const EnvironmentCostEstimator = ({
costTextColor = colors.accent,
style,
}: Props) => {
const { detachedDisk, diskConfig } = analysisConfig;
const { computeType, gpuConfig, machine, numNodes, 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,
numNodes,
};
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(
@@ -76,11 +94,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}>
Original file line number Diff line number Diff line change
@@ -122,7 +122,6 @@ export const EnvironmentInformedActionPanel = ({
)}
<EnvironmentCostEstimator
{...{ analysisConfig }}
data-test-id='cost-estimator'
isGKEApp={appType !== UIAppType.JUPYTER}
style={costEstimatorStyle}
/>
18 changes: 12 additions & 6 deletions ui/src/app/components/runtime-configuration-panel.tsx
Original file line number Diff line number Diff line change
@@ -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 {
@@ -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(),
}
@@ -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 &&
Original file line number Diff line number Diff line change
@@ -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,
});
}}
Original file line number Diff line number Diff line change
@@ -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;
@@ -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}>
45 changes: 4 additions & 41 deletions ui/src/app/utils/analysis-config.spec.tsx
Original file line number Diff line number Diff line change
@@ -813,19 +813,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);

@@ -839,48 +831,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 = {
6 changes: 6 additions & 0 deletions ui/src/app/utils/analysis-config.tsx
Original file line number Diff line number Diff line change
@@ -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,
Loading