Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Print pending actions during apply and destroy #6032

Closed
wants to merge 1 commit into from
Closed

Print pending actions during apply and destroy #6032

wants to merge 1 commit into from

Conversation

glasser
Copy link
Contributor

@glasser glasser commented Apr 6, 2016

During apply and destroy commands, prints the current set of
resources if no apply step has started or stopped within ten seconds.

Fixes #5932.

@glasser
Copy link
Contributor Author

glasser commented Apr 6, 2016

Caveats: There are no tests (are there tests anywhere of UiHook?) and the timeout is not configurable (would anyone want to configure it?).

This was referenced Apr 6, 2016
Closed
Merged
@glasser
Copy link
Contributor Author

glasser commented Apr 6, 2016

I suppose it might be cleaner to do this as a separate Hook from UiHook... Thoughts?

@glasser
Copy link
Contributor Author

glasser commented Apr 6, 2016

I updated this to be a separate hook, which seemed cleaner.

If it's important that UiHook know that its Ui is concurrent, maybe it can do some sort of type check?

By the way, while testing this, I noticed that sometimes the last few "Destruction complete" messages didn't appear and the countHook "destroyed" count was too low! I don't think this could be caused by this change, though...

@glasser
Copy link
Contributor Author

glasser commented Apr 6, 2016

Looking into test failure.

@glasser
Copy link
Contributor Author

glasser commented Apr 6, 2016

Resolved test failure.

glasser
glasser reviewed Apr 11, 2016
View reviewed changes
command/hook_pending.go
} else {
// PreApply.
if _, found := idToOp[event.id]; found {
panic(fmt.Sprintf("got duplicate PreApply for %s", event.id))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this panic can fire. Working on an updated version which refcounts calls to Pre/PostApply.

@glasser
Print pending actions during apply and destroy
f88f0f9 
During `apply` and `destroy` commands, prints the current set of
resources if no apply step has started or stopped within ten seconds.

Fixes hashicorp#5932.

Note: this removes the wrapping of UiHook.Ui in a ConcurrentUi, because
it appears that it is already a ConcurrentUi when constructed in Meta
anyway. Since there are now two hooks that share the Ui, what's
important is that the underlying Ui passed to both hooks is concurrent;
the fact that UiHook's is itself concurrent is distracting.
@glasser
Copy link
Contributor Author

glasser commented Apr 11, 2016

Fixed panic issue with refcounting.

@radeksimko radeksimko mentioned this pull request Apr 13, 2016
Closed
@mitchellh
Copy link
Contributor

Hey @glasser thanks! I completely missed this and ended up duplicating your work in #6162. I went the route of modifying UIHook to show pending resources when nothing has happened to a specific resource for a period of time.

There aren't any tests for the UI hook, probably one of the only non-tested areas of Terraform. :)

I like the decisions you made with yours but lets go with the more naive and simple approach with #6162 and see how that goes. Sorry!

@mitchellh mitchellh closed this Apr 13, 2016
@glasser
Copy link
Contributor Author

glasser commented Apr 13, 2016

I learned a lot about Terraform writing this, so no hard feelings. And yours is definitely a lot simpler!

One thing I discovered playing around with this, FYI: I definitely saw a bunch of cases where PostApply didn't seem to be called consistently at the end of an apply! I especially saw this on things like template_file (with create_before_destroy, in case that's relevant). In addition to seeing them show up in my "pending" messages for longer than I'd expect, I noticed that they never got Destruction complete messages and that the count at the end undercounted them as well. For example, see the end of this log:

9 operations pending: destroying 9 (module.stack.aws_autoscaling_group.app, module.stack.aws_autoscaling_group.proxy, module.stack.aws_autoscaling_group.service, module.stack.aws_ecs_task_definition.aggregator, module.stack.aws_ecs_task_definition.autocerts, module.stack.aws_ecs_task_definition.scheduler, module.stack.aws_launch_configuration.proxy, module.stack.aws_launch_configuration.service, module.stack.template_file.ssh_flagfile)
module.stack.aws_launch_configuration.app: Destroying...

Apply complete! Resources: 12 added, 6 changed, 2 destroyed.

None of those 9 operations get a Destruction complete or contribute to the destroyed count. Just thought you might want to know... maybe I should open a bug, but I don't have a great reproduction otherwise. I think your change will make this more visible too (it's really obvious when you see a bunch of "Still destroying" and then it just ends!)

@ghost
Copy link

ghost commented Apr 26, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
core enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Print actions in progress during apply
3 participants
@glasser @mitchellh @stack72