Skip to content

Commit

Permalink
fix(core): emit namespaceStatus events during provider init (#6759)
Browse files Browse the repository at this point in the history
Before this fix, `namespaceStatus` events weren't actually being emitted
during provider resolution, since the event bus on the plugin context
hadn't been set up to pipe `namespaceStatus` events to the main event
bus (this happened later in the flow).

This is the probable cause for some users experiencing dangling
namespaces that weren't being cleared up by Cloud's automatic
environment cleanup when module/action resolution failed
(since the namespace status events that would usually be emitted by the
"get status" and "deploy" handlers for those actions weren't being
emitted). This still needs to be verified, but most likely fixes the
issue.

This was fixed by setting up the event forwarding during the
initialization of the plugin event bus.
  • Loading branch information
thsig authored Jan 13, 2025
1 parent 4c83cd3 commit c704a35
Show file tree
Hide file tree
Showing 8 changed files with 85 additions and 48 deletions.
8 changes: 8 additions & 0 deletions core/src/plugin-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ export class PluginEventBroker extends EventEmitter<PluginEvents, PluginEventTyp
this.garden.events.onKey("_exit", this.abortHandler, garden.sessionId)
this.garden.events.onKey("_restart", this.abortHandler, garden.sessionId)

// Always pipe `namespaceStatus` events to the main event bus, since we need this to happen both during provider
// resolution (where `prepareEnvironment` is called, see `ResolveProviderTask`) and inside action handlers.
//
// Note: If any other plugin events without action-specific metadata are needed, they should be added here.
this.on("namespaceStatus", (status: NamespaceStatus) => {
this.garden.events.emit("namespaceStatus", status)
})

this.on("abort", () => {
this.aborted = true
})
Expand Down
15 changes: 4 additions & 11 deletions core/src/plugins/kubernetes/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
prepareNamespaces,
deleteNamespaces,
getNamespaceStatus,
clearNamespaceCache,
getSystemNamespace,
} from "./namespace.js"
import type { KubernetesPluginContext, KubernetesConfig, KubernetesProvider, ProviderSecretRef } from "./config.js"
Expand Down Expand Up @@ -40,6 +39,7 @@ import { getIngressApiVersion, supportedIngressApiVersions } from "./container/i
import type { Log } from "../../logger/log-entry.js"
import { ingressControllerInstall, ingressControllerReady } from "./nginx/ingress-controller.js"
import { styles } from "../../logger/styles.js"
import { isTruthy } from "../../util/util.js"

const dockerAuthSecretType = "kubernetes.io/dockerconfigjson"
const dockerAuthDocsLink = `
Expand Down Expand Up @@ -164,8 +164,6 @@ export async function prepareEnvironment(
await ingressControllerInstall(k8sCtx, log)
}

const nsStatus = await getNamespaceStatus({ ctx: k8sCtx, log, provider })
ctx.events.emit("namespaceStatus", nsStatus)
return { status: { ready: true, outputs: status.outputs } }
}

Expand Down Expand Up @@ -199,7 +197,7 @@ export async function cleanupEnvironment({
}
})
)
).filter(Boolean)
).filter(isTruthy)

if (namespacesToDelete.length === 0) {
return {}
Expand All @@ -212,18 +210,13 @@ export async function cleanupEnvironment({
nsDescription = `namespaces ${namespacesToDelete[0]} and ${namespacesToDelete[1]}`
}

const entry = log
const k8sLog = log
.createLog({
name: "kubernetes",
})
.info(`Deleting ${nsDescription} (this may take a while)`)

await deleteNamespaces(<string[]>namespacesToDelete, api, entry)

// Since we've deleted one or more namespaces, we invalidate the NS cache for this provider instance.
clearNamespaceCache(provider)

ctx.events.emit("namespaceStatus", { namespaceName: namespace, state: "missing", pluginName: provider.name })
await deleteNamespaces({ namespaces: namespacesToDelete, ctx, api, log: k8sLog })

return {}
}
Expand Down
25 changes: 3 additions & 22 deletions core/src/plugins/kubernetes/kubernetes-type/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import type { KubernetesPluginContext, KubernetesProvider } from "../config.js"
import { configureSyncMode, convertKubernetesModuleDevModeSpec } from "../sync.js"
import { apply, deleteObjectsBySelector } from "../kubectl.js"
import { streamK8sLogs } from "../logs.js"
import { getActionNamespace, getActionNamespaceStatus } from "../namespace.js"
import { deleteNamespaces, getActionNamespace, getActionNamespaceStatus } from "../namespace.js"
import { getForwardablePorts, killPortForwards } from "../port-forward.js"
import { getK8sIngresses } from "../status/ingress.js"
import type { ResourceStatus } from "../status/status.js"
Expand All @@ -28,13 +28,7 @@ import {
} from "../status/status.js"
import type { BaseResource, KubernetesResource, KubernetesServerResource, SyncableResource } from "../types.js"
import type { ManifestMetadata, ParsedMetadataManifestData } from "./common.js"
import {
convertServiceResource,
gardenNamespaceAnnotationValue,
getManifests,
getMetadataManifest,
parseMetadataResource,
} from "./common.js"
import { convertServiceResource, getManifests, getMetadataManifest, parseMetadataResource } from "./common.js"
import type { KubernetesModule } from "./module-config.js"
import { configureKubernetesModule } from "./module-config.js"
import { configureLocalMode, startServiceInLocalMode } from "../local-mode.js"
Expand Down Expand Up @@ -493,20 +487,7 @@ export const deleteKubernetesDeploy: DeployActionHandler<"delete", KubernetesDep
const [namespaceManifests, otherManifests] = partition(manifests, (m) => m.kind === "Namespace")

if (namespaceManifests.length > 0) {
await Promise.all(
namespaceManifests.map((ns) => {
const selector = `${gardenAnnotationKey("service")}=${gardenNamespaceAnnotationValue(ns.metadata.name)}`
return deleteObjectsBySelector({
log,
ctx,
provider,
namespace,
selector,
objectTypes: ["Namespace"],
includeUninitialized: false,
})
})
)
await deleteNamespaces({ namespaces: namespaceManifests.map((ns) => ns.metadata.name), api, ctx, log })
}
if (otherManifests.length > 0) {
await deleteObjectsBySelector({
Expand Down
24 changes: 23 additions & 1 deletion core/src/plugins/kubernetes/namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export async function ensureNamespace(
nsCache.set(providerUid, cache)
}
})
ctx.events.emit("namespaceStatus", { pluginName: ctx.provider.name, namespaceName: namespace.name, state: "ready" })

return result
}
Expand Down Expand Up @@ -303,7 +304,21 @@ export async function prepareNamespaces({ ctx, log }: GetEnvironmentStatusParams
}
}

export async function deleteNamespaces(namespaces: string[], api: KubeApi, log?: Log) {
/**
* Note: When possible, always use this helper to delete k8s namespaces, since that ensures that namespace status
* events are emitted and the provider namespace cache is cleared.
*/
export async function deleteNamespaces({
namespaces,
api,
ctx,
log,
}: {
namespaces: string[]
api: KubeApi
ctx: KubernetesPluginContext
log?: Log
}) {
for (const ns of namespaces) {
try {
await api.core.deleteNamespace({ name: ns })
Expand Down Expand Up @@ -338,6 +353,13 @@ export async function deleteNamespaces(namespaces: string[], api: KubeApi, log?:
})
}
}
if (namespaces.length > 0) {
for (const ns of namespaces) {
ctx.events.emit("namespaceStatus", { pluginName: ctx.provider.name, namespaceName: ns, state: "missing" })
}
// Since we've deleted one or more namespaces, we invalidate the NS cache for this provider instance.
clearNamespaceCache(ctx.provider)
}
}

export async function getActionNamespace({
Expand Down
5 changes: 0 additions & 5 deletions core/src/router/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import { getNames } from "../util/util.js"
import { defaultProvider } from "../config/provider.js"
import type { ConfigGraph } from "../graph/config-graph.js"
import { ActionConfigContext, ActionSpecContext } from "../config/template-contexts/actions.js"
import type { NamespaceStatus } from "../types/namespace.js"
import { TemplatableConfigContext } from "../config/template-contexts/project.js"
import type { ParamsBase } from "../plugin/handlers/base/base.js"
import { Profile } from "../util/profiling.js"
Expand Down Expand Up @@ -74,12 +73,8 @@ export abstract class BaseRouter {
events: PluginEventBroker | undefined
): Promise<PluginActionParamsBase> {
const provider = await this.garden.resolveProvider({ log, name: handler.pluginName })

const ctx = await this.garden.getPluginContext({ provider, templateContext, events })

// Forward plugin events that don't need any action-specific metadata (currently just `namespaceStatus` events).
ctx.events.on("namespaceStatus", (status: NamespaceStatus) => this.garden.events.emit("namespaceStatus", status))

return {
ctx,
log,
Expand Down
30 changes: 30 additions & 0 deletions core/test/integ/src/plugins/kubernetes/kubernetes-type/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,18 @@ describe("kubernetes-type handlers", () => {
expect(remoteResources[0].kind).to.equal("Deployment")
})

it("emits a namespaceStatus event for the app namespace", async () => {
const { deployParams } = await prepareActionDeployParams("module-simple", {})
garden.events.eventLog = []

await getKubernetesDeployStatus(deployParams)

const nsStatusEvent = garden.events.eventLog.find((e) => e.name === "namespaceStatus")

expect(nsStatusEvent).to.exist
expect(nsStatusEvent!.payload.namespaceName).to.eql("kubernetes-type-test-default")
})

it("should return missing status when metadata ConfigMap is missing", async () => {
const { resolvedAction, deployParams } = await prepareActionDeployParams("module-simple", {})

Expand Down Expand Up @@ -354,6 +366,18 @@ describe("kubernetes-type handlers", () => {
expect(remoteResources[0].metadata?.name).to.equal("busybox-deployment")
})

it("emits a namespaceStatus event for the app namespace", async () => {
const { deployParams } = await prepareActionDeployParams("module-simple", {})
garden.events.eventLog = []

await kubernetesDeploy(deployParams)

const nsStatusEvent = garden.events.eventLog.find((e) => e.name === "namespaceStatus")

expect(nsStatusEvent).to.exist
expect(nsStatusEvent!.payload.namespaceName).to.eql("kubernetes-type-test-default")
})

it("should successfully deploy when serviceResource doesn't have a containerModule", async () => {
const { deployParams } = await prepareActionDeployParams("module-simple", {})

Expand Down Expand Up @@ -644,13 +668,19 @@ describe("kubernetes-type handlers", () => {
action: ns2Graph.getDeploy("namespace-resource"),
force: false,
})
garden.events.eventLog = []

// This should only delete kubernetes-type-ns-2.
await garden.processTasks({ tasks: [deleteDeployTask], throwOnError: true })

expect(await getDeployedResource(ctx, ctx.provider, ns1Manifest!, log), "ns1resource").to.exist
expect(await getDeployedResource(ctx, ctx.provider, ns2Manifest!, log), "ns2resource").to.not.exist
expect(await getDeployedResource(ctx, ctx.provider, ns2Resource!, log), "ns2resource").to.not.exist

const nsStatusEvent = garden.events.eventLog.find(
(e) => e.name === "namespaceStatus" && e.payload.namespaceName === "kubernetes-type-ns-2"
)
expect(nsStatusEvent).to.exist
})

it("deletes all resources including a metadata ConfigMap describing what was last deployed", async () => {
Expand Down
15 changes: 10 additions & 5 deletions core/test/integ/src/plugins/kubernetes/namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ describe("Kubernetes Namespace helpers", () => {
})

describe("getNamespaceStatus", () => {
it("should return the namespace status and emit a namespace status event on the plugin event broker", async () => {
it("should return the namespace status and emit a namespace status event", async () => {
let namespaceStatusFromEvent: NamespaceStatus | null = null
const expecteedNamespaceName = "container-default"
const expectedNamespaceName = "container-default"
ctx.events.once("namespaceStatus", (s) => (namespaceStatusFromEvent = s))
const status = await getNamespaceStatus({
log,
Expand All @@ -58,14 +58,16 @@ describe("Kubernetes Namespace helpers", () => {
skipCreate: true,
})
expect(namespaceStatusFromEvent).to.exist
expect(namespaceStatusFromEvent!.namespaceName).to.eql(expecteedNamespaceName)
expect(namespaceStatusFromEvent!.namespaceName).to.eql(expectedNamespaceName)
expect(status).to.exist
expect(status.namespaceName).to.eql(expecteedNamespaceName)
expect(status.namespaceName).to.eql(expectedNamespaceName)
})
})

describe("ensureNamespace", () => {
it("should create the namespace if it doesn't exist, with configured annotations and labels", async () => {
it("should create the namespace if it doesn't exist, with configured annotations and labels and emit a namespace status event", async () => {
let namespaceStatusFromEvent: NamespaceStatus | null = null
ctx.events.once("namespaceStatus", (s) => (namespaceStatusFromEvent = s))
const namespace = {
name: namespaceName,
annotations: { foo: "bar" },
Expand All @@ -86,6 +88,9 @@ describe("Kubernetes Namespace helpers", () => {

expect(result.created).to.be.true
expect(result.patched).to.be.false

expect(namespaceStatusFromEvent).to.exist
expect(namespaceStatusFromEvent!.namespaceName).to.eql(namespaceName)
})

it("should add configured annotations if any are missing", async () => {
Expand Down
11 changes: 7 additions & 4 deletions core/test/integ/src/plugins/kubernetes/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ import { expect } from "chai"
import { KubeApi } from "../../../../../src/plugins/kubernetes/api.js"
import type { KubernetesPluginContext, KubernetesProvider } from "../../../../../src/plugins/kubernetes/config.js"

import { getDataDir, makeTestGarden } from "../../../../helpers.js"
import { getDataDir, makeTestGarden, type TestGarden } from "../../../../helpers.js"
import { getEnvironmentStatus, prepareEnvironment } from "../../../../../src/plugins/kubernetes/init.js"
import type { PrepareEnvironmentParams } from "../../../../../src/plugin/handlers/Provider/prepareEnvironment.js"
import type { Garden } from "../../../../../src/index.js"

async function ensureNamespaceDoesNotExist(api: KubeApi, namespaceName: string) {
if (!(await namespaceExists(api, namespaceName))) {
Expand All @@ -40,7 +39,7 @@ describe("kubernetes provider handlers", () => {
let log: Log
let ctx: KubernetesPluginContext
let api: KubeApi
let garden: Garden
let garden: TestGarden
const namespaceName = "kubernetes-provider-handler-test"

before(async () => {
Expand Down Expand Up @@ -68,7 +67,8 @@ describe("kubernetes provider handlers", () => {
expect(namespaceStatus).to.be.false
})

it("should prepare the environment with the prepareEnvironment handler", async () => {
it("should prepare the environment with the prepareEnvironment handler and emit a namespaceStatus event", async () => {
garden.events.eventLog = []
const status = await getEnvironmentStatus({ ctx, log })
const params: PrepareEnvironmentParams = {
ctx,
Expand All @@ -80,6 +80,9 @@ describe("kubernetes provider handlers", () => {
expect(envStatus.status.ready).to.be.true
const namespaceStatus = await namespaceExists(api, namespaceName)
expect(namespaceStatus).to.be.true
const namespaceStatusEvent = garden.events.eventLog.find((e) => e.name === "namespaceStatus")
expect(namespaceStatusEvent).to.exist
expect(namespaceStatusEvent?.payload.namespaceName).to.equal(namespaceName)
})
})
})

0 comments on commit c704a35

Please sign in to comment.