-
Notifications
You must be signed in to change notification settings - Fork 125
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
API for SPAN #1430
API for SPAN #1430
Conversation
I did tests with ETCD: Create SPAN before starting vpp-agent without any interfaces
Add interfaces while vpp-agent is running
Update SPAN while vpp-agent is running
Delete SPAN while vpp-agent is running
Create SPAN again while vpp-agent is running
Stop and start vpp-agent
Show interfaces and SPAN table using `vppctl`
|
@rewenset could you also please test the scenario in which only agent is restarted, but VPP remains running? In that case the first resync transaction should do nothing (no operations) since everything is already configured as requested by etcd content. |
api/models/vpp/interfaces/span.proto
Outdated
option (gogoproto.messagename_all) = true; | ||
|
||
message Span { | ||
enum State { |
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.
Wouldn't it be better to call this parameter "Direction"? "State" sounds like something related to runtime status as opposed to configuration.
api/models/vpp/interfaces/span.proto
Outdated
string interface_from = 1; | ||
string interface_to = 2; | ||
State state = 3; | ||
bool is_l2 = 4; |
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.
Please fix the formatting (probably mixed tabs and spaces)
clientv2/linux/data_change_api.go
Outdated
@@ -136,6 +138,8 @@ type DeleteDSL interface { | |||
|
|||
// VppInterface adds a request to delete an existing VPP network interface. | |||
VppInterface(ifaceName string) DeleteDSL | |||
// VppSpan adds VPP span to the Delete request. | |||
VppSpan(span *vpp_interfaces.Span) DeleteDSL |
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.
Just call it Span across the client - no need to prefix with "Vpp" since there is no LinuxSpan
} | ||
if value.InterfaceTo == "" { | ||
return kvs.NewInvalidValueError(ErrSpanWithoutInterface, "interface_to") | ||
} |
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.
please check also that State is not UNKNOWN
Please address the review comments and then re-check the problematic agent-restart scenario with this bug fix applied: #1432 |
@rewenset Please fix the issues reported by Travis. |
First of all, sorry for not fixing tests and doing this agent-restart scenario earlier 🙏 "Restart Agent without restarting VPP" test(tested with bug fix applied: #1432) Show FullResync transaction
|
Related issue #1386
TODO: