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

Ensure that batch APIs handle partial success conditions #162

Closed
4 tasks
javierhonduco opened this issue May 3, 2022 · 1 comment · Fixed by #374
Closed
4 tasks

Ensure that batch APIs handle partial success conditions #162

javierhonduco opened this issue May 3, 2022 · 1 comment · Fixed by #374
Assignees
Labels
API Break bug Something isn't working

Comments

@javierhonduco
Copy link
Contributor

javierhonduco commented May 3, 2022

More context on #159.

  • GetValueAndDeleteBatch
  • DeleteKeyBatch
  • GetValueBatch
  • UpdateBatch
javierhonduco pushed a commit to javierhonduco/parca-agent that referenced this issue May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this issue May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this issue May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this issue May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this issue May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handling errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial gets, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial gets, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 3, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 4, 2022
when dealing with partial gets, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
javierhonduco added a commit to javierhonduco/libbpfgo that referenced this issue Aug 4, 2022
when dealing with partial deletion, more context on aquasecurity#162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
grantseltzer pushed a commit that referenced this issue Aug 4, 2022
when dealing with partial gets, more context on #162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
grantseltzer pushed a commit that referenced this issue Aug 4, 2022
when dealing with partial deletion, more context on #162

Signed-off-by: Francisco Javier Honduvilla Coto <[email protected]>
@geyslan geyslan self-assigned this Aug 29, 2023
@geyslan
Copy link
Member

geyslan commented Aug 31, 2023

UpdateBatch is a special case. We cannot ignore the error of updating fewer entries than were requested.

Thinking about the Batch API, I suppose we should break it by returning the count updated by the kernel, thus leaving it up to the consumer to decide if it's a failure or not.

diff --git a/map-low.go b/map-low.go
index 7cf3d95..fd6ed73 100644
--- a/map-low.go
+++ b/map-low.go
@@ -247,7 +247,7 @@ func (m *BPFMapLow) DeleteKey(key unsafe.Pointer) error {
 // BPFMapLow Batch Operations
 //
 
-func (m *BPFMapLow) GetValueBatch(keys unsafe.Pointer, startKey, nextKey unsafe.Pointer, count uint32) ([][]byte, error) {
+func (m *BPFMapLow) GetValueBatch(keys unsafe.Pointer, startKey, nextKey unsafe.Pointer, count uint32) ([][]byte, uint32, error) {
        valueSize, err := calcMapValueSize(m.ValueSize(), m.Type())
        if err != nil {
                return nil, fmt.Errorf("map %s %w", m.Name(), err)
@@ -291,7 +291,7 @@ func (m *BPFMapLow) GetValueBatch(keys unsafe.Pointer, startKey, nextKey unsafe.
        return collectBatchValues(values, uint32(countC), valueSize), nil
 }
 
-func (m *BPFMapLow) GetValueAndDeleteBatch(keys, startKey, nextKey unsafe.Pointer, count uint32) ([][]byte, error) {
+func (m *BPFMapLow) GetValueAndDeleteBatch(keys, startKey, nextKey unsafe.Pointer, count uint32) ([][]byte, uint32, error) {
        valueSize, err := calcMapValueSize(m.ValueSize(), m.Type())
        if err != nil {
                return nil, fmt.Errorf("map %s %w", m.Name(), err)
@@ -328,7 +328,7 @@ func (m *BPFMapLow) GetValueAndDeleteBatch(keys, startKey, nextKey unsafe.Pointe
        return collectBatchValues(values, uint32(countC), valueSize), nil
 }
 
-func (m *BPFMapLow) UpdateBatch(keys, values unsafe.Pointer, count uint32) error {
+func (m *BPFMapLow) UpdateBatch(keys, values unsafe.Pointer, count uint32) (uint32, error) {
        countC := C.uint(count)
 
        optsC, errno := C.cgo_bpf_map_batch_opts_new(C.BPF_ANY, C.BPF_ANY)
@@ -355,7 +355,7 @@ func (m *BPFMapLow) UpdateBatch(keys, values unsafe.Pointer, count uint32) error
        return nil
 }
 
-func (m *BPFMapLow) DeleteKeyBatch(keys unsafe.Pointer, count uint32) error {
+func (m *BPFMapLow) DeleteKeyBatch(keys unsafe.Pointer, count uint32) (uint32, error) {
        countC := C.uint(count)
 
        optsC, errno := C.cgo_bpf_map_batch_opts_new(C.BPF_ANY, C.BPF_ANY)

@geyslan geyslan mentioned this issue Aug 31, 2023
@geyslan geyslan added the bug Something isn't working label Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Break bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants