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

[ovsp4rt] Implement Client class (work in progress) #674

Closed
wants to merge 34 commits into from

Conversation

ffoulkes
Copy link
Contributor

@ffoulkes ffoulkes commented Aug 26, 2024

Superseded by PR #727. Retaining this PR for reference.

This is a project to improve the testability of ovsp4rt. See issue #701 for details.

  • Modified ovsp4rt.cc to use a Client object instead of an OvsP4rtSession object to communicate with the P4Runtime server.

  • Made each public C API function a wrapper around a C++ function that accepts a Client object as a parameter. This allows a unit test to replace the object with a test double (e.g. a mock).

  • Moved the C API functions to another file, separating the user interface from the implementation. This allows a substitute front end to be used for testing.

  • Implemented HaveEntry predicates as wrappers around the GetEntry functions. Modified the code to use the predicates when the purpose of the read request is to test for the existence of an entry.

Note

The BUILD_CLIENT cmake option is enabled by default in this branch. We cannot compile the modified version of ovsp4rt.cc without it.

- Implemented the `Context` class, to provide an abstract interface
  to the P4Runtime server.

- Modified ovsp4rt.cc to use the `Context` object instead of the
  `OvsP4rtSession` object.

In addition to simplifying the code, the `Context` object should
allow us to mock the P4Runtime server for unit testing.

Signed-off-by: Derek Foster <[email protected]>
@ffoulkes ffoulkes added tests Affects unit tests enhancement New feature or request and removed tests Affects unit tests labels Aug 26, 2024
- Made each public C API function a wrapper around a C++ function
  that accepts the Context object and one of its parameters.
  This makes it possible to inject a test double.

Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
@ffoulkes ffoulkes changed the title [ovsp4rt] Implement Context class [ovsp4rt] Implement Client class Sep 3, 2024
- Converted most of the GetEntry functions to HaveEntry predicates,
  to stress the difference between testing for the existence of an
  entry and retrieving its contents.

Signed-off-by: Derek Foster <[email protected]>
- Defined a virtual destructor.

- Made the methods virtual.

- Added internal documentation.

Signed-off-by: Derek Foster <[email protected]>
@ffoulkes ffoulkes changed the title [ovsp4rt] Implement Client class [ovsp4rt] Implement Client class (work in progress) Nov 6, 2024
- Enabled the BUILD_CLIENT cmake option in the development
  branch.

Signed-off-by: Derek Foster <[email protected]>
- Added missing include directory and link libraries when
  building ovsp4rt_client object library.

Signed-off-by: Derek Foster <[email protected]>
- Fixed compile errors when building the Client library.

- Fixed link errors when building ovsp4rt with the Client
  and Journal libraries.

- Removed the session() method from the Client class.

Signed-off-by: Derek Foster <[email protected]>
ffoulkes and others added 4 commits December 16, 2024 13:51
- Changed `client` parameter type from `Client` to
  `ClientInterface`, to support dependency injection.

Signed-off-by: Derek Foster <[email protected]>
- Moved the C API wrapper functions to a separate source
  file, to allow a substitute front end to be used for
  testing.

Signed-off-by: Derek Foster <[email protected]>
- Created JournalClient, a subclass of Client that is
  instrumented to record interactions between OVS and
  the P4Runtime server.

- Created a variant of the public API that uses
  JournalClient objects.

Signed-off-by: Derek Foster <[email protected]>
- Renamed ovsp4rt_c_api.cc to ovsp4rt_standard_api.cc
  and ovsp4rt_cc_api.h to ovsp4rt_internal_api.h, to
  make their purpose clearer.

Signed-off-by: Derek Foster <[email protected]>
- Noted instances where we ignore errors without explanation.

Signed-off-by: Derek Foster <[email protected]>
ffoulkes added a commit that referenced this pull request Dec 26, 2024
- Moved the C API wrapper functions to a separate source
  file. This makes it possible to substitute a different
  implementation of the API, for testing.

  Extracted from PR #674. Works with an updated version
  of ovsp4rt.cc that has not been committed. Currently
  excluded from the build.

Signed-off-by: Derek Foster <[email protected]>
ffoulkes added a commit that referenced this pull request Dec 26, 2024
- Created a variant of the public API that uses
  JournalClient objects.

  Extracted from PR #674. Works with several files that
  have not yet been committed, or that are being committed
  separately. Currently excluded from the build.

Signed-off-by: Derek Foster <[email protected]>
- Restored the original names of the GetEntry functions, and
  reimplemented the HavEntry predicates as wrappers around the
  GetEntry functions.

Signed-off-by: Derek Foster <[email protected]>
- Added methods to the Journal class to record P4Runtime server
  requests and responses.

Signed-off-by: Derek Foster <[email protected]>
Signed-off-by: Derek Foster <[email protected]>
ffoulkes added a commit that referenced this pull request Jan 1, 2025
- Moved the C API wrapper functions to a separate source
  file. This makes it possible to substitute a different
  implementation of the API, for testing.

  Extracted from PR #674. Works with an updated version
  of ovsp4rt.cc that has not been committed. Currently
  excluded from the build.

Signed-off-by: Derek Foster <[email protected]>
ffoulkes added a commit that referenced this pull request Jan 1, 2025
- Created a variant of the public API that uses
  JournalClient objects.

  Extracted from PR #674. Works with several files that
  have not yet been committed, or that are being committed
  separately. Currently excluded from the build.

Signed-off-by: Derek Foster <[email protected]>
@ffoulkes
Copy link
Contributor Author

ffoulkes commented Jan 6, 2025

Superseded by PR #727.
Closing without deleting, so the original PR is available for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant