Reanalyze events, history, log and decouple from implementation #18

hmpf · 2023-06-22T09:30:56Z

This is a major rethink of what events, models, and history looks like. The data classes themselves contain only what is needed to have valid information, and the info is validated on creation by Pydantic. The data classes are filled with data and changed by Engine classes, which are adapted to whatever API/protocol is needed. Using the Zino1 wire protocol has been hidden in the Zino1EventEngine.

The goal is that the data classes can be used both by servers and clients.

MVC-speak: the data classes are models, the engines are the controllers.

Everything is written from the bottom up to be easy to unit test, without using mock.patch().

Feedback needed:

naming. For instance:
- is EventEngine clear enough? This is quite abstract since a layer has been added between the protocol and the data, so names are even harder than usual.
- the names for all the State-enums, even more verbose maybe (unabbreviate things)? Only visible to programmers.
- the (uppercase) keys in the enums: should they similarly be more verbose? They're only visible to programmers so we should think programmer UX.
filenames and placement. For instance:
- should the EventEngine class be in "event_types.py" or somewhere else? It is not an abstract class, it contains methods independent of having a connection to a zino-server.
- Is "event_types.py" the right place for the history and log-models?
- some helper functions should probably move to "utils.py"

Missing:

tests
should there be a bootstrapper/factory now to get a Zino 1 connection, instead of the doc-string in "zino1.py"? It's not here yet because it needs more thinking.
updates via the update channel. We don't need it for a demo and it needs more thinking. Because this is not implemented, it is necessary to get_events after setting state or changing history.

Please nitpick! Please ask why things are laid out as they are!

github-actions · 2023-06-22T09:32:10Z

Test results

0 tests 0 ✔️ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit d344a10.

♻️ This comment has been updated with latest results.

codecov · 2023-06-22T09:32:35Z

Codecov Report

❗ No coverage uploaded for pull request base (main@8f4c733). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage        ?   67.63%           
=======================================
  Files           ?        9           
  Lines           ?     1029           
  Branches        ?        0           
=======================================
  Hits            ?      696           
  Misses          ?      333           
  Partials        ?        0

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lunkwill42

This is probably a great improvement, but my head just isn't into grokking all of this at 3pm on my last day before summer vacation.

I wholly support the separation of concerns, and would appreciate maybe a face-to-face rundown of this compared to what we're doing in the new zino 2.0 codebase when we get back from vacay.

lunkwill42 · 2023-06-23T12:51:50Z

src/zinolib/event_types.py

+    * "testing" was used six times from 2000 to 2001
+    * "admin0" was used ten times from 2000 to 2002
+    * "flapping" was used 22 times on 2016-07-13T09:55 UTC
+    * "notPresent" was used 161 times during 2003
+    * "5" was used 389 times from 1998 to 2000
+    * "dormant" was used 813 times from 2000 to 2006


I would assume these are mostly derived from IF-MIB::ifOperState, as defined on page 31 of RFC 2863:

up(1), -- ready to pass packets down(2), testing(3), -- in some test mode unknown(4), -- status can not be determined -- for some reason. dormant(5), notPresent(6), -- some component is missing lowerLayerDown(7) -- down due to state of -- lower-layer interface(s)

Zino does feature a flapping status, but I think this might be in a different event attribute, as the actual operational status fetched from the port is either up/down (or any of the other values above). "Flapping" is just Zino's way of flagging an event for a port that keeps switching between up/down states very often.

I suspect the "flapping" enum value is no longer used/supported since it was only ever used once, during one minute, since 1998. There is a separate enum (FlapState) for showing flapping which I assume was added after that happened.

Does the existing server push these values on blindly?

We'll have to ask HE about this.

Go right ahead and ask HE. Since many of the "strange" values exist only in old data and for limited periods, I would mostly assume they are due to old bugs or at least old versions of the Zino codebase, and do not necessarily represent today's situation.

Most certainly, the 5 value you saw in the data is identical to dormant: Zino likely got this state value from some ports, and some hiccup in Scotty prevented the raw value from being translated to the MIB's name for that value, dormant.

lunkwill42

This looks mostly fine to me - I've added a couple of nitpicks about missing type annotations that would be helpful, but this applies really to more locations than the few I've commented.

I'd still appreciate a F2F rundown of the architecture here, as I'm not all that familiar with the original ritz/zinolib code. At least that would put me in a better state to comment on your design suggestions. Maybe we can schedule one for next week.

src/zinolib/zino1.py

tests/test_zinolib_event_types.py

lunkwill42 · 2023-08-17T08:23:42Z

src/zinolib/event_types.py

+    def get_downtime(self):
+        """Calculate downtime on this PortState"""
+        # If no transition is detected, use now.
+        now = utcnow()
+        lasttrans = self.lasttrans or now
+        accumulated = self.ac_down or timedelta(seconds=0)
+
+        if self.port_state in [PortState.DOWN, PortState.LOWER_LAYER_DOWN]:
+            return accumulated + now - lasttrans
+        else:
+            return accumulated


I don't know the original ritz/zinolib code very well. Is this algorithm taken directly from there?

Pretty much, yes. The original code calls datetime.now() multiple times and access lasttrans/ac_down via a case-object, not a typed event-object. Grep for def get_downtime in src/zinolib/ritz.py.

The EventEngine currently only supports the first load of events, not updates. This was originally written with pydantic v1, v2 was released during development of this rather long-lived branch. Since v2 made our subclass of BaseModel unnecessary, we attempted to upgrade via bump-pydantic. It didn't pick up on our sub-class and some other annoyances. Input to timedelta works different, on v1 you could build a timedelta from an int of seconds stored in a string, in v2 it must be an int. Interestingly the v1 docs does not mention that int-as-string is supported.

hmpf · 2023-10-11T07:49:15Z

This is becoming a ridiculously long-lived branch.

Tests are in, engines have been renamed to managers and collected in zinolib.controllers.

Bootstrapping and updates via the update-channel are in a different branch based on this, waiting for a merge.

podliashanyk

It works against Howits ig you remove SessionAdapter. See comments

src/zinolib/zino1.py

Rename *engine to *manager and collect them all in the directory controllers. The classes were renamed because the exact same name was used in a different project. They were also moved since the base class was in the wrong file.

hmpf · 2023-10-12T05:37:57Z

The file "src/zinolib/zino1.py" will be removed as soon as this PR is merged, it has only existed to make it easier to use the library while it was in flux.

src/zinolib/event_types.py

podliashanyk

Looks good to me. The implementation was also thoroughly tested by using this code in Howitz and it works really well.

There are some small nitpicks, but nothing major (can be merged as is for my part):

remove commented out code (see comments)
get_downtime could return a formatted string without microseconds instead of timedelta, as its only purpose is to be displayed in the table. But this could either be handled in a separate PR, or be delegated to the frontend completely.

And when this is merged, #24 can also be closed 😌

src/zinolib/event_types.py

sonarcloud · 2023-10-12T08:41:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

No Coverage information
0.1% Duplication

hmpf requested review from lunkwill42, johannaengland, podliashanyk, stveit and runborg June 22, 2023 09:31

hmpf added help wanted Extra attention is needed zino labels Jun 22, 2023

lunkwill42 reviewed Jun 23, 2023

View reviewed changes

hmpf force-pushed the decoupling branch 2 times, most recently from 58af551 to ff06d0d Compare June 29, 2023 13:03

hmpf force-pushed the decoupling branch from ff06d0d to 1778c5f Compare July 25, 2023 10:23

hmpf self-assigned this Aug 7, 2023

hmpf force-pushed the decoupling branch from 1778c5f to 6142409 Compare August 15, 2023 13:09

lunkwill42 reviewed Aug 17, 2023

View reviewed changes

podliashanyk force-pushed the decoupling branch 2 times, most recently from 4381cc8 to 8bd4422 Compare August 21, 2023 11:23

hmpf force-pushed the decoupling branch 2 times, most recently from a8a7305 to efd8ba4 Compare August 22, 2023 10:11

hmpf force-pushed the decoupling branch from efd8ba4 to 69cd772 Compare September 4, 2023 11:44

hmpf force-pushed the decoupling branch 3 times, most recently from 5105619 to f0fa020 Compare October 11, 2023 06:21

hmpf and others added 2 commits October 11, 2023 09:35

Remove whitespaces but add trailing space

0a03684

hmpf force-pushed the decoupling branch from f0fa020 to e5278c3 Compare October 11, 2023 07:42

hmpf marked this pull request as ready for review October 11, 2023 07:49

hmpf requested a review from lunkwill42 October 11, 2023 07:50

podliashanyk requested changes Oct 11, 2023

View reviewed changes

src/zinolib/zino1.py Outdated Show resolved Hide resolved

src/zinolib/zino1.py Outdated Show resolved Hide resolved

src/zinolib/zino1.py Outdated Show resolved Hide resolved

hmpf added 3 commits October 12, 2023 07:35

Rename *Engine

0debffe

Rename *engine to *manager and collect them all in the directory controllers. The classes were renamed because the exact same name was used in a different project. They were also moved since the base class was in the wrong file.

Hide ritz by renaming errors

a76ef6d

Handle removed events

f970218

hmpf force-pushed the decoupling branch from e5278c3 to f970218 Compare October 12, 2023 05:36

hmpf requested a review from podliashanyk October 12, 2023 05:38

podliashanyk reviewed Oct 12, 2023

View reviewed changes

src/zinolib/event_types.py Outdated Show resolved Hide resolved

hmpf requested a review from podliashanyk October 12, 2023 08:06

podliashanyk approved these changes Oct 12, 2023

View reviewed changes

src/zinolib/event_types.py Show resolved Hide resolved

hmpf mentioned this pull request Oct 12, 2023

Silently ignore rogue event IDs #24

Closed

Protect important comment

d344a10

hmpf merged commit 0c80bef into Uninett:main Oct 12, 2023
9 checks passed

hmpf deleted the decoupling branch October 12, 2023 09:05

podliashanyk mentioned this pull request Oct 17, 2023

Refactor after changes in zinolib Uninett/Howitz#24

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reanalyze events, history, log and decouple from implementation #18

Reanalyze events, history, log and decouple from implementation #18

hmpf commented Jun 22, 2023

github-actions bot commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

lunkwill42 left a comment

lunkwill42 Jun 23, 2023

hmpf Jun 28, 2023 •

edited

Loading

hmpf Aug 16, 2023

lunkwill42 Aug 17, 2023

lunkwill42 left a comment •

edited

Loading

lunkwill42 Aug 17, 2023

hmpf Aug 21, 2023

hmpf commented Oct 11, 2023

podliashanyk left a comment

hmpf commented Oct 12, 2023

podliashanyk left a comment •

edited

Loading

sonarcloud bot commented Oct 12, 2023

Reanalyze events, history, log and decouple from implementation #18

Reanalyze events, history, log and decouple from implementation #18

Conversation

hmpf commented Jun 22, 2023

github-actions bot commented Jun 22, 2023 • edited Loading

Test results

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

lunkwill42 left a comment

Choose a reason for hiding this comment

lunkwill42 Jun 23, 2023

Choose a reason for hiding this comment

hmpf Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

hmpf Aug 16, 2023

Choose a reason for hiding this comment

lunkwill42 Aug 17, 2023

Choose a reason for hiding this comment

lunkwill42 left a comment • edited Loading

Choose a reason for hiding this comment

lunkwill42 Aug 17, 2023

Choose a reason for hiding this comment

hmpf Aug 21, 2023

Choose a reason for hiding this comment

hmpf commented Oct 11, 2023

podliashanyk left a comment

Choose a reason for hiding this comment

hmpf commented Oct 12, 2023

podliashanyk left a comment • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Oct 12, 2023

github-actions bot commented Jun 22, 2023 •

edited

Loading

codecov bot commented Jun 22, 2023 •

edited

Loading

hmpf Jun 28, 2023 •

edited

Loading

lunkwill42 left a comment •

edited

Loading

podliashanyk left a comment •

edited

Loading