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.

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

vcsim: add support for PropertyCollector incremental updates #1170

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

dougm
Copy link
Member

@dougm dougm commented Jul 9, 2018

Prior to this change, clients could only call WaitForUpdatesEx once,
which would return the current state. The next invocation with Version
given would fail as unsupported. Clients can now use WaitForUpdatesEx
to wait for multiple updates.

The basic idea is that methods should now update object fields using
the Registry.Update method instead of setting fields directly.
Registry.Update uses the same PropertyChange structure that is sent as
part of the WaitForUpdatesEx response. Registry.Update will also apply
the PropertyChange(s) to the local object, removing the need to set
fields directly.

  • govc commands now use Client.Flag.WithCancel to catch SIGINT,
    allowing proper cleanup of collectors

  • Add vcsim ListView support to ViewManager

Fixes #922

@dougm
Copy link
Member Author

dougm commented Jul 9, 2018

Still some tests and doc to add, but the basics are functioning now. There are still methods that are setting object fields directly, we can convert more to using UpdateObject in separate PRs.
The property path filters also still need to be applied, currently all updates are included in the response, though the objects themselves are properly filtered.

A few examples that now work against vcsim:

% govc tasks -f

% govc events -f

% govc object.collect -wait 1h -type m / runtime.powerState

@dougm
Copy link
Member Author

dougm commented Jul 9, 2018

cc @ausevj @hickeng @kars7e @embano1

@dougm dougm force-pushed the vcsim-wait-for-updates branch 17 times, most recently from 8b4cddf to 0f2ee66 Compare July 14, 2018 04:14
@dougm
Copy link
Member Author

dougm commented Jul 14, 2018

Added more tests and property path filters since opening the PR, ready for review now.

@dougm dougm force-pushed the vcsim-wait-for-updates branch 4 times, most recently from 0c52d89 to c7b8abd Compare July 17, 2018 00:31
werr = f(wctx)
}()

sig := make(chan os.Signal)
Copy link

@jiatongw jiatongw Jul 18, 2018

Choose a reason for hiding this comment

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

sig := make(chan os.Signal, 1)?
https://gobyexample.com/signals as a reference

Copy link
Member Author

Choose a reason for hiding this comment

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

That reference doesn't say why though. Go's docs are a bit better: https://golang.org/pkg/os/signal/#Notify
Your comment here also made me realize the sig channel needs to be created earlier, before the go routine calling f is started. Changed so that happens first.

@@ -82,30 +131,19 @@ func TestRace(t *testing.T) {
t.Fatal(err)
Copy link

@jiatongw jiatongw Jul 18, 2018

Choose a reason for hiding this comment

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

Here is the tricky part, about t.Fatal() in goroutine. I have read similar code before when I was learning Golang and I searched online. There is a group discussion: Whenever t.Fatal is called in a goroutine, it is swallowed and lost. (reference https://groups.google.com/forum/#!topic/vault-tool/zNlHJLxoo3w, also here ipfs/kubo#2043). One of their guys also used staticcheck to check their code. So, for double check, I also ran it against this file. Result is the same as theirs. So, we can discuss about that.
Line 119, 126, 131, 152

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I've changed those to use t.Error instead.

@jiatongw
Copy link

Generally, I pulled your PR and ran some commands, it works well.

@dougm dougm force-pushed the vcsim-wait-for-updates branch 2 times, most recently from f573a39 to 8a778fc Compare July 18, 2018 14:24
@jiatongw
Copy link

@dougm Travis CI build failed here. simulator/property_collector_test.go line 404, 442 also need to change to t.Error

@dougm
Copy link
Member Author

dougm commented Jul 18, 2018

Yeah I've been re-running the Travis build manually as the race detector is finding some issues that I don't see locally. I'll fix the Fatal -> Error logs.

@jiatongw
Copy link

Looks good to me 👍

@dougm dougm force-pushed the vcsim-wait-for-updates branch 3 times, most recently from 3b18524 to faaeee8 Compare July 19, 2018 20:36
Prior to this change, clients could only call WaitForUpdatesEx once,
which would return the current state.  The next invocation with Version
given would fail as unsupported.  Clients can now use WaitForUpdatesEx
to wait for multiple updates.

The basic idea is that methods should now update object fields using
the Registry.UpdateObject method instead of setting fields directly.
UpdateObject uses the same PropertyChange structure that is sent as
part of the WaitForUpdatesEx response.  UpdateObject will also apply
the PropertyChange(s) to the local object, removing the need to set
fields directly.

- govc commands now use Client.Flag.WithCancel to catch SIGINT,
  allowing proper cleanup of collectors

- Add vcsim ListView support to ViewManager

Fixes vmware#922
@dougm dougm merged commit 8d973c3 into vmware:master Jul 20, 2018
@dougm dougm deleted the vcsim-wait-for-updates branch July 20, 2018 05:25
@dougm dougm added this to the 2018.09 milestone Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants