Azure-pipelines-tasks: Cache post-job should respect jobs execute condition

[fabianpittroff]

Required Information

Question, Bug, or Feature?
Type: Bug

Enter Task Name: CacheBeta/Cache

Environment

• Agent - Hosted and Private

Private

• macOS Mojave
• Current agent version: '2.154.1'

Issue Description

Disabling the CacheBeta/Cache job per conditions, doesn't include the post-job. The post-job runs always, combined with the error #10906.

The job is defined inside of a task group aswell.

Task logs

Expected behavior

The post-job cache shouldn't run, if the cache job itself is disabled per condition.

Troubleshooting

-/-

Error logs

-/-

For reference here the PR.

[willsmythe]

Good suggestion.

The docs indicate that the post-job "save" step will always runs if the job's overall status is "successful", but this is a good case where better controls are needed.

A few possible options:

1. Copy/reuse the condition from the step (which defaults to succeeded()) on the post-job (save) step. This addresses the requirement and probably makes sense in most cases.

2. Surface a saveCondition input (or ideally a property next to the standard condition property). If specified, this value would be evaluated like a condition on any other step. This enables the case where a job only needs to consume a cache, never create it. Note: we have talked about solving this scenario in a different way: an action input that has three options: restore, save, and restore and save (the default). We should keep this in mind as we think through the solution here.

I tend to think option 1 makes the most sense (alongside an optional action input), but we should iterate ...

[johnterickson]

@bryanmacfarlane Is there a recommended way to implement #1 from above?

[johnterickson]

@fabianpittroff thanks for reporting this!

[willsmythe]

@johnterickson / @bryanmacfarlane - I looked into this when we moved to the combined cache task. Currently the agent sets the condition for all post-job steps to always (see https://github.com/microsoft/azure-pipelines-agent/blob/master/src/Agent.Worker/JobExtension.cs#L187). This probably makes sense for most post-job steps, but not for caching since you generally don't want to save a bad cache.

Ideally there would be a way in the task.json to tell the agent to use the task's execution condition for its post job execution step. The condition would then be either succeeded() or whatever was set on the step's condition.

Doing this would also avoid the need for the logic in the plugin that checks the build status and bails out if not succeeded ...

[johnterickson]

@willsmythe Should this be a warning?
https://github.com/microsoft/azure-pipelines-agent/blob/4121da4b1e3ae70143c22a34c896842d4f1fac39/src/Agent.Plugins/PipelineCache/SavePipelineCacheV0.cs#L48-L52

[fadnavistanmay]

This is released in agent 2.155.0 version Closing the issue.

[ncw]

I don't think this fix is working...

On this build

2019-08-05T11:13:56.9671795Z Current agent version: '2.155.1'

I see

So the cache step is skipped, but the post step still runs

This is using a condition with the cache, eg

  - task: CacheBeta@0
    continueOnError: true
    inputs:
      key: go-build-cache | $(Agent.JobName)
      path: $(GOCACHE)
    displayName: Cache go build
    condition: ne( variables['GOCACHE'], '' )

Where GOCACHE is set in variables and overridden on a task by task basis.

Example build: https://dev.azure.com/rclone/rclone/_build/results?buildId=27&view=logs

Full yml file:
azure-pipelines.yml.txt

[willsmythe]

@fadnavistanmay - the logic in the "save" plugin that checks job status, etc needs to be moved ahead of the fingerprint calculation. We also should really consider a better solution that involves the agent actually evaluating the post step's condition so the save plugin logic is never even called.

@ncw - thanks for reporting. A short-term workaround would be to wrap the $(Agent.JobName) variable in double quotes so it gets treated like a string literal, instead of as a filename. Like this:

  - task: CacheBeta@0
    continueOnError: true
    inputs:
      key: go-build-cache | "$(Agent.JobName)"
      path: $(GOCACHE)
    displayName: Cache go build
    condition: ne( variables['GOCACHE'], '' )

[ncw]

@willsmythe wrote:

thanks for reporting. A short-term workaround would be to wrap the $(Agent.JobName) variable in double quotes so it gets treated like a string literal, instead of as a filename. Like this:

  - task: CacheBeta@0
    continueOnError: true
    inputs:
      key: go-build-cache | "$(Agent.JobName)"
      path: $(GOCACHE)
    displayName: Cache go build
    condition: ne( variables['GOCACHE'], '' )

That works - thank you very much :-)

I think the docs need improving here - it isn't clear to me what is a file and what is a string!

I think the example is probably wrong...

Isn't $(Agent.OS) referring to a file here?

[takluyver]

With the cache step skipped, the save step now shows

##[warning]Skipping because restore step did not run.

Is it possible to remove the warning? We get a count of warnings showing up on Github like this:

But this isn't pointing to any potential problem.

[johnterickson]

Thanks for pointing this out @takluyver!
@fadnavistanmay Please change that line to Info level

[fadnavistanmay]

Thanks. Modifying in this PR https://github.com/microsoft/azure-pipelines-agent/pull/2535

[fadnavistanmay]

This is releases as part of the agent 2.160.0 Thanks

[APIWT]

@fadnavistanmay We are running into this issue. How do we check what version of the agent we are on? Also, does it help that we are using Hosted Agents? Thanks!