-
Notifications
You must be signed in to change notification settings - Fork 17
Add new step to automatically verify if the deployed container app started successfully #909
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
Changes from 47 commits
89dea14
a5ec82c
e8a0f76
f86fd9b
5e33e78
0c4a03c
d997c6b
7b106f1
30a0a20
05147cb
2658198
9b39261
adc3086
274dd77
9a84dfb
9a261c1
6348dab
3890f1f
9ac858e
d3958c4
c9106dc
d5abab8
a9e0c8b
0e14be3
372e08f
53c2dd0
a00c074
5403657
fcd5ea9
fe097b3
c76eb9d
b71ed6e
b61d516
95e2c11
be56465
2e03b28
2bcf2c1
3d059df
8509144
53fb1e4
8865693
4ed0174
756a2d1
890f296
c936518
febfc0a
85a5a16
6d25c9e
e1eed98
982ba44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { KnownRevisionProvisioningState, KnownRevisionRunningState, type ContainerAppsAPIClient, type Revision } from "@azure/arm-appcontainers"; | ||
| import { LogsQueryResultStatus, type LogsTable } from "@azure/monitor-query"; | ||
| import { parseAzureResourceId, uiUtils } from "@microsoft/vscode-azext-azureutils"; | ||
| import { AzureWizardExecuteStepWithActivityOutput, createSubscriptionContext, maskUserInfo, nonNullValueAndProp, parseError, type IParsedError, type LogActivityAttributes } from "@microsoft/vscode-azext-utils"; | ||
| import { type Progress } from "vscode"; | ||
| import { ext } from "../../../extensionVariables"; | ||
| import { type ContainerAppStartVerificationTelemetryProps } from "../../../telemetry/ContainerAppStartVerificationTelemetryProps"; | ||
| import { type SetTelemetryProps } from "../../../telemetry/SetTelemetryProps"; | ||
| import { createContainerAppsAPIClient, createLogsQueryClient } from "../../../utils/azureClients"; | ||
| import { delay } from "../../../utils/delay"; | ||
| import { localize } from "../../../utils/localize"; | ||
| import { type IngressContext } from "../../ingress/IngressContext"; | ||
| import { type ImageSourceContext } from "./ImageSourceContext"; | ||
|
|
||
| type ContainerAppStartVerificationContext = ImageSourceContext & IngressContext & SetTelemetryProps<ContainerAppStartVerificationTelemetryProps>; | ||
|
|
||
| /** | ||
| * Verifies that the recently deployed container app did not have any startup issues. | ||
| * | ||
| * Note: Sometimes an image builds and deploys successfully but fails to run. | ||
| * This leads to the Azure Container Apps service silently reverting to the last successful revision. | ||
| */ | ||
| export class ContainerAppStartVerificationStep<T extends ContainerAppStartVerificationContext> extends AzureWizardExecuteStepWithActivityOutput<T> { | ||
| public priority: number = 681; | ||
| public stepName: string = 'containerAppStartVerificationStep'; | ||
|
|
||
| private _client: ContainerAppsAPIClient; | ||
|
|
||
| protected getOutputLogSuccess = (context: T): string => localize('verifyContainerAppSuccess', 'Verified container app "{0}" deployment started successfully.', context.containerApp?.name); | ||
| protected getOutputLogFail = (context: T): string => localize('updateContainerAppFail', 'Failed to verify container app "{0}" deployment started successfully.', context.containerApp?.name); | ||
| protected getTreeItemLabel = (): string => localize('verifyContainerAppLabel', 'Verify container app deployment started successfully'); | ||
|
|
||
| public async execute(context: T, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> { | ||
| progress.report({ message: localize('verifyingContainerApp', 'Verifying container app startup status...') }); | ||
| const containerAppName: string = nonNullValueAndProp(context.containerApp, 'name'); | ||
|
|
||
| // Estimated time (n=1): 1s | ||
|
MicroFish91 marked this conversation as resolved.
|
||
| const revisionId: string | undefined = await this.waitAndGetRevisionId(context, 1000 * 10 /** maxWaitTimeMs */); | ||
| if (!revisionId) { | ||
| throw new Error(localize('revisionCheckTimeout', 'Status check timed out before retrieving the latest deployed container app revision.')); | ||
| } | ||
|
|
||
| // Estimated time (n=1): 20s | ||
| const revisionStatus: string | undefined = await this.waitAndGetRevisionStatus(context, revisionId, containerAppName, 1000 * 60 /** maxWaitTimeMs */); | ||
|
|
||
| const parsedResource = parseAzureResourceId(revisionId); | ||
| if (!revisionStatus) { | ||
| throw new Error(localize('revisionStatusTimeout', 'Status check timed out for the deployed container app revision "{0}".', parsedResource.resourceName)); | ||
| } else if (revisionStatus !== KnownRevisionRunningState.Running) { | ||
| try { | ||
| context.telemetry.properties.targetCloud = context.environment.name; | ||
|
|
||
| // Try to query and provide any logs to the LLM before throwing | ||
| await this.tryAddLogAttributes(context, parsedResource.resourceName); | ||
| context.telemetry.properties.addedContainerAppStartLogs = 'true'; | ||
| } catch (error) { | ||
| const perr: IParsedError = parseError(error); | ||
| ext.outputChannel.appendLog(localize('logQueryError', 'Error encountered while trying to verify container app revision logs through log query platform.')); | ||
| ext.outputChannel.appendLog(perr.message); | ||
| context.telemetry.properties.addedContainerAppStartLogs = 'false'; | ||
| context.telemetry.properties.getLogsQueryError = maskUserInfo(perr.message, []); | ||
| } | ||
|
|
||
| throw new Error(localize( | ||
| 'unexpectedRevisionState', | ||
| 'The deployed container app revision "{0}" has failed to start. If you are updating an existing container app, the service will try to revert to the previous working revision. Inspect the application logs to check for any known startup issues.', | ||
| parsedResource.resourceName, | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| public shouldExecute(context: T): boolean { | ||
| return !!context.containerApp; | ||
| } | ||
|
|
||
| private async waitAndGetRevisionId(context: T, maxWaitTimeMs: number): Promise<string | undefined> { | ||
| this._client ??= await createContainerAppsAPIClient([context, createSubscriptionContext(context.subscription)]); | ||
|
|
||
| const resourceGroupName: string = nonNullValueAndProp(context.containerApp, 'resourceGroup'); | ||
| const containerAppName: string = nonNullValueAndProp(context.containerApp, 'name'); | ||
|
|
||
| let revision: Revision | undefined; | ||
| let revisions: Revision[]; | ||
|
|
||
| const start: number = Date.now(); | ||
|
|
||
| while (true) { | ||
| if ((Date.now() - start) > maxWaitTimeMs) { | ||
| break; | ||
| } | ||
|
|
||
| await delay(1000); | ||
|
MicroFish91 marked this conversation as resolved.
Outdated
|
||
|
|
||
| revisions = await uiUtils.listAllIterator(this._client.containerAppsRevisions.listRevisions(resourceGroupName, containerAppName)); | ||
| revision = revisions.find(r => r.name === context.containerApp?.latestRevisionName && r.template?.containers?.[context.containersIdx ?? 0].image === context.image); | ||
|
|
||
| if (revision) { | ||
| return revision.id; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| private async waitAndGetRevisionStatus(context: T, revisionId: string, containerAppName: string, maxWaitTimeMs: number): Promise<string | undefined> { | ||
| this._client ??= await createContainerAppsAPIClient([context, createSubscriptionContext(context.subscription)]); | ||
| const parsedRevision = parseAzureResourceId(revisionId); | ||
|
|
||
| let revision: Revision; | ||
| const start: number = Date.now(); | ||
|
|
||
| while (true) { | ||
| if ((Date.now() - start) > maxWaitTimeMs) { | ||
| break; | ||
| } | ||
|
|
||
| await delay(2000); | ||
|
MicroFish91 marked this conversation as resolved.
Outdated
|
||
|
|
||
| revision = await this._client.containerAppsRevisions.getRevision(parsedRevision.resourceGroup, containerAppName, parsedRevision.resourceName); | ||
|
|
||
| if ( | ||
| revision.provisioningState === KnownRevisionProvisioningState.Deprovisioning || | ||
| revision.provisioningState === KnownRevisionProvisioningState.Provisioning || | ||
| revision.runningState === KnownRevisionRunningState.Processing || | ||
| revision.runningState === 'Activating' // For some reason this isn't listed in the known enum | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| return revision.runningState; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Try to query for any logs associated with the revision and add them to the Copilot activity attributes | ||
| */ | ||
| private async tryAddLogAttributes(context: T, revisionName: string) { | ||
| // Basic validation check since we're including a name directly in the query | ||
| if (revisionName.length > 54 || !/^[\w-]+$/.test(revisionName)) { | ||
| const invalidName: string = localize('unexpectedRevisionName', 'Internal warning: Encountered an unexpected revision name format "{0}". Skipping log query for the revision status check.', revisionName); | ||
| ext.outputChannel.appendLog(invalidName); | ||
| throw new Error(invalidName); | ||
|
nturinski marked this conversation as resolved.
|
||
| } | ||
|
|
||
| const workspaceId = context.managedEnvironment.appLogsConfiguration?.logAnalyticsConfiguration?.customerId; | ||
| if (!workspaceId) { | ||
| return; | ||
| } | ||
|
|
||
| const logsQueryClient = await createLogsQueryClient(context); | ||
| const query = ` | ||
| ContainerAppConsoleLogs_CL | ||
| | where RevisionName_s == "${revisionName}" | ||
| | project TimeGenerated, Stream_s, Log_s | ||
| | order by TimeGenerated desc | ||
| `; | ||
|
|
||
| const queryResult = await logsQueryClient.queryWorkspace(workspaceId, query, { | ||
| // <= 5 min ago (ISO 8601) | ||
| duration: 'PT5M' | ||
| }); | ||
|
|
||
| if (queryResult.status !== LogsQueryResultStatus.Success) { | ||
| return; | ||
| } | ||
|
|
||
| const lines: string[] = []; | ||
| const table: LogsTable = queryResult.tables[0]; | ||
|
|
||
| if (!table.rows.length) { | ||
| // Note: Often times we will only be able to find logs when the image source was for `RemoteAcrBuild` | ||
| throw new Error(localize('noQueryLogs', 'No query logs were found for revision "{0}".', revisionName)); | ||
|
MicroFish91 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| lines.push(table.columnDescriptors.map(c => c.name ?? '{columnName}').join(',')); | ||
| for (const row of table.rows) { | ||
| if (!Array.isArray(row)) { | ||
| continue; | ||
| } | ||
| lines.push(row.map(r => r instanceof Date ? r.toLocaleString() : String(r)).join(' ')); | ||
| } | ||
|
|
||
| const logs: LogActivityAttributes = { | ||
| name: 'Container App Console Logs', | ||
| description: `Container runtime logs for revision "${revisionName}" (<= 5 min ago). When a container app update was unsuccessful, these should be inspected to help identify the root cause.`, | ||
| content: lines.join('\n'), | ||
| }; | ||
|
|
||
| context.activityAttributes ??= {}; | ||
| context.activityAttributes.logs ??= []; | ||
| context.activityAttributes?.logs.push(logs); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { type Container, type Ingress } from "@azure/arm-appcontainers"; | ||
| import { AzureWizardExecuteStepWithActivityOutput, nonNullProp } from "@microsoft/vscode-azext-utils"; | ||
| import { AzureWizardExecuteStepWithActivityOutput, nonNullProp, type AzureWizardExecuteStep } from "@microsoft/vscode-azext-utils"; | ||
| import * as retry from "p-retry"; | ||
| import { type Progress } from "vscode"; | ||
| import { ext } from "../../../extensionVariables"; | ||
|
|
@@ -14,6 +14,7 @@ import { type IngressContext } from "../../ingress/IngressContext"; | |
| import { enabledIngressDefaults } from "../../ingress/enableIngress/EnableIngressStep"; | ||
| import { RegistryCredentialType } from "../../registryCredentials/RegistryCredentialsAddConfigurationListStep"; | ||
| import { updateContainerApp } from "../../updateContainerApp"; | ||
| import { ContainerAppStartVerificationStep } from "./ContainerAppStartVerificationStep"; | ||
| import { type ImageSourceContext } from "./ImageSourceContext"; | ||
| import { getContainerNameForImage } from "./containerRegistry/getContainerNameForImage"; | ||
|
|
||
|
|
@@ -87,4 +88,8 @@ export class ContainerAppUpdateStep<T extends ImageSourceContext & IngressContex | |
| public shouldExecute(context: T): boolean { | ||
| return !!context.containerApp; | ||
| } | ||
|
|
||
| public addExecuteSteps(): AzureWizardExecuteStep<T>[] { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason to not just add this step upfront when we create the wizard?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking, if this is just something that should always happen on create or update, there's no need to increase cognitive overhead where people need to remember to add this step. This just couples it directly to the steps that need it, and then no one ever needs to think about it again. The main argument against it in my mind is that it obfuscates some of the logic where you can't just easily look at the wizard steps on the main command page to figure out this is being run. That said, we already obfuscate a lot of steps like that with subWizards, so I figured it's not all that different.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, if the assumption of it always needing to run after create or update is wrong, then we should definitely decouple it. It's fairly low cost either way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can't think of many great arguments for or against. I was mostly thinking this as well:
While we do end up having to obfuscate some of these steps, they all tend to be conditional subwizards which unfortunately, we can't help but do. |
||
| return [new ContainerAppStartVerificationStep()]; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| export interface ContainerAppStartVerificationTelemetryProps { | ||
| /** | ||
| * Indicates the result of attempting to add logs during container app start verification. | ||
| * | ||
| * 1. `undefined` — Did not attempt to add logs. | ||
| * 2. `false` — Attempted to add logs but failed. | ||
| * 3. `true` — Successfully added logs. | ||
| */ | ||
| addedContainerAppStartLogs?: 'true' | 'false'; | ||
| getLogsQueryError?: string; | ||
| targetCloud?: string; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.