refactor: Remove ReplicaVersion::default() - #11087
Conversation
The replica version is the git commit hash that the replica (and the entire GuestOS) is built from. The value is read from `/opt/ic/share/version.txt` in the GuestOS and passed to the orchestrator and the replica via CLI args. Previously, we used a global static variable to store the version. The code could access it via `ReplicaVersion::default()`. The implementation read the global variable which was usually set at the start of the program (e.g., in main). The implementation fell back to the `CARGO_PKG_VERSION` when the global was unset (in most tests). Since `ReplicaVersion` is defined in ic_types, the implementation used the crate version which has been 0.9.0 since Dec 2023. Because the replica version was stored in a global variable, tests could not mock this value. This is a major refactor which: - explicitly wires `ReplicaVersion` to components that previously used `ReplicaVersion::default()` - adds a `test_replica_version()` to be used strictly by tests only - removes `ReplicaVersion::default()` since the default value was confusing and error-prone
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to 898faad. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
|
✅ No security or compliance issues detected. Reviewed everything up to 898faad. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
|
✅ No security or compliance issues detected. Reviewed everything up to 898faad. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
|
Wow. I mean, I guess we all agree that |
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
I guess this is not ready for review, since it is still in draft, and more commits are coming in, but I opened this, and started looking, so let me send some of my early incomplete thoughts...
| NodeId::from(PrincipalId::new_node_test_id(i)) | ||
| } | ||
|
|
||
| pub fn test_replica_version() -> ReplicaVersion { |
There was a problem hiding this comment.
This is NOT a request for change. Rather, it is just a statement of general principle.
I would rather use lazy_static over a 0-argument fn, but in this file, there's already user_anonymous_id. Sigh. I guess we can sweep through later and convert to lazy_static.
There was a problem hiding this comment.
I'm not a huge fan of lazy_static because it's kind of a hack (a custom type that has Deref implemented on it, the destructor never runs). Whereas a value returned from a function has a clear lifetime. For performance/identity reasons, reusing a static can have many benefits but I find using a method cleaner in a test if the underlying code is not const.
| unit_delay_millis: 500, | ||
| initial_notary_delay_millis: INITIAL_NOTARY_DELAY.as_millis() as u64, | ||
| replica_version_id: ReplicaVersion::default().into(), | ||
| replica_version_id: "replica_test_version".to_string(), |
There was a problem hiding this comment.
Why not also use test_replica_version here?
There was a problem hiding this comment.
Optimally, it would be nice to not use the same constant everywhere because it gives false assumptions (for example, in production a Block coming from the wire may have a different version inside than the replica's current version but then in the test both would be the same because both are wired up to test_replica_version()). So I tried using different constants to avoid these false equalities. However, it quickly gets annoying as many of our test mocks/factories/setups just hardcode the version and if two different factories use two different versions, the code under test will not like that (code where the requirement is legitimately that the replica versions in two components must be the same). It would have take way too much time to use different constants but then use the same constant when the test requires so. For simplicity and the sake of not wasting more time on this, I use test_replica_version() in most places but I still tried to use different constants in a few places where there is no requirement for the version to be the same as in some other place. In a perfect world, the test cases would explicitly set up the mocks such that versions match if and only if they have to match for the test case to pass.
There was a problem hiding this comment.
SG, but then, that makes me think we should not have test_replica_version at all. Instead, all tests come up with their own local test value. WDYT?
There was a problem hiding this comment.
Yup, we should do that. But it just takes a lot of time to set that up and I don't think anybody will every want to invest time into that. The reason is that tests often use shared mocks/setup functions. Optimally, we'd always pass the replica version to the setup function but it'd require changing a lot of setup functions. So instead, setup functions hardcode some replica version but then different setups will have different replica versions and test cases that use multiple such setup functions will have a mismatch in places where there shouldn't be a mismatch (e.g. consensus making blocks using a different replica version than what's the elected registry version). I actually tried this approach but there were just too many test failures to fix one by one so I had to introduce a shared constant.
My preferred solution would be automatic compile-time dependency injection using some library that automatically wires up the components, but introducing one is a lot of work and there'd certainly be opposers who prefer the current manual DI.
…-replica-version # Conflicts: # rs/consensus/dkg/src/lib.rs # rs/consensus/mocks/src/lib.rs # rs/consensus/src/consensus/notary.rs # rs/consensus/src/consensus/priority.rs # rs/https_outcalls/consensus/src/pool_manager.rs
…-replica-version # Conflicts: # rs/nns/integration_tests/src/upgrades_handler.rs # rs/registry/canister/tests/update_subnet_and_elect_replica_version.rs
3 participants
The replica version is the git commit hash that the replica (and the entire GuestOS) is built from. The value is read from
/opt/ic/share/version.txtin the GuestOS and passed to the orchestrator and the replica via CLI args. Previously, we used a global static variable to store the version. The code could access it viaReplicaVersion::default(). The implementation read the global variable which was usually set at the start of the program (e.g., in main). The implementation fell back to theCARGO_PKG_VERSIONwhen the global was unset (in most tests). SinceReplicaVersionis defined inic_types, the implementation used the crate version which has been 0.9.0 since Dec 2023. The replica version was stored in a global variable, so tests could not mock this value. In addition, prod code that transitively depended onReplicaVersion::default()may have not known about it and thus implicitly used the confusing 0.9.0 value (which does not even match the commit hash format).This is a major refactor which:
ReplicaVersionto components that previously usedReplicaVersion::default()-> any prod code that uses the replica version must explicitly pass the versiontest_replica_version()to be used strictly by tests onlyReplicaVersion::default()since the default value was confusing and error-prone