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

BS: Max MaxExpTime configurable #2972

Merged

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Aug 8, 2019

This PR makes the MaxExpTime for segments configurable through the policy.

fixes #2968


This change is Reviewable

@oncilla oncilla added the BS label Aug 8, 2019
@oncilla oncilla requested a review from scrye August 8, 2019 06:43
@oncilla oncilla added this to the Q3S2 milestone Aug 8, 2019
@oncilla oncilla force-pushed the pub-bs-segment-length-configurable branch from 291bae5 to 23545fe Compare August 8, 2019 06:44
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)


go/beacon_srv/main.go, line 385 at r1 (raw file):

			MTU:        uint16(topo.MTU),
			Signer:     signer,
			MaxExpTime: maxExpTimeFactory(t.store, beacon.PropPolicy),

Rename MaxExpTime to something to more clearly indicate it's a function callback. It sounds static right now, which is misleading, it needs to have a verb. Something like GetMaxExpTime or ComputeMaxExpTime.


go/beacon_srv/main.go, line 385 at r1 (raw file):

			MTU:        uint16(topo.MTU),
			Signer:     signer,
			MaxExpTime: maxExpTimeFactory(t.store, beacon.PropPolicy),

What is the reasoning behind making this a callback instead of a number?


go/beacon_srv/internal/beacon/policy.go, line 186 at r1 (raw file):

	// MaxExpTime indicates the maximum value for the expiration time when
	// extending the segment.
	MaxExpTime *spath.ExpTimeType `yaml:"MaxExpTime"`

What happens if the value in the yaml file exceeds the maximum for uint8?


go/beacon_srv/internal/beacon/store.go, line 106 at r1 (raw file):

func (s *Store) MaxExpTime(policyType PolicyType) spath.ExpTimeType {
	switch policyType {
	case UpRegPolicy:

How does MaxExpTime impact UpSegmentRegistration/DownSegmentRegistration/CoreSegmentRegistration?


go/beacon_srv/internal/config/sample.go, line 41 at r1 (raw file):

const policiesSample = `
# Output a sample policy file by providing the -help-policy flag.

💯

Copy link
Contributor Author

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 17 files reviewed, 4 unresolved discussions (waiting on @scrye)


go/beacon_srv/main.go, line 385 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Rename MaxExpTime to something to more clearly indicate it's a function callback. It sounds static right now, which is misleading, it needs to have a verb. Something like GetMaxExpTime or ComputeMaxExpTime.

Done.


go/beacon_srv/main.go, line 385 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

What is the reasoning behind making this a callback instead of a number?

The store has ownership of the policies.
In the future we probably want support for reloadable policies.
Also, at some point we had a discussion, that we might want to store the policies in the database to ensure a consistent state (when they get more complex)

Having it as a callbacks gives us the following:

  1. Simple refactor now. (otherwise we need a way to extract the information and initialize the extender with it)
  2. Simple policy reloading

go/beacon_srv/internal/beacon/policy.go, line 186 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

What happens if the value in the yaml file exceeds the maximum for uint8?

parsing will fail.


go/beacon_srv/internal/beacon/store.go, line 106 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

How does MaxExpTime impact UpSegmentRegistration/DownSegmentRegistration/CoreSegmentRegistration?

It is used when terminating the segment to compute the MaxExpTime of the last hop field.

@oncilla oncilla force-pushed the pub-bs-segment-length-configurable branch from d5e2685 to 15251f5 Compare August 8, 2019 10:23
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

This PR makes the MaxExpTime for segments configurable through the
policy.

fixes scionproto#2968
@oncilla oncilla force-pushed the pub-bs-segment-length-configurable branch from 15251f5 to c3fa58e Compare August 8, 2019 11:59
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla oncilla merged commit 97efff4 into scionproto:master Aug 8, 2019
@oncilla oncilla deleted the pub-bs-segment-length-configurable branch August 8, 2019 12:13
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.

BS: Propagation policy should be used to configure segment lifetime
2 participants