Advertisement accept policies #181
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
fraborg
changed the title
fraborg
force-pushed
the
frb/policy-update
branch
from
dd16edf
to
9d76d78
Compare
fraborg
force-pushed
the
frb/policy-update
branch
from
9d76d78
to
2c03b0f
Compare
3 tasks
fraborg
force-pushed
the
frb/policy-update
branch
4 times, most recently
from
4355399
to
95b7845
Compare
fraborg
changed the title
fraborg
force-pushed
the
frb/policy-update
branch
2 times, most recently
from
7d45e19
to
d61f2f7
Compare
fraborg
force-pushed
the
frb/policy-update
branch
3 times, most recently
from
e7d633c
to
86c3441
Compare
fraborg
force-pushed
the
frb/policy-update
branch
3 times, most recently
from
ca70582
to
816f1e5
Compare
- AdvertisementConfig has been modified with 2 major fields, one for the broadcaster the other for the operator. - in AdvOperatorConfig, the autoAccept flag is now an enum with different types of acceptance policies
The approach now is "apply the config from now on": advertisements already accepted are not deleted if the config changes
added documentation for Cluster configuration
missing parameter in adv operator and peering request operator
fraborg
force-pushed
the
frb/policy-update
branch
from
816f1e5
to
f5abb20
Compare
mlavacca
approved these changes
Aug 28, 2020
frisso
reviewed
Aug 29, 2020
|
|
||
| const ( | ||
| // AutoAcceptWithinMaximum means all the Advertisement received will be accepted until the MaxAcceptableAdvertisement limit is reached | ||
| // AutoAcceptAll can be achieved by setting MaxAcceptableAdvertisement to infinite |
There was a problem hiding this comment.
@fraborg Sorry for coming so late but... what's the value for "infinite" here?
There was a problem hiding this comment.
the maximum value of int32 is 2147483647, but we can set the MaxAcceptableAdvertisement range from 0 to a value we decide, which can be our "infinite"
There was a problem hiding this comment.
I would write this in the code (or, better, the definition of the structure), so that users know what they can configure.
Btw, not sure if we're telling users if this number refers to the max number of contemporary advertisements accepted, or the total number over time. Just in case, make sure this is clearly explained.
frisso
reviewed
Aug 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR modifies the
ClusterConfigCRD to define a policy for the acceptance of Advertisements.The AdvertisementConfig has been split into 2 main fields:
AcceptPolicyis an enum which can have the following valuesAutoAcceptAllcan be achieved by setting MaxAcceptableAdvertisement to infiniteAutoRefuseAllcan be achieved by setting MaxAcceptableAdvertisement to 0Further policies can be easily added
The general approach towards config updates has changed too:
Other fixes
How Has This Been Tested?
test/controller.gototest/config-watcher.goadding Advertisement creation on a test cluster