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

Network Instance State Collector #3117

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Mar 24, 2023

The process of collecting state data and metrics for network instances (IP assignments of app interfaces, interface metrics, application flows) has been previously scattered all across the zedrouter codebase. This commit consolidates stata data collecting into a standalone component defined by an interface nistate.Collector, meaning that the default Linux-based implementation is replaceable, which opens up the possibility of adding native support for other network stacks to EVE.

Another important goal of this refactoring was to simplify the existing implementation and make it easier to understand. Previously, state data were collected by multiple Go routines and exchanged through some additional channels and pubsub topics. It was quite difficult to understand the code as a whole and to see what actually is the intended behavior of some of its parts.

@milan-zededa
Copy link
Contributor Author

All relevant Yetus issues were fixed.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Did you consider the stutter yetus comment on line 29 in pkg/pillar/nistate/statecollector.go?

If you don't want to rename it (e.g., to just Collector), then it makes sense to add a comment to make the complaint go away.

The process of collecting state data and metrics for network instances
(IP assignments of app interfaces, interface metrics, application flows)
has been previously scattered all across the zedrouter codebase.
This commit consolidates stata data collecting into a standalone
component defined by an interface nistate.Collector, meaning that the
default Linux-based implementation is replaceable, which opens up
the possibility of adding native support for other network stacks to EVE.
Another important goal of this refactoring was to simplify the existing
implementation and make it easier to understand. Previously, state data
were collected by multiple Go routines and exchanged through some
additional channels and pubsub topics. It was quite difficult to
understand the code as a whole and to see what actually is the
intended behaviour of some of its parts.

Signed-off-by: Milan Lenco <milan@zededa.com>
@milan-zededa
Copy link
Contributor Author

Did you consider the stutter yetus comment on line 29 in pkg/pillar/nistate/statecollector.go?

On second thought, I decided to do the renaming.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM (and ignoring the reported existing yetus issues for zedroutertypes.go).

But do we have any tests which verifies that we get the status, metrics, and flowlogs somewhere?

@milan-zededa
Copy link
Contributor Author

But do we have any tests which verifies that we get the status, metrics, and flowlogs somewhere?

In eden we do. However, these tests do not go very deep and do not cover all the fields of these published state data. After zedrouter refactoring I would like to have a look at eden tests and extend the existing ones plus add some more (for networking).

@eriknordmark eriknordmark merged commit 9bdfed1 into lf-edge:master Apr 3, 2023
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.

2 participants