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

Add MD.Clone function #177

Merged
merged 1 commit into from
Dec 28, 2024
Merged

Add MD.Clone function #177

merged 1 commit into from
Dec 28, 2024

Conversation

djdongjin
Copy link
Member

@djdongjin djdongjin commented Dec 26, 2024

MD is not thread-safe so sometimes we need to make a copy before modification, e.g. in containerd,

https://github.com/containerd/containerd/blob/c3efa0cb339ffc2d6c9f03d7fb4e5e6577d0194b/pkg/namespaces/ttrpc.go#L30-L36

This PR adds a MD.Clone function, similar to golang's http.Header.Clone (the code is copied there). Also add a benchmark that shows golang's implementation is better than a simple copy approach (both memory&latency):

$ go test -bench='Benchmark.*Clone' -benchmem
goos: linux
goarch: amd64
pkg: github.com/containerd/ttrpc
cpu: Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
BenchmarkMetadataClone/size=5-4          2663775               494.2 ns/op            80 B/op          1 allocs/op
BenchmarkMetadataClone/size=10-4          707372              1608 ns/op             871 B/op          2 allocs/op
BenchmarkMetadataClone/size=20-4          371546              2829 ns/op            1785 B/op          2 allocs/op
BenchmarkMetadataClone/size=50-4          194526              5800 ns/op            4436 B/op          3 allocs/op
BenchmarkSimpleMetadataClone/size=5-4            1809780               679.0 ns/op            80 B/op          5 allocs/op
BenchmarkSimpleMetadataClone/size=10-4            525523              2128 ns/op             871 B/op         11 allocs/op
BenchmarkSimpleMetadataClone/size=20-4            272677              4355 ns/op            2583 B/op         22 allocs/op
BenchmarkSimpleMetadataClone/size=50-4            100525             10794 ns/op            6798 B/op         55 allocs/op
PASS
ok      github.com/containerd/ttrpc     11.859s

metadata.go Outdated
Comment on lines 82 to 83
// Preserve nil values. ReverseProxy distinguishes
// between nil and zero-length header values.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we shouldn't copy this comment verbatim from the HTTP implementation. Any mention of a ReverseProxy is a potential source for confusion, as ttrpc which is typically used for communication between two process on the same physical machine, using a unix domain socket as the transport.

Maybe we should just leave

if vv == nil { // Preserve nil values.

here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @klihub . SGTM, updated the PR.

@djdongjin djdongjin requested a review from klihub December 27, 2024 15:58
@djdongjin djdongjin force-pushed the metadata-clone branch 2 times, most recently from b80dcd2 to 1a82100 Compare December 27, 2024 16:11

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Jin Dong <[email protected]>
@klihub klihub requested a review from fuweid December 27, 2024 17:41
@fuweid fuweid merged commit 3b8c8b7 into containerd:main Dec 28, 2024
11 checks passed
@djdongjin
Copy link
Member Author

Hi @fuweid thanks for approving/merging this PR! I wonder if we could do a ttrpc release recently (ideally also include this dependency upgrade PR #178 but it's optional).

This is mainly to fix a concurrent map write panic in otelttrpc (containerd/otelttrpc#2), which is reported in containerd/containerd#11138)

thanks

@fuweid
Copy link
Member

fuweid commented Dec 30, 2024

ping @djdongjin https://github.com/containerd/ttrpc/releases/tag/v1.2.7 this is new tag.

@djdongjin
Copy link
Member Author

ping @djdongjin https://github.com/containerd/ttrpc/releases/tag/v1.2.7 this is new tag.

Great, thanks for the quick release!

Mengkzhaoyun pushed a commit to open-beagle/containerd that referenced this pull request Feb 7, 2025
containerd 2.0.2

Welcome to the v2.0.2 release of containerd!

The second patch release for containerd 2.0 includes a number of bug fixes and improvements.

* Remove confusing warning in cri runtime config migration ([#11256](containerd/containerd#11256))
* Fix runtime platform loading in cri image plugin init ([#11248](containerd/containerd#11248))

* Update runc binary to v1.2.4 ([#11239](containerd/containerd#11239))

Please try out the release binaries and report any issues at
https://github.com/containerd/containerd/issues.

* Jin Dong
* Derek McGowan
* Akihiro Suda
* Kazuyoshi Kato
* Henry Wang
* Krisztian Litkey
* Phil Estes
* Samuel Karp
* Sebastiaan van Stijn
* Akhil Mohan
* Brian Goff
* Chongyi Zheng
* Maksym Pavlenko
* Mike Brown
* Pierre Gimalac
* Wei Fu
<details><summary>23 commits</summary>
<p>

* Prepare release notes for v2.0.2 ([#11245](containerd/containerd#11245))
  * [`cdaf4dfb4`](containerd/containerd@cdaf4df) Prepare release notes for v2.0.2
* Update platforms to latest rc ([#11259](containerd/containerd#11259))
  * [`eb125e1dd`](containerd/containerd@eb125e1) Update platforms to latest rc
* Remove confusing warning in cri runtime config migration ([#11256](containerd/containerd#11256))
  * [`468079c5c`](containerd/containerd@468079c) Remove confusing warning in cri runtime config migration
* Fix runtime platform loading in cri image plugin init ([#11248](containerd/containerd#11248))
  * [`a2d9d4fd5`](containerd/containerd@a2d9d4f) Fix runtime platform loading in cri image plugin init
* make sure console master tty is closed on task exit ([#11246](containerd/containerd#11246))
  * [`184ffad01`](containerd/containerd@184ffad) Add integ test to check tty leak
  * [`17181ed33`](containerd/containerd@17181ed) fix master tty leak due to leaking init container object
* Bump up otelttrpc to 0.1.0 ([#11242](containerd/containerd#11242))
  * [`8666e7422`](containerd/containerd@8666e74) Bump up otelttrpc to 0.1.0
* ctr: `ctr images import --all-platforms`: fix unpack ([#11236](containerd/containerd#11236))
  * [`c4270430d`](containerd/containerd@c427043) ctr: `ctr images import --all-platforms`: fix unpack
* Update runc binary to v1.2.4 ([#11239](containerd/containerd#11239))
  * [`7373ddd70`](containerd/containerd@7373ddd) update runc binary to v1.2.4
* downgrade go-difflib and go-spew to tagged releases ([#11222](containerd/containerd#11222))
  * [`f34147772`](containerd/containerd@f341477) downgrade go-difflib and go-spew to tagged releases
* Add a build tag to disable std `plugin` import ([#11213](containerd/containerd#11213))
  * [`dca769485`](containerd/containerd@dca7694) chore: add a build tag to disable containerd plugin import
* Update golangci to 1.60.3 ([#11187](containerd/containerd#11187))
  * [`5942b3fcb`](containerd/containerd@5942b3f) Update golangci to 1.60.3
</p>
</details>
<details><summary>6 commits</summary>
<p>

* Add dependabot and upgrade golang and dependency versions ([containerd/otelttrpc#3](containerd/otelttrpc#3))
  * [`2d46141`](containerd/otelttrpc@2d46141) upgrade golang, deps, CI versions
  * [`64922e7`](containerd/otelttrpc@64922e7) Add dependabot CI
* Fix concurrent map panic on metadata ([containerd/otelttrpc#2](containerd/otelttrpc#2))
  * [`2ba3be1`](containerd/otelttrpc@2ba3be1) Fix concurrent map panic on inject metadata
  * [`f50a922`](containerd/otelttrpc@f50a922) UT for concurrent inject/extract metadata
</p>
</details>
<details><summary>6 commits</summary>
<p>

* Move windows matcher logic so all platforms can use ([containerd/platforms#22](containerd/platforms#22))
  * [`7c58292`](containerd/platforms@7c58292) Move windows matcher logic so all platforms can use
* replace testify with stdlib in tests ([containerd/platforms#21](containerd/platforms#21))
  * [`86a86b7`](containerd/platforms@86a86b7) replace testify with stdlib in tests
* Replace arm64 minor variant logic with lookup table ([containerd/platforms#18](containerd/platforms#18))
  * [`364665a`](containerd/platforms@364665a) Replace arm64 minor variant logic with lookup table
</p>
</details>
<details><summary>5 commits</summary>
<p>

* Add MD.Clone function ([containerd/ttrpc#177](containerd/ttrpc#177))
  * [`430f734`](containerd/ttrpc@430f734) Add MD.Clone
* server: fix a Serve() vs. (immediate) Shutdown() race ([containerd/ttrpc#175](containerd/ttrpc#175))
  * [`c4d96d5`](containerd/ttrpc@c4d96d5) server: fix Serve() vs. immediate Shutdown() race.
  * [`ed6c3ba`](containerd/ttrpc@ed6c3ba) server_test: add Serve()/Shutdown() race test.
</p>
</details>

* **github.com/containerd/otelttrpc**  ea5083fda723 -> v0.1.0
* **github.com/containerd/platforms**  v1.0.0-rc.0 -> v1.0.0-rc.1
* **github.com/containerd/ttrpc**      v1.2.6 -> v1.2.7
* **github.com/davecgh/go-spew**       d8f796af33cc -> v1.1.1
* **github.com/pmezard/go-difflib**    5d4384ee4fb2 -> v1.0.0
* **github.com/stretchr/testify**      v1.9.0 -> v1.10.0

Previous release can be found at [v2.0.1](https://github.com/containerd/containerd/releases/tag/v2.0.1)
* `containerd-<VERSION>-<OS>-<ARCH>.tar.gz`:         ✅Recommended. Dynamically linked with glibc 2.31 (Ubuntu 20.04).
* `containerd-static-<VERSION>-<OS>-<ARCH>.tar.gz`:  Statically linked. Expected to be used on non-glibc Linux distributions. Not position-independent.

In addition to containerd, typically you will have to install [runc](https://github.com/opencontainers/runc/releases)
and [CNI plugins](https://github.com/containernetworking/plugins/releases) from their official sites too.

See also the [Getting Started](https://github.com/containerd/containerd/blob/main/docs/getting-started.md) documentation.
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.

None yet

4 participants