Skip to content

Commit

Permalink
fix(core): remove StageBuildTask from GetServiceStatusTask dependencies
Browse files Browse the repository at this point in the history
We shouldn't be staging the build when getting the service status as it
can remove build artifacts like those generated by the helm module. Had
to make adjustments to the kubernetes module getServiceStatus handler to
account for this change.
  • Loading branch information
eysi09 committed Feb 12, 2020
1 parent 52b4dcd commit a89d2e5
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 25 deletions.
33 changes: 25 additions & 8 deletions garden-service/src/plugins/kubernetes/kubernetes-module/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,20 @@ import { LogEntry } from "../../../logger/log-entry"
* Reads the manifests and makes sure each has a namespace set (when applicable) and adds annotations.
* Use this when applying to the cluster, or comparing against deployed resources.
*/
export async function getManifests(
api: KubeApi,
log: LogEntry,
module: KubernetesModule,
export async function getManifests({
api,
log,
module,
defaultNamespace,
readFromSrcDir = false,
}: {
api: KubeApi
log: LogEntry
module: KubernetesModule
defaultNamespace: string
): Promise<KubernetesResource[]> {
const manifests = await readManifests(module)
readFromSrcDir?: boolean
}): Promise<KubernetesResource[]> {
const manifests = await readManifests(module, log, readFromSrcDir)

return Bluebird.map(manifests, async (manifest) => {
// Ensure a namespace is set, if not already set, and if required by the resource type
Expand All @@ -50,11 +57,21 @@ export async function getManifests(

/**
* Read the manifests from the module config, as well as any referenced files in the config.
*
* @param module The kubernetes module to read manifests for.
* @param readFromSrcDir Whether or not to read the manifests from the module build dir or from the module source dir.
* In general we want to read from the build dir to ensure that manifests added via the `build.dependencies[].copy`
* field will be included. However, in some cases, e.g. when getting the service status, we can't be certain that
* the build has been staged and we therefore read the manifests from the source.
*
* TODO: Remove this once we're checking for kubernetes module service statuses with version hashes.
*/
export async function readManifests(module: KubernetesModule) {
export async function readManifests(module: KubernetesModule, log: LogEntry, readFromSrcDir = false) {
const fileManifests = flatten(
await Bluebird.map(module.spec.files, async (path) => {
const absPath = resolve(module.buildPath, path)
const manifestPath = readFromSrcDir ? module.path : module.buildPath
const absPath = resolve(manifestPath, path)
log.debug(`Reading manifest for module ${module.name} from path ${absPath}`)
return safeLoadAll((await readFile(absPath)).toString())
})
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ interface KubernetesStatusDetail {

export type KubernetesServiceStatus = ServiceStatus<KubernetesStatusDetail>

async function build({ module }: BuildModuleParams<KubernetesModule>): Promise<BuildResult> {
async function build({ module, log }: BuildModuleParams<KubernetesModule>): Promise<BuildResult> {
// Get the manifests here, just to validate that the files are there and are valid YAML
await readManifests(module)
await readManifests(module, log)
return { fresh: true }
}

Expand All @@ -71,7 +71,10 @@ async function getServiceStatus({
skipCreate: true,
})
const api = await KubeApi.factory(log, k8sCtx.provider)
const manifests = await getManifests(api, log, module, namespace)
// FIXME: We're currently reading the manifests from the module source dir (instead of build dir)
// because the build may not have been staged.
// This means that manifests added via the `build.dependencies[].copy` field will not be included.
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace, readFromSrcDir: true })

const { state, remoteResources } = await compareDeployedResources(k8sCtx, api, namespace, manifests, log)

Expand All @@ -98,7 +101,7 @@ async function deployService(params: DeployServiceParams<KubernetesModule>): Pro
provider: k8sCtx.provider,
})

const manifests = await getManifests(api, log, module, namespace)
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace })

const pruneSelector = getSelector(service)
await apply({ log, provider: k8sCtx.provider, manifests, pruneSelector })
Expand Down Expand Up @@ -130,7 +133,7 @@ async function deleteService(params: DeleteServiceParams): Promise<KubernetesSer
})
const provider = k8sCtx.provider
const api = await KubeApi.factory(log, provider)
const manifests = await getManifests(api, log, module, namespace)
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace })

await deleteObjectsBySelector({
log,
Expand All @@ -155,7 +158,7 @@ async function getServiceLogs(params: GetServiceLogsParams<KubernetesModule>) {
provider: k8sCtx.provider,
})
const api = await KubeApi.factory(log, provider)
const manifests = await getManifests(api, log, module, namespace)
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace })

return getAllLogs({ ...params, provider, defaultNamespace: namespace, resources: manifests })
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export async function runKubernetesTask(params: RunTaskParams<KubernetesModule>)

// Get the container spec to use for running
const { command, args } = task.spec
const manifests = await getManifests(api, log, module, namespace)
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace })
const resourceSpec = task.spec.resource || getServiceResourceSpec(module, undefined)
const target = await findServiceResource({
ctx: k8sCtx,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export async function testKubernetesModule(params: TestModuleParams<KubernetesMo
const api = await KubeApi.factory(log, k8sCtx.provider)

// Get the container spec to use for running
const manifests = await getManifests(api, log, module, namespace)
const manifests = await getManifests({ api, log, module, defaultNamespace: namespace })
const resourceSpec = testConfig.spec.resource || getServiceResourceSpec(module, undefined)
const target = await findServiceResource({ ctx: k8sCtx, log, manifests, module, resourceSpec, baseModule: undefined })
const container = getResourceContainer(target, resourceSpec.containerName)
Expand Down
10 changes: 1 addition & 9 deletions garden-service/src/tasks/get-service-status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { TaskResults } from "../task-graph"
import { prepareRuntimeContext } from "../runtime-context"
import { getTaskVersion, TaskTask } from "./task"
import Bluebird from "bluebird"
import { StageBuildTask } from "./stage-build"

export interface GetServiceStatusTaskParams {
garden: Garden
Expand Down Expand Up @@ -44,13 +43,6 @@ export class GetServiceStatusTask extends BaseTask {
async getDependencies() {
const deps = await this.graph.getDependencies({ nodeType: "deploy", name: this.getName(), recursive: false })

const stageBuildTask = new StageBuildTask({
garden: this.garden,
log: this.log,
module: this.service.module,
force: this.force,
})

const statusTasks = deps.deploy.map((service) => {
return new GetServiceStatusTask({
garden: this.garden,
Expand All @@ -74,7 +66,7 @@ export class GetServiceStatusTask extends BaseTask {
})
})

return [stageBuildTask, ...statusTasks, ...taskTasks]
return [...statusTasks, ...taskTasks]
}

getName() {
Expand Down
3 changes: 3 additions & 0 deletions garden-service/src/types/plugin/service/getServiceStatus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ export const getServiceStatus = {
Called ahead of any actions that expect a service to be running, as well as the
\`garden get status\` command.
NOTE: This handler should not use the build directory since it's not guaranteed
that the build will be staged or completed before this handler is called.
`,
paramsSchema: serviceActionParamsSchema.keys({
runtimeContext: runtimeContextSchema,
Expand Down

0 comments on commit a89d2e5

Please sign in to comment.