Bookends should always run

[d2lam]

Currently, if the build fails in the middle, bookends would not be executed. This shouldn't be the case. For example, if users store some deploy logs in the artifact folder. If the build breaks, they need to be able to look at the logs to debug. However, the screwdriver-artifact-bookend won't run since the build breaks.

[d2lam]

Verifying that it works: https://cd.screwdriver.cd/pipelines/122/builds/3708 The step exit fails, but it still runs the step stuff (since alwaysRun is set to true for this step) and also runs the bookend:

One thing is that the status of these steps are Failure. I think we should make those steps (for example, stuff and artifact-bookend) to be Success if runs successfully, while keeping the build status as Failure.

I can dig into this more if that is the expected behavior.

[petey]

Yeah, this should definitely keep the correct step status.

[d2lam]

Actually this is not working. Those steps were not run successfully (returning code 254). Need to look into this more.

[d2lam]

@minz1027 and I did some investigation on this.

• This traps the EXIT to echo an extra line for STEP_ID before exiting. However, it does not prevent the shell from exiting.
• The launcher will attempt to run the commands because alwaysRun is true(Notice how update step status is called for stuff, but not what). However, since the shell already exits, subsequent steps will not be able to execute.
• We don't have the solution to this yet. Any ideas?

[d2lam]

Consulted with @rphan about this. The trap route would not work for us. One way that could get this work is to have a wrapper script and this script would run all the steps. In wrapper.sh:

. /tmp/step1.sh
. /tmp/step2.sh      # if this step fails, step3 will still run
. /tmp/step3.sh

However, this will require a lot of refactor in launcher. Currently, each command is fed into the pty like this: https://github.com/screwdriver-cd/launcher/blob/master/executor/executor.go#L83

A few concerns:

• sh won't work. We need to switch to bash @stjohnjohnson are we ok with this? I remember we didn't want to use bash because sh is more standard.
• If we write a wrapper.sh that includes all the step scripts, we need to handle the update step status in between steps. And we would need to do this in script instead of go.
• Also, this basically means always run ALL the steps. For example, if step2 fails & step3 doesn't have "alwaysRun", it will still run in this case. We need to add some extra checks so that step3 doesn't run in this example.

@petey we should figure out the priority on this. It's going to be a big change. If we choose to not implement this yet, should we roll back the change? Technically, it won't affect anything. The alwaysRun would just not work.

[d2lam]

Discussed with the team and decided to roll back this feature. We should revisit this later.

[d2lam]

Reason to roll back: based on current implementation of launcher, it's impossible to stay in the shell if the step fails & exits.

[d2lam]

Update Dec 8, 2017 Taking another stab at this, following some of the ideas here: https://github.com/screwdriver-cd/screwdriver/blob/master/design/director.md

PR is opened: https://github.com/screwdriver-cd/launcher/pull/133

Implementation details:

• Setup steps and user steps are run in one single shell. These steps will only run if previous steps succeed. If there is error, the first error will be stored and returned after the teardown steps execute.
• After that, the shell will be killed and the teardown steps will be run in new shells. However, even if teardown bookend 1 fails, teardown bookend 2 will still run.
• Notes: environment variables set by the previous shell that runs user steps will NOT be passed to the teardown steps.

[jithine]

This now works as expected. Thanks @d2lam @minz1027