pixie icon indicating copy to clipboard operation
pixie copied to clipboard

feat: support fallback probes when a probe attach fails

Open CameronHall opened this issue 1 year ago • 5 comments

Summary: Support fallback probes when a probe attach fails

This should address the issue mentioned in https://github.com/pixie-io/pixie/issues/932. I ran into this while trying to run Stirling on WSL.

Type of change: /kind bug

Test Plan: TBD

CameronHall avatar Jan 05 '24 15:01 CameronHall

Thanks @ddelnano. I've addressed the linting issues

CameronHall avatar Jan 06 '24 05:01 CameronHall

@JamesMBartlett sorry it took so long. Updated it now.

CameronHall avatar Mar 09 '24 02:03 CameronHall

@CameronHall thanks again for getting this PR open. There was recent interest in this change from a community slack discussion, so I went ahead and added on top of your change to address the remaining feedback.

@JamesMBartlett std::unique_ptr runs into problems since this relies on a copy constructor being present. Instead of trying to working around that, I chose to use a std::shared_ptr. Let me know what you think.

ddelnano avatar May 09 '24 20:05 ddelnano

@JamesMBartlett std::unique_ptr runs into problems since this relies on a copy constructor being present. Instead of trying to working around that, I chose to use a std::shared_ptr. Let me know what you think.

I think we should reserve using shared pointers for when the memory is actually shared between two different owners (in this case its just allocated at init), so IMO we should just change the usage in BCCWrapper. Looking at it, it seems that the probe specs are only ever used as a const reference so we should just change the class to store const references instead of copying the probe spec.

JamesMBartlett avatar May 14 '24 05:05 JamesMBartlett

it seems that the probe specs are only ever used as a const reference so we should just change the class to store const references instead of copying the probe spec.

@JamesMBartlett are you talking about changing the kprobes_ member to that data type? I believe std::vector elements need to be assignable and after trying that I haven't been able to find an option that compiles. I feel like std::vector is probably not the right fit and a read only container would make it easier to accomplish what you described.

If that wasn't what you were thinking, let me know and I'm happy to try it.

ddelnano avatar May 14 '24 15:05 ddelnano