-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Move program attachment logic out of replaceDatapath()
#32468
Labels
Comments
ti-mo
added
sig/loader
Impacts the loading of BPF programs into the kernel.
kind/tech-debt
Technical debt
labels
May 10, 2024
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 13, 2024
…Datapath This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window a potential 'rollback' can happen in (see code comments) due to the risks involved, and it never being correct to begin with. - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 14, 2024
This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window a potential 'rollback' can happen in (see code comments) due to the risks involved, and it never being correct to begin with. - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 15, 2024
This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window a potential 'rollback' can happen in (see code comments) due to the risks involved, and it never being correct to begin with. - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 16, 2024
This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window during which a 'revert' can happen (see code comments) due to the risks involved and its questionable effectiveness - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 17, 2024
This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window during which a 'revert' can happen (see code comments) due to the risks involved and its questionable effectiveness - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
added a commit
to ti-mo/cilium
that referenced
this issue
May 22, 2024
This unblocks cilium#29333. See cilium#32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window during which a 'revert' can happen (see code comments) due to the risks involved and its questionable effectiveness - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: cilium#32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
github-merge-queue bot
pushed a commit
that referenced
this issue
May 22, 2024
This unblocks #29333. See #32468 for more context. This commit refactors replaceDatapath() to loadDatapath() by factoring device attachment out of the function into the caller. The main reasons are flexibility and transparency. replaceDatapath() was called from many places and needed to do a lot. This change is the first step to handing individual callers an object representing actual bpf object handles, so they can correctly manage its lifecycle. In the future, ebpf.LoadAndAssign will be used for better readability. Some callers attach the same program to multiple interfaces, some attach multiple programs (ingress/egress) to the same interface, and some use a mixture of both. This has caused loops to creep into replaceDatapath, giving it many arguments and many overall responsibilities, making it hard to form intuition around. Major changes made in this commit: - lifted attach{SKB,XDP}Program out of the function, into all callers, making them call attach* methods explicitly - removed `replaceDatapathOptions` - reduced the window during which a 'revert' can happen (see code comments) due to the risks involved and its questionable effectiveness - removed a few points where context cancellations are obeyed, to be continued in a subsequent commit Fixes: #32468 Signed-off-by: Timo Beckers <timo@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
This will unblock #29333, but this is one step to removing
replaceDatapath()
altogether. See the issue for full context.At its core, this will require returning an additional value from
replaceDatapath()
, either a loaded Collection or a custom struct for use withLoadAndAssign()
, but the latter will require a static CollectionSpec.Maps key for cilium_calls_*, which needs #32059 to land first.In addition to calling
replaceDatapath()
, callers will also have to explicitly callattachXDPProgram
orattachSKBProgram
afterwards, but the benefit is we'll likely be able to get rid ofreplaceDatapathOptions.xdpMode
.The text was updated successfully, but these errors were encountered: