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

Major zookeeper support update; Switch from events to polling. #80

Merged
merged 2 commits into from
Apr 27, 2015
Merged

Major zookeeper support update; Switch from events to polling. #80

merged 2 commits into from
Apr 27, 2015

Conversation

kormat
Copy link
Contributor

@kormat kormat commented Apr 24, 2015

Using ZK watches meant events were being constantly generated, this
change switches over to polling the incoming ZK shared path every half
second.

Also:

  • Add a lot more error handling, especially for corner-cases.
  • Make the log output a lot less chatty
  • Added time checks around some of the heavy-weight ZK operations
  • Show a warning if we're failing to keep our propagation/registration
    intervals

I'm still not satisfied with the performance of this. I'm seeing reading of shared PCBs sometimes taking up to 10s, and propagation failing to keep inside its interval. I have an idea to fix this, but requires some more work.

(For #50)

@pszalach @shitz

Using ZK watches meant events were being constantly generated, this
change switches over to polling the incoming ZK shared path every half
second.

Also:
- Add a lot more error handling, especially for corner-cases.
- Make the log output a lot less chatty
- Added time checks around some of the heavy-weight ZK operations
- Show a warning if we're failing to keep our propagation/registration
  intervals
"""
desc = "Fetching list of %s PCBs from shared path" % name
entries = self.zk.get_shared_entries(
path,
Copy link
Contributor

Choose a reason for hiding this comment

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

general remark:
4 space indentation or align to a parenthesis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damnit, yes. More generally i'm strongly in favour of us using the Pep8 style guide.

Anyway, fixed, and in a few other places too.

@pszal
Copy link
Contributor

pszal commented Apr 27, 2015

One case we should consider: what happens in a case of network partition? E.g. one BS loses connection with ZK.
I think that disconnected BS should propagate, as it guarantees propagation when at least one BS is alive. Also redundant, propagated beacons are not a problem, as child AD will anyway selected the best ones.

What do you think?

pszal added a commit that referenced this pull request Apr 27, 2015
Major zookeeper support update; Switch from events to polling.
@pszal pszal merged commit 5f16561 into scionproto:master Apr 27, 2015
@kormat
Copy link
Contributor Author

kormat commented Apr 27, 2015

Hurm. So, that's possible, but it would complicate the already-complicated logic even further.

mawyss pushed a commit to mawyss/scion that referenced this pull request Jan 15, 2021
juagargi added a commit to juagargi/scion that referenced this pull request Oct 5, 2021
Squashed changes for DRKey, ported to latest upstream master branch.

This includes the following main commits:

* 707a5e6 DRKey ported from the old SCIONLab (scionproto#77)

  This is itself a squashed commit  containing the bulk of the newly
  added DRKey implementation.

* 5a623a2 Allow a second call to initQUICSockets. (scionproto#79)
* df3a258 unify personal annotations to JordiSubira (scionproto#80)
* 9380c76 fix Level 1 key Exchange (scionproto#81)

  Add sanity check in the drkey_fetcher which validates that the
  response srcIA matches the intended server IA.

* a90f354 Remove redundant fields from Lvl1Req/Resp (scionproto#83)
* 64f4c23 cs/drkey: handle situations where no path to a peer AS can be found (scionproto#90)

  This condition is now handled analog to a similar condition in
  github.com/scionproto/scion/go/pkg/trust.AuthRouter.ChooseServer

The main change applied to make this compatible with the master
branch was to resolve the renaming / moving of what was previously
pkg/sciond to now pkg/daemon.

Co-authored-by: JordiSubira <jordi.subira.nieto@gmail.com>
Co-authored-by: Juan A. Garcia Pardo <juagargi@gmail.com>
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