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

api: WaitForUpdatesEx & DestroyPropertyFilter #3331

Merged

Conversation

akutz
Copy link
Member

@akutz akutz commented Jan 8, 2024

Description

This patch introduces Ex variants for many of the property collector types/methods to take advantage of the remote API DestroyPropertyFilter. Now it is possible to use a single property collector for waiting for updates and remove previously added filters.

BREAKING: The semantics around the helper functions in the
          property package have changed. Please review any
          code that calls this package to ensure it is
          compatible with the new behaviors.

Closes: NA

Type of change

Please mark options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Build related change

How Has This Been Tested?

  • New unit test for property.WaitForUpdatesEx
  • New unit test for property.Collector's WaitForUpdatesEx function
  • New unit test for DestroyPropertyFilter

Checklist:

  • My code follows the CONTRIBUTION guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch 3 times, most recently from 14d5c45 to 5a43f74 Compare January 8, 2024 16:15
@akutz akutz requested a review from dougm January 8, 2024 21:08
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @akutz
The new property.WaitForUpdatesEx lgtm.
Keep in mind ServiceContent.PropertyCollector (aka property.DefaultCollector()) is a session singleton, not safe for multiple threads to WaitForUpdatesEx on.
object.Task.{Wait,WaitForResult} and the new Ex versions both use DefaultCollector.Create(), so the Ex version(s) are not thread safe.
Thinking:

  • leave the existing object.Task methods as-is
  • change task.Wait to always use property.WaitForUpdatesEx, rather than add task.WaitEx (don't think anyone currently calls task.Wait directly?)
  • then apps would call task.Wait to re-use a PC instance

property.Filter - tempted to do a breaking rename of the existing type, to property.Match or ?
Then property.Filter can be used to wrap the MO instead of FilterEx

@akutz
Copy link
Member Author

akutz commented Jan 9, 2024

Hi @dougm ,

Thanks for taking a look. Yeah, the concurrency thing is an issue. What are your thoughts on starting WaitForUpdatesEx in a Goroutine the moment a session is established, using the default PC, and then we change the way Wait occurs for everybody else to:

  • Register on update callback
  • Add prop filter
  • Receive update via callback
  • Remove prop filter

That way there's always a single, long-running WaitForUpdates goroutine that can be used to listen for all updates, whenever there's a prop filter added?

@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch from 5a43f74 to 6201145 Compare January 9, 2024 16:37
@akutz
Copy link
Member Author

akutz commented Jan 9, 2024

Hey @dougm,

Please see the refactored PR. I added a lock to the PropertyCollector such that any calls to WaitForUpdates, WaitForUpdatesEx, or CheckForUpdates must obtain an exclusive lock on the property collector. The attempt to obtain the lock will timeout if the context is cancelled.

@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch 2 times, most recently from dca80e4 to 9914a6d Compare January 9, 2024 16:40
@akutz akutz changed the title WaitForUpdatesEx & DestroyPropertyFilter api: WaitForUpdatesEx & DestroyPropertyFilter Jan 9, 2024
@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch 3 times, most recently from d6acfa8 to 8be3adb Compare January 9, 2024 17:31
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @akutz , can you also add a BREAKING: section to the commit? Suggesting one more for the list (CreateFilter)

property/collector.go Outdated Show resolved Hide resolved
property/collector.go Outdated Show resolved Hide resolved
@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch 2 times, most recently from 12a6037 to 5ea7ac5 Compare January 10, 2024 14:02
This patch introduces Ex variants for many of the property
collector types/methods to take advantage of the remote API
DestroyPropertyFilter. Now it is possible to use a single
property collector for waiting for updates and remove
previously added property filters.

BREAKING: The semantics around the helper functions in the
          property package have changed. Please review any
          code that calls this package to ensure it is
          compatible with the new behaviors.
@akutz akutz force-pushed the feature/waitforupdatesex-destroy-prop-filter branch from 5ea7ac5 to 95aa257 Compare January 10, 2024 14:14
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @akutz !

@akutz akutz merged commit e24c985 into vmware:main Jan 10, 2024
11 checks passed
@akutz akutz deleted the feature/waitforupdatesex-destroy-prop-filter branch January 10, 2024 17:24
dougm added a commit to dougm/govmomi that referenced this pull request Aug 10, 2024
In the PR vmware#3331 refactor WaitOptions was changed to pass by value, rather than reference.
The Truncated value needs to be propagated from WaitForUpdatesEx response to the caller.

Fixes vmware#3501
dougm added a commit to dougm/govmomi that referenced this pull request Aug 10, 2024
In the PR vmware#3331 refactor WaitOptions was changed to pass by value, rather than reference.
The Truncated value needs to be propagated from WaitForUpdatesEx response to the caller.

Fixes vmware#3501
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