-
Notifications
You must be signed in to change notification settings - Fork 40
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
configure bgp during early networking #4480
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rcgoodfellow This looks very good. I suggest testing the upgrade before merge though.
@@ -103,6 +103,17 @@ pub struct BgpPeerConfig { | |||
pub port: String, | |||
/// Address of the peer. | |||
pub addr: Ipv4Addr, | |||
/// How long to keep a session alive without a keepalive in seconds. | |||
/// Defaults to 6. | |||
pub hold_time: Option<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a Duration
typically for these, but if you want/need to keep them as u64
then I'd suggest adding a suffix like s
as in hold_time_s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently part of the data stored in the bootstore, but I think because these are all optional fields deserialization should still work. I would suggest testing the upgrade path though, where you initialize a rack with the old version and then install the new to see what happens.
@@ -132,6 +132,51 @@ | |||
"format": "uint32", | |||
"minimum": 0.0 | |||
}, | |||
"connect_retry": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we rewrite the sled-plan after RSS time, so this should be fine. I also just confirmed that adding a sled doesn't create a plan, and so we don't have to worry about reading an old plan with a new schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrade tested on madrid. Ship it!
Also brings in the following which fixes GitHub rate limit errors in the deploy CI job.
To test this I supplied the following
config-rss.toml
file to sled-agent in a single-node Omicron development environment and the system came up with an established BGP session.Updated docs for this RSS config are in