Wrapper startup-script sets startup-script-status to "done" instead of "error" when a wrapped startup script fails.

supersmo commented 2 years ago

Overview of the Issue

The Wrapper startup-script defined in startup.go: https://github.com/hashicorp/packer-plugin-googlecompute/blob/2378461bb1590bcfa1ce35d12fb84635b4d5b840/builder/googlecompute/startup.go#L16

sets the metadata startup-script-status to "done" instead of "error" when the wrapped script fails.

To fix it change: https://github.com/hashicorp/packer-plugin-googlecompute/blob/2378461bb1590bcfa1ce35d12fb84635b4d5b840/builder/googlecompute/startup.go#L55-L58

To something like:

if [ "$RETVAL" == "0" ]; then
  echo "Packer startup script done."
  SetMetadata %[2]s %[3]s
else
  echo "Packer startup script failed with return value: $RETVAL"
  SetMetadata %[2]s %[4]s
fi
exit $RETVAL
`, StartupWrappedScriptKey, StartupScriptStatusKey, StartupScriptStatusDone, StartupScriptStatusError)

Reproduction Steps

Create a startup-script that fails and let the builder wrap it. Trigger a packer build and notice that the startup-script-status is set to "done".

Plugin and Packer version

plugin: googlecompute 1.05

github-actions[bot] commented 1 year ago

This issue has been synced to JIRA for planning.

JIRA ID: HPR-864

nywilken commented 1 year ago

Hi @supersmo thanks for bubbling this up. Taking a look at the issue I can understand the confusion here, thanks for adding a potential fix to the script. Looking at the code it appears that the builder will continue to wait and eventually timeout if the status set in the Metadata is anything but "done". Which could be a reason why the status was never set correctly or it was truly a miss on our end.

That said, by updating the code to set the metadata status to "error", would you expect Packer to fail if the startup script resulted in a failure?

Seeing as the current behavior would have resulted in an a failed Packer build due to a timeout issue. I'm allowing Packer to fail if the wrapped startup script failed to prevent the user from thinking that everything executed without error. Please let me know if that is not what you think a user would expect.

nywilken commented 1 year ago

Thanks again for reporting, this should be fixed by #135 ; some test binaries are available via the link below if you would like to test the fix.

https://app.circleci.com/pipelines/github/hashicorp/packer-plugin-googlecompute/256/workflows/980ca1fe-50d9-4188-9351-f3bbbb730a08/jobs/2859/artifacts

supersmo commented 1 year ago

That said, by updating the code to set the metadata status to "error", would you expect Packer to fail if the startup script resulted in a failure?

Seeing as the current behavior would have resulted in an a failed Packer build due to a timeout issue. I'm allowing Packer to fail if the wrapped startup script failed to prevent the user from thinking that everything executed without error. Please let me know if that is not what you think a user would expect.

Yepp, I'd expect the Packer build to fail. 👍

We've switched over to os-login since this was reported so we're no longer using the statup metadata for our packing, but I trust that it now works 😊

heyealex commented 1 year ago

Is there an ETA on when a new version with this fix will be released?

hashicorp / packer-plugin-googlecompute