Skip to content

link_linux: Add deserialization of IFF_RUNNING flag#1038

Merged
aboch merged 1 commit intovishvananda:mainfrom
dylandreimerink:link-running-flag-deserialization
Jan 21, 2025
Merged

link_linux: Add deserialization of IFF_RUNNING flag#1038
aboch merged 1 commit intovishvananda:mainfrom
dylandreimerink:link-running-flag-deserialization

Conversation

@dylandreimerink
Copy link
Copy Markdown
Contributor

@dylandreimerink dylandreimerink commented Dec 12, 2024

I noticed that the decoded link flags never include net.FlagRunning. It seems deserialization logic was never added for this particular flag. Having access to the running flag is actually useful to detect when a link is in a NO-CARRIER state, which happens when a layer 1 issue occurs such as an unplugged or broken cable.

I also removed the comment since the function now deviates from the original.

@aboch
Copy link
Copy Markdown
Collaborator

aboch commented Dec 12, 2024

Thank you @dylandreimerink
Please check the test failure is related
image

@dylandreimerink
Copy link
Copy Markdown
Contributor Author

dylandreimerink commented Dec 12, 2024 •
edited
Loading

Hmm, it seems net.FlagRunning was added in go v1.20. But CI run go v1.17. Go v1.22 is currently the earliest supported version according to the go release policy, so would it be reasonable to bump the minimum go version?

@dylandreimerink
Copy link
Copy Markdown
Contributor Author

dylandreimerink commented Dec 13, 2024 •
edited
Loading

I bumped the Go version, which seems to have fixed the original failure. But now we have a new one:

=== RUN   TestRuleListFiltered/IPv6/returns_rules_filtered_by_fwmark_0/0xFFFFFFFF#01
    rule_test.go:592: Expected len: 1, got: 0

        --- PASS: TestRuleListFiltered/IPv6/returns_rules_filtered_by_fwmark_0/0xFFFFFFFF (0.00s)
        --- PASS: TestRuleListFiltered/IPv6/returns_rules_filtered_by_fwmark_0x1234/0 (0.00s)
        --- FAIL: TestRuleListFiltered/IPv6/returns_rules_filtered_by_fwmark_0/0xFFFFFFFF#01 (0.00s)

Interestingly the suite contains the exact same test twice (hence the #01 at the end of the last sub-test name). Both the name, and the test case is the same: First and Second. But it is only on the second that fails.

Not sure if this is test flakyness or something the Go bump might have caused 🤔 .

@aboch
Copy link
Copy Markdown
Collaborator

aboch commented Jan 8, 2025 •
edited
Loading

Thanks @dylandreimerink
The #01 suffix on those tests is there in older runs (w/ go1.17) as well.

Can you please push a separate PR to bump go to 1.22. This project follows a 1 fix/change/feature per PR.

Thanks

@dylandreimerink
Copy link
Copy Markdown
Contributor Author

dylandreimerink commented Jan 14, 2025

Can you please push a separate PR to bump go to 1.22. This project follows a 1 fix/change/feature per PR.

Understandable. I will make a separate PR for the go version bump and rebase this PR once that one is in.

@dylandreimerink dylandreimerink mentioned this pull request Jan 14, 2025
Merged
@aboch
Copy link
Copy Markdown
Collaborator

aboch commented Jan 16, 2025

please rebase to latest

@dylandreimerink
link_linux: Add deserialization of IFF_RUNNING flag
ace2555 
Add deserialization of the `IFF_RUNNING` link flag which translates to
`net.FlagRunning`.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink force-pushed the link-running-flag-deserialization branch from f817abd to ace2555 Compare January 20, 2025 12:52
@aboch
Copy link
Copy Markdown
Collaborator

aboch commented Jan 21, 2025

LGTM

@aboch aboch merged commit 86d2f69 into vishvananda:main Jan 21, 2025
tklauser added a commit to tklauser/netlink that referenced this pull request Jan 21, 2025
@tklauser
go.mod: bump to Go 1.20
e503761 
PR vishvananda#1038 started using
net.FlagRunning which was introduced in Go 1.20, see
https://go.dev/doc/go1.20#netpkgnet. Bump the minimum required Go
version in go.mod accordingly and run `go mod tidy`.
@tklauser tklauser mentioned this pull request Jan 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@borkmann borkmann borkmann approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dylandreimerink @aboch @borkmann