-
Notifications
You must be signed in to change notification settings - Fork 9
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
update main module with vlan endpoint implementation #1
update main module with vlan endpoint implementation #1
Conversation
/cc @JanScheurich |
|
||
// Config holds configuration parameters from environment variables | ||
type Config struct { | ||
Name string `default:"vlan-vpp-responder" desc:"Name of vlan vpp responder"` |
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 seems to be in the name assigned to the parent interface. Linux interface names are limited to 15 characters. The default name here seems too long. Can it be shortened?
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.
yes jan, done.
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.
Usually this parameters is the name of the NSE... I would suggest considering not using it as the interface name, but rather generating that.
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.
sure, done.
case <-i.stop: | ||
return | ||
default: | ||
parentIf, err := netlink.LinkByName(i.parentIfName) |
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.
It's not clear to me when and how the parent interface is actually created and injected into the NSE pod.
It can't really happen at registration of the NSE or its grpc handler, right?
Does it happen implicitly by the forwarder when the first client connects? How can the ifServer go-routine be sure that the parent interface is already successfully injected by the forwarder when it processes the first "add" operation?
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 go routine is started at endpoint bringup as part of creation of ifconfig
server chain element (line 65-69). The teardown happens when the endpoint is deleted.
This waits for ifOp
entry on the ifOps
channel. The write of ifOp
happens at Request/Close method of ifconfig server chain element which happens for every NS client. so this go routine is run after all the elements (ipam, vlan etc.) in the chain are executed which is sufficient (as connection obj holds everything) to create vlan sub interface and configure ip, route etc. The parent interface is injected by forwarder, that's why this go routine is made to sleep for 1 sec in a busy wait when parent interface is not found.
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.
Oh, I didn't notice the sleep 1 loop. That's how you address the race. Should there be some "timeout" in case the parent interface never shows up and return an error?
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 suggest using netlink.LinkSubscribe instead of a Sleep. Whenever possible, subscribe to events, don't poll :)
Something like
done := make(<-chan struct{})
defer close(done)
ch := make(chan<- LinkUpdate,1)
defer close(ch)
if err := netlink.LinkSubscribe(ch,done);err != nil { ... }
parentIf, err := netlink.LinkByName(i.parentIfName)
if err != nil {
for _, event := range ch {
// Check for your interface appearing in LinkUpdate and break
}
}
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.
yes @edwarnicke , this is a good approach, changed it now, please have look.
90e329a
to
7995a17
Compare
Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
…ribe Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
7995a17
to
cb2af5d
Compare
Signed-off-by: Periyasamy Palanisamy <[email protected]>
798c385
to
84afff7
Compare
This adds the following:
Signed-off-by: Periyasamy Palanisamy [email protected]