Move program attachment logic out of replaceDatapath()
#32468
Assignees
Labels
Comments
ti-mo
added
sig/loader
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 callattachXDPProgramorattachSKBProgramafterwards, but the benefit is we'll likely be able to get rid ofreplaceDatapathOptions.xdpMode.The text was updated successfully, but these errors were encountered: