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

Preserve results when NLM_F_DUMP_INTR is set #1050

Conversation

adrianmoisey
Copy link
Contributor

Similar to #1018, but for ConntrackDeleteFilters()

Relates to kubernetes/kubernetes#129562

return 0, err
}

errMsgs = append(errMsgs, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

this allows us to at least do a best effort to try to clean the entries matching the filter

Copy link
Contributor

@thaJeztah thaJeztah Jan 20, 2025

Choose a reason for hiding this comment

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

Just occurred to me; err may be nil here? Should probably be inside the err != nil branch? Perhaps also a comment couldn't hurt?

if err != nil {
	if  !errors.Is(err, ErrDumpInterrupted)  {
		return 0, err
	}
	// This allows us to at least do a best effort to try to clean the
	// entries matching the filter.
	errMsgs = append(errMsgs, err.Error())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you. Fixed in 2742887

@aojea
Copy link
Contributor

aojea commented Jan 17, 2025

LGTM

ping @aboch

Comment on lines 226 to 230

// Protocol returns "tcp".
func (*ProtoInfoTCP) Protocol() string {return "tcp"}
func (*ProtoInfoTCP) Protocol() string { return "tcp" }
func (p *ProtoInfoTCP) toNlData() ([]*nl.RtAttr, error) {
ctProtoInfo := nl.NewRtAttr(unix.NLA_F_NESTED | nl.CTA_PROTOINFO, []byte{})
ctProtoInfo := nl.NewRtAttr(unix.NLA_F_NESTED|nl.CTA_PROTOINFO, []byte{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some unrelated formatting changes made it in. It's a bit of a pain (my IDE also formats these on save), but better to handle those separate from this change;

  • can you revert the formatting changes
  • squash the commits? (second commit is fixing up the first one, and as the first one isn't merged yet, we may as well keep it as a single change).

I have a PR pending to fix formatting for the whole repo that's still pending; #1009

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry. I should have checked that before pushing. I'll fix it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! It's a bit of a pain for sure!

I'm not a maintainer on this repository, but hope that "gofumpt" PR gets accepted and merged, as I find myself undoing formatting changes every time 😂

@adrianmoisey adrianmoisey force-pushed the ErrDumpInterrupted-for-ConntrackDeleteFilters branch from 2742887 to bfa83ee Compare January 20, 2025 11:59
conntrack_linux.go Outdated Show resolved Hide resolved
Similar to vishvananda#1018, but for
ConntrackDeleteFilters()

Relates to kubernetes/kubernetes#129562
@adrianmoisey adrianmoisey force-pushed the ErrDumpInterrupted-for-ConntrackDeleteFilters branch from 01177bd to 665565b Compare January 20, 2025 14:31
Copy link
Contributor

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

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

LGTM

@aboch
Copy link
Collaborator

aboch commented Jan 21, 2025

LGTM

@aboch aboch merged commit 3642538 into vishvananda:main Jan 21, 2025
2 checks passed
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.

5 participants