Sync epoch #622
Sync epoch #622
Conversation
vvarg229
requested review from
roman-khimov,
cthulhu-rider and
evgeniiz321
as code owners
| @@ -202,7 +202,7 @@ def test_session_token_expiration_flags( | |||
| session=session_token, | |||
| ) | |||
|
|
|||
| self.tick_epochs(2) | |||
| self.tick_epochs_and_wait(2) | |||
There was a problem hiding this comment.
iiuc tick_epochs_and_wait asserts that all nodes match certain condition (>=CURRENT+2) and have exactly the same epoch, am i right?
if so, for cases like this, we may lighten requirement to different values matching the condition (e.g. if we tick from 10 twice, then we are OK with any 12+ value). While same values are definitely better and expected much more, the test doesn't break with the different ones IMO
There was a problem hiding this comment.
Different tests can have different requirements, so having a single current epoch is safer. And it's feasible in general, the next epoch update is expected to happen long after the current one once it's updated. If we have premature epoch ticks from IR that's a problem and failing a test in this case is appropriate behaviour.
There was a problem hiding this comment.
i'd leave the tests granular and, if possible, not adding dependencies on other subsystems (we test them separately). Such a test may fail on a strange tick, but this may not be a node's token verification bug. If epoch is OK, then token is OK, and vice versa, no matter how they tick. Stronger precondition does not improve the test IMO
this is my developer vision, may be @vvarg229 @evgeniiz321 have other opinion
There was a problem hiding this comment.
What if you want to test token's nbf? It won't be OK if some nodes are to have N+1 epoch.
There was a problem hiding this comment.
@cthulhu-rider let's continue the discussion in another issue?
I don't really understand how the ticking of an epoch can affect a session token.
vvarg229
force-pushed
the
sync_epoch
branch
4 times, most recently
from
8e72775
to
0bb4ffa
Compare
Fixed |
Added functionality to make sure the new epoch ticked. This is to ensure that the epoch is ticked before the next step in tests where it is necessary. Signed-off-by: Oleg Kulachenko <[email protected]>
Signed-off-by: Oleg Kulachenko <[email protected]>
Signed-off-by: Oleg Kulachenko <[email protected]>
| current_epoch = self.get_epoch() | ||
| for _ in range(epochs_to_tick): | ||
| self.tick_epoch() | ||
| epoch.wait_for_epochs_align(self.shell, self.cluster, current_epoch) |
There was a problem hiding this comment.
is it ok that current_epoch won't be really current? shouldn't it be updated inside the cycle?
There was a problem hiding this comment.
is it ok that current_epoch won't be really current? shouldn't it be updated inside the cycle?
That's right - we are comparing the current epoch and the epoch that will be after the ticking.
Closes #614
Tests result:
https://http.t5.fs.neo.org/86C4P6uJC7gb5n3KkwEGpXRfdczubXyRNW5N9KeJRW73/330-1693812139/index.html
https://github.com/vvarg229/neofs-node/actions/runs/6063915255/job/16466308125