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.

Sign up for GitHub

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

Jump to bottom

Address device <-> node addressing race #29555

Merged
merged 2 commits into from Dec 11, 2023
Merged

Address device <-> node addressing race #29555

merged 2 commits into from Dec 11, 2023

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Dec 1, 2023 •
edited

See commit messages.

Related: #29532

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 1, 2023
joamaki
joamaki approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

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

Few nits. Mostly needs comments.

daemon/cmd/daemon.go Outdated Show resolved Hide resolved
daemon/cmd/cells.go Outdated Show resolved Hide resolved
daemon/cmd/device-reloader.go Outdated Show resolved Hide resolved
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/fix-device-node-addr-race branch 2 times, most recently from a7eb615 to 1759b84 Compare December 4, 2023 11:59
@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 4, 2023

/test

@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 4, 2023

/ci-ginkgo

@bimmlerd bimmlerd added sig/loader Impacts the loading of BPF programs into the kernel. release-note/misc This PR makes changes that have no direct user impact. area/modularization labels Dec 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 4, 2023
@bimmlerd
daemon: add a device reloader that isn't as racy
cd0e8f2 
With the introduction of the NodeAddress table, and the reimplementation
of the NodeAddressing on top of it, we have introduced a race in the
current, experimental runtime device detection. It's possible that the
datapath is reloaded with a new set of devices which is not reflected in
the node addressing interface yet.

We want to get rid of device manager anyway, so use this opportunity to
replace this functionality with a device reloader (this patch), and kill
the racy Listen (next patch).

Fixes: 9d7e452 (datapath: Reimplement NodeAddressing around Table[NodeAddress])

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd
device-manager: remove listen (and its tests)
7687fa8 
With Listen now only being used in tests, remove the functionality and
its tests.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/fix-device-node-addr-race branch from 758d22c to 7687fa8 Compare December 4, 2023 15:16
@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 4, 2023

/ci-ginkgo

@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 4, 2023

/test

@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 5, 2023

CI triage

@bimmlerd
Copy link
Member Author

bimmlerd commented Dec 5, 2023

/ci-runtime

@bimmlerd bimmlerd marked this pull request as ready for review December 5, 2023 09:49
@bimmlerd bimmlerd requested review from a team as code owners December 5, 2023 09:49
@bimmlerd bimmlerd added the kind/bug This is a bug in the Cilium logic. label Dec 5, 2023
pippolo84
pippolo84 reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've left a question inline about the reloader job.
Also, what about adding some test for the new cell? I understand this is a bugfix and don't want to block on this, so it is fine for me to add them in a follow up PR.

daemon/cmd/device-reloader.go Show resolved Hide resolved
pippolo84
pippolo84 approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

💯

@joamaki joamaki added this pull request to the merge queue Dec 11, 2023
Merged via the queue into cilium:main with commit 0999f5a Dec 11, 2023
61 checks passed
@joestringer joestringer mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@ldelossa ldelossa Awaiting requested review from ldelossa ldelossa is a code owner automatically assigned from cilium/sig-datapath

Assignees
No one assigned
Labels
area/modularization kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bimmlerd @joamaki @pippolo84