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

Pass span context in ociwclayer #1402

Merged
merged 1 commit into from
May 23, 2022
Merged

Conversation

helsaawy
Copy link
Contributor

Bypass function in base package (.\layers.go) and use layer functions
defined in .\internal\wclayer, since those allow propagating (span)
context.

Signed-off-by: Hamza El-Saawy [email protected]

Bypass function in base packaged (layers.go) and use layer functions
defined in `internal/wclayer`, since those allow propagating (span)
context.

Signed-off-by: Hamza El-Saawy <[email protected]>
@helsaawy helsaawy requested a review from a team as a code owner May 18, 2022 18:44
@jterry75
Copy link
Contributor

Do we know why ociwclayer was using hcsshim Docker api's in the first place? Do we risk breaking anything here?

@helsaawy
Copy link
Contributor Author

Do we know why ociwclayer was using hcsshim Docker api's in the first place? Do we risk breaking anything here?

I dont think so. ociwclayer seems to only be passing an empty hcsshim.DriverInfo{} struct, so the filepath.Join(info.HomeDir, id) turns into a filepath.Join("", id).

@jterry75
Copy link
Contributor

Yea thats a v1 ISM. @TBBle - PTAL given your recent work on containerd that consumed this.

@TBBle
Copy link
Contributor

TBBle commented May 21, 2022

ociwclayer was using the public APIs because I unified it over from existing code in other repos (containerd and docker contained variations of this package's code), which only had access to the public APIs, and because all the other APIs used by containerd and Docker still take this empty DriverInfo.

Nothing should break from this change, since the callers are already passing a context into the public pkg/ociwclayer APIs.

I kind of wanted to make internal/wclayer (or some of its APIs) public too and just get rid of DriverInfo in non-legacy callers, but I expected to land a fully-tested Snapshotter in containerd first so there was a known-good state to iterate from, and that got jammed up on the Base Layer Manipulation PR. I was hoping to move over to the RS5 APIs (computestorage) since both Docker and containerd support RS5 onwards only, so perhaps exposing more of internal/wclayer isn't valuable anyway.

@helsaawy helsaawy merged commit ce36677 into microsoft:master May 23, 2022
@helsaawy helsaawy deleted the he/wclayer branch May 23, 2022 14:39
anmaxvl added a commit that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants