provider: Add session persistence #422
Conversation
This commit contains most of the SOAP session persistence code. Persisting sessions to disk will work mostly in the same way that govc does, and in fact most of the code here has been adapted from this work. However, we also need to persist REST sessions which of course govc does not know about. This will be added in a similar fashion. This code is ultimately necessary to ensure acceptance tests work, which create enough new sessions to hit the default 1000 session limit in vSphere, mainly from the re-creation of the provider every time testAccProviders() is called. Note that just fixing this helper won't work, as the test framework we currently use for the acceptance tests runs individual tests in their own sub-process as well, meaning that there would not be any re-use across the entire set of tests, just within the one test, so improvements would be marginal.
Got nearly everything done and working now, save the REST client session re-use. This could possibly be something wrong with determining the REST client's validity, which is work we are doing in vmware/vic concurrently with this.
We were using the incorrect method type for checking on sessions in the work we are doing in the VIC tags client. Fixed that now, and session persistence for tags is now working. Also corrected/added a couple of log messages for the REST client to make the REST client configuration process more/properly verbose.
Probably caused by an errant vim key combo. :P
* Added the environment variables that can be used to specify values. * Renamed the directory options to their new keys (s/directory/path/).
Now using "path" instead of "directory", to be consistent with the rest of the options.
One test to test for persistence, and another to test to ensure that sessions did not persist.
|
I wonder if by default this should use I'm a little worried that it potentially leaks data in its default setup but maybe I misunderstand how the sessions work? |
|
Oh nm, without being a shared location it eliminates the benefit :( As its opt-in, this is probably fine. |
| if err != nil { | ||
| t.Fatalf("error creating REST session temp directory: %s", err) | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
maybe split this in to two defers as there is a possible directory leak if 2nd one errors
There was a problem hiding this comment.
Yeah, good catch. I think I was thinking "well defer will happen even on fatal" but that won't happen for the inner fatals, will it ;p Will fix.
| t.Fatalf("error creating REST session temp directory: %s", err) | ||
| } | ||
| defer func() { | ||
| if err = os.RemoveAll(vimSessionDir); err != nil { |
|
Thanks @paultyng, yeah, this is how govc does it too (although not opt-in, but rather opt-out, probably due to the much larger risk of running multiple successive/concurrent govc processes versus us here). I looked at the JSON saved by the SOAP session code and the only sensitive thing leaked is the session ID. So while that of course needs to be treated with care (and is as such mentioned in the docs), it's not as bad as say having full-on credentials there. Temporary directories could be created ahead of time and passed in, but that's something we can't really control within TF itself. An example is kind of "given" in the acceptance tests on how that can be done, and I'm going to apply a similar pattern to CI so that it uses a path in the working directory so that we can clean it up afterwards. Will be merging it shortly after making a couple of updates - splitting the defers in the acceptance tests up and also adding a skip for |
* Split the tempdir removal defers up so that failing on their removal does not possibly cause dangling directories. * Added another pre-check for TF_ACC as these test are not being skipped on unit testing.
This should have been tested properly in #422, but the way providerConfigure was returning a fully configured Client versus the configuration made it hard to do so, so it was skipped, and of course I was burned as a result. :P This fixes things so that the actual creation of the Config struct is moved to a separate NewConfig function, which allows us to test for the valid config keys. providerConfigure then just calls that.
ghost
locked and limited conversation to collaborators
This introduces session persistence to the provider.
This is actually being driven by our needs in getting the provider up and running in our CI for nightly acceptance tests. Unfortunately, the large amount of new TF contexts that the acceptance test suite will make for a single test means that running all of these tests - even in succession versus concurrently - consumes all of the available sessions within vSphere - and that limit is 1000. 😨
Introducing this has removed session-related errors from the acceptance tests when running them in CI.
The method has been largely adapted from govc and the SOAP client session caching has been designed to be compatible with default behaviour in govc - it should actually be able to re-use govc sessions if they exist. However, while it is on by default in govc, it is not in Terraform.
This work has also produced vmware/vic#7464 which will have to be re-vendored once it's merged. For now we are working off a fork.