Fix #40, move threader shutdown after all work has completed

[dannyk81]

@pryorda @Jc2k this fixes #40 for me

please take a look

[Jc2k]

The threader shutdown is where it was because there is a dependency between the vm metrics and the vm perf counters (it assumes self._labels is populated already). So this is probably introducing a non deterministic race 😞

[dannyk81]

Ohhh... I see it now.

Well, looks like using the threader after it was already shutdown is causing some issues.

Perhaps the dependency can be more explicitly defined? maybe using separate execute contexts?

[Jc2k]

Ahh! Because you can’t submit jobs after shutdown is called! Of course!

Well I can probably get rid of the nested threads all together with a property collector (it can fetch all data for all hosts and data stores in one call). Will try and look at doing that.

[dannyk81]

That'd be great @Jc2k, I'm going to close this PR on account that this fix is not safe.

@pryorda FYI though that v0.3.0 is currently broken for hosts and datastores, but with @Jc2k's help we should be able to fix this.

[pryorda]

@Jc2k @dannyk81 Good finds. I'm surprised we didn't notice this in the PR. I think pushing the shutdown lower down should be fine. @Jc2k Can you elaborate on the Race Condition you're worried about?

[pryorda]

@dannyk81 I don't want to close this just yet. This might be the immediate fix, but not the log term solution.

[pryorda]

I think it will be beneficial to add #41 and #42 to a test branch before merging to v0.3.1 and i'll get that done asap for testing. 1-2hr

[pryorda]

Release updated: https://github.com/pryorda/vmware_exporter/releases/tag/v0.3.0

[Jc2k]

I think that this PR is unsafe, and is trading one race for another. After this PR is merged the extra code that runs early depends on self._labels. That structure is generated on a thread. The current way ensures that structure is populated before the code that uses it runs. The new way allows it to run before it exists.

[dannyk81]

@pryorda I didn't see the issue that pushing down the shutdown was creating, but this fix as-is creates another problem.

I suggest to close this and work on a fix which will reconcile both races properly.

[pryorda]

We should able to force a thread completion before moving on. I think you're right in saying that maybe we should remove some of the nested threading

[pryorda]

Do we want to continue the conversation here or move it to #41 and #40?

[dannyk81]

I suggest we close this one and discuss this over in #40