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

bridge: Extract some netlink operations code to file #998

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ormergi
Copy link
Contributor

@ormergi ormergi commented Jan 14, 2024

This is a small refactoring around netlink operations logic.
Setting link state up and waiting for link oper state code is moved to link package.

@ormergi
Copy link
Contributor Author

ormergi commented Jan 14, 2024

/cc @EdDev @AlonaKaplan

}

if idx == len(retries)-1 {
return nil, fmt.Errorf("timeout waiting for %q state up", linkName)
Copy link
Member

Choose a reason for hiding this comment

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

nit: please say what state the link is currently in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

func SetUp(linkName string) error {
link, err := netlink.LinkByName(linkName)
if err != nil {
return fmt.Errorf("failed to retrieve link: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

nit: please use %w to wrap errors everywhere.

Copy link
Contributor Author

@ormergi ormergi Jan 15, 2024

Choose a reason for hiding this comment

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

I didn't saw error being unwrapped in the bridge plugin code, also we didnt wrapped them before this change 🙂
Why is it necessary to wrap them here?

Copy link
Member

Choose a reason for hiding this comment

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

because all of this code was written before %w was a thing :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@squeed squeed left a comment

Choose a reason for hiding this comment

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

A few small nits, then this looks good!

@ormergi
Copy link
Contributor Author

ormergi commented Jan 15, 2024

@squeed thanks for the review 🙂 , please take another look.


link, err = netlink.LinkByName(linkName)
if err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

let's be consistent and return here fmt.Errorf("failed to retrieve link: %v", err) as in SetUp

if err := netns.Do(func(_ ns.NetNS) error {
link, err := netlink.LinkByName(args.IfName)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want a function for this? I understand the one for retries which includes some logic with the for loop but this is simple and not used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent was to improve readability, even though there is no much of logic its less code to read, and may reduce some cognitive load (I dont need to know we need to fetch the link in order to bring it down).

Also, with #997 changes, it makes the code read better:

...
else if !disableContIface {
        link.SetUp(ifaceName)
}
...

WDYT?

Copy link
Member

@mlguerrero12 mlguerrero12 Jan 16, 2024

Choose a reason for hiding this comment

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

no, I think this is simple enough. If you want to improve readability, then abstract all the logic inside if layer3 branch, then in your other pr, you can do

if !disableContIface {
	if layer {
		layer3func()
	} else {
		thiscode
	}
	WaitOperStateUp()
}

This is a small refactoring around netlink operations
logic.
Setting link state up and waiting for link oper state
code is moved to link package.

Signed-off-by: Or Mergi <[email protected]>
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.

3 participants