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

[#349] Add service name generator to be able to run tests concurrently #472

Merged

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Oct 16, 2024

Notes for Reviewer

Fixes some parts of the problem but not everything

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Relates to #349
Closes #475

@elfenpiff elfenpiff self-assigned this Oct 16, 2024
orecham
orecham previously approved these changes Oct 16, 2024
Copy link
Contributor

@orecham orecham left a comment

Choose a reason for hiding this comment

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

Had to do the exact same thing in the RMW 😆 .

@elfenpiff
Copy link
Contributor Author

elfenpiff commented Oct 16, 2024

When set_log_level(LogLevel::TRACE) is active one test fails with this:

ServiceEventTest/0.open_or_create_service_does_exist

      353 [D] Builder { storage_name: FileName { value: FixedSizeByteString<255> { len: 40, data: "26380db3f2d1d21a17def8273faec2e5097cf3e2" } }, supplementary_size: 0, ha
s_ownership: false, config: Configuration { suffix: FileName { value: FixedSizeByteString<255> { len: 8, data: ".dynamic" } }, prefix: FileName { value: FixedSizeByteStrin
g<255> { len: 5, data: "iox2_" } }, path: Path { value: FixedSizeByteString<255> { len: 14, data: "/tmp/iceoryx2/" } }, _data: PhantomData<iceoryx2::service::dynamic_confi
g::DynamicConfig> }, timeout: 500ms, initializer: , _phantom_data: PhantomData<iceoryx2::service::dynamic_config::DynamicConfig> }
| Failed to open  since the version number was not set - (it is not initialized after 500ms).

Which then lead to a failure when opening the dynamic service info and then the builder assumes the services is in a corrupted state.
This comes only up with bazel, never occurs with cmake or with Rust (which has the same test)

The mechanism that seems to fail here is the concurrent dynamic storage creation/opening for the ipc::Service. To support concurrent creation/opening the creation must somehow tell the opener that the initialization is still in progress. This is done with two mechanisms:

  1. The permission is set to write only as long as the memory is initializing.
  2. The first 8 byte (or so) are zeroed by the OS and
  3. when the initialization is finished the version number is written.
  4. The permission is set to read/write

When the opener comes it first checks if it has read/write permissions, if not, initialization is still in progress. If it has read/write permissions it also checks the version number. If it is zero, still in initialization, if not zero it must match its own local version number.

…hecked for changes instead of reloading the underlying memory
@elfenpiff
Copy link
Contributor Author

Okay, there was an actual bug in the dynamic storage posix shared memory implementation. When the creation process is still initializing the memory, the opener takes a COPY of the version number instead of a pointer to it and checks it until it becomes non-zero.

The thing is, this is a bug that should only occur on OSes that do not support adjusting the permissions of the shm during runtime like Mac OS and Windows and not linux.

And this also does not fix the failing test

…es CARGO_PKG_VERSION_{MAJOR|MINOR|PATCH} all to 0 by setting them to u16::MAX
@elfenpiff elfenpiff force-pushed the iox2-349-bazel-compatible-tests branch from ac74d05 to 7f662b9 Compare October 16, 2024 10:34
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.92%. Comparing base (cd27eee) to head (b3dfa90).
Report is 27 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
- Coverage   78.93%   78.92%   -0.01%     
==========================================
  Files         197      198       +1     
  Lines       23637    23627      -10     
==========================================
- Hits        18657    18647      -10     
  Misses       4980     4980              
Files with missing lines Coverage Δ
iceoryx2-bb/elementary/src/package_version.rs 87.50% <100.00%> (+17.50%) ⬆️
...yx2-cal/src/dynamic_storage/posix_shared_memory.rs 89.29% <100.00%> (-0.25%) ⬇️

... and 4 files with indirect coverage changes

}

PackageVersion::from_u64(PACKAGE_VERSION.load(Ordering::Relaxed))
PackageVersion::from_version(MAJOR, MINOR, PATCH)
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, do we have a specific version convention? not sure why we didn't use the semver, which is popular in rust. so in the future we may have beta, alpha or rc versions.

https://crates.io/crates/semver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general we try to be very cautious to introduce external crates. For any other project I would agree with you but iceoryx2 shall be certified as ASIL D according to ISO26262 which is the highest safety standard in the automotive world.

This requires 100% MC/DC coverage for every line of code. Every test must be covered by a requirement, we need detailed architecture documents and documentation, a safety manual and so on.

So, every dependency we introduce must satisfy all of this! No exception. But actually, nearly none of them does it, and it is a challenge to do it. We have a strategy in place how to solve this but one of the key factors is - try to avoid external crates as much as possible.

Another answer we need to find is, how to handle or prevent supply chain attacks.

So in light of all of this, we try to be very cautious with external dependencies since the more we have the more work we have to do in the near future.

Copy link
Contributor

Choose a reason for hiding this comment

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

ASIL D according to ISO26262 ... how to handle or prevent supply chain attacks.

understood, quite a bit different from the normal project. thanks for your explaination:)

@@ -31,6 +32,15 @@ using ServiceTypeLocal = TypeServiceType<ServiceType::Local>;

using ServiceTypes = ::testing::Types<ServiceTypeIpc, ServiceTypeLocal>;

inline auto generate_service_name() -> ServiceName {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it deserves a unit test:)

@@ -79,10 +79,11 @@ number is `Xold.Yold.Zold`.
10. Adjust the `<version>` to `X.Y.Z` in `$GIT_ROOT$/package.xml`.
11. Call `rg "Xold\.Yold\.Zold"` and adjust all findings.
* C and C++ examples, `BUILD.bazel` & `CMakeLists.txt`
12. **Merge all changes to `main`.**
13. Set tag on GitHub and add the release document as notes to the tag
12. Adjust the major, minor and patch version number in `iceoryx2_bb_elementary::PackageVersion`
Copy link
Member

Choose a reason for hiding this comment

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

Having a script taking care of setting the version number in all known locations would be helpful. I'll add it to my todo list :)

@elfenpiff elfenpiff merged commit a657228 into eclipse-iceoryx:main Oct 16, 2024
44 of 45 checks passed
@elfenpiff elfenpiff deleted the iox2-349-bazel-compatible-tests branch October 16, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use an alternative approach to make version available in PackageVersion
4 participants