Repository navigation
fix: allow containers to start using a large numbers of ports - #4290
Conversation
|
Because of my modifications, CI fails. |
|
Why are we not storing the range itself in the label? |
|
Hi, @apostasie Thanks for commnets!
I also think of a way to keep the range of ports in the label, but the following edge case would violate the upper limit of the number of labels. nerdctl run -p 127.0.0.1:32000:32000 -p 127.0.0.2:32001:32001 ~ -p 127.0.0.254:32253:32253 nginxConsidering these cases, I think that when the port mapping information is given to the Label, it would violate the upper limit, so I implemented the way of this PR so that the port mapping information is not retained in the Label. When nerdctl compose up is executed, the ports specified in compose.yaml are expanded and executed by the following function: The ports are expanded and executed as |
02db289 to
c6e55d1
Compare
|
@AkihiroSuda so, this PR does move some of the network configuration information from labels (or annotations) to the filesystem. Future work to support per-network IP assignment (and options) will require significant re-jiggle of other annotations ( That overall does beg the question: should we stop using labels/annotations to store network configuration altogether, and just use a filesystem store for that instead? Answer to this question should inform whether we should consider expanding the scope of this PR a little bit to come up with a generic net-config-store that would in the future accommodate for other changes and additions ^. |
👍 We have to keep existing containers functional, though, if we are going to merge this in v2.1.x. |
|
Thanks for comments !!! I understand that it is acceptable to implement a policy of storing port mapping information in a local file. |
16ddd16 to
68534ed
Compare
|
Could you review when you have time ? 🙏 |
| Networks = Prefix + "networks" | ||
|
|
||
| // Ports is a JSON-marshalled string of []cni.PortMapping . | ||
| Ports = Prefix + "ports" |
There was a problem hiding this comment.
This change seems to break ports of containers created with an existing version of nerdctl ?
Then probably we should postpone this to v2.2.0
There was a problem hiding this comment.
Thanks for comments!!!
This change is not backward compatible, so these backward incompatible changes should be postponed until the update from 2.1 to 2.2.
If so, is it okay to leave this PR open for now until the release of 2.2 approaches?
I'll rebase as necessary and ask you to review this PR when the release of 2.2 approaches.
There was a problem hiding this comment.
Yes, please.
There should be also a human-friendly warning message when an incompatible container exists.
There was a problem hiding this comment.
I'll add handling for cases where there are containers that are not backward compatible (e.g., when containers created using the -p option are already exist).
|
What if we make this change backward compatible? eg: func LoadPortMappings(dataStore, namespace, id string) ([]cni.PortMapping, error) {
// if there is no port information from NetworkStore, look into the labels |
|
@haytok a few comments above - also I think we should make this backward compatible (at least for reading the ports from the labels if they are there). |
|
Hi @apostasie Thanks for commens!!! I hadn't considered backward compatibility at all, so I'll update based on your advices to make backward compatible. |
|
I've added the logic to maintain backward compatibility. However, maybe all failures are flaky ...
Details
2025-06-17T14:53:34.7593504Z container_logs_test.go:313:
2025-06-17T14:53:34.7593560Z
2025-06-17T14:53:34.7593715Z +------------------------------------------------------------------------------------------------------------+
2025-06-17T14:53:34.7594063Z | ➡️ | ⚙️ /usr/local/bin/nerdctl logs -f testlogswithoutnewlineoreof-f625832d |
2025-06-17T14:53:34.7594222Z +------------------------------------------------------------------------------------------------------------+
2025-06-17T14:53:34.7594417Z | 🌱 | HOME=/root |
2025-06-17T14:53:34.7594667Z | | PATH=/usr/local/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin |
2025-06-17T14:53:34.7594770Z | | :/bin |
2025-06-17T14:53:34.7595027Z | | DOCKER_CONFIG=/tmp/TestLogsWithoutNewlineOrEOF3385429920/003 |
2025-06-17T14:53:34.7595290Z | | NERDCTL_TOML=/tmp/TestLogsWithoutNewlineOrEOF3385429920/003/nerdctl.toml |
2025-06-17T14:53:34.7595526Z +------------------------------------------------------------------------------------------------------------+
2025-06-17T14:53:34.7595727Z | ⏰ | <1s (limit: 3m0s) |
2025-06-17T14:53:34.7595888Z +------------------------------------------------------------------------------------------------------------+
2025-06-17T14:53:34.7596214Z | 📁 | /tmp/TestLogsWithoutNewlineOrEOF3385429920/002 |
2025-06-17T14:53:34.7596372Z +------------------------------------------------------------------------------------------------------------+
2025-06-17T14:53:34.7596434Z
2025-06-17T14:53:34.7596977Z container_logs_test.go:313: 🔗 �]8;;file:///go/src/github.com/containerd/nerdctl/cmd/nerdctl/container/container_logs_test.go#313:1�testCase.Run(t)�]8;;��[0m
2025-06-17T14:53:34.7597042Z
2025-06-17T14:53:34.7597110Z <<<<<<<<<<<<<<<<<<<<
2025-06-17T14:53:34.7597242Z 🖊️ Inspecting output (equals)
2025-06-17T14:53:34.7597339Z 👀 testing: ``
2025-06-17T14:53:34.7597450Z ❌ FAILED! = `'Hello World!
2025-06-17T14:53:34.7597532Z There is no newline'`
2025-06-17T14:53:34.7597595Z >>>>>>>>>>>>>>>>>>>>
Details
Details
Details
|
|
Yes, all
|
| return nil, fmt.Errorf("failed to parse label %q=%q: %s", labels.Ports, portsJSON, err.Error()) | ||
| return ports, fmt.Errorf("failed to parse label %q=%q: %s", labels.Ports, portsJSON, err.Error()) | ||
| } | ||
| log.L.Warnf("container %s is using legacy port mapping configuration. To ensure compatibility with the new port mapping logic, please recreate this container. For more details, see: https://github.com/containerd/nerdctl/pull/4290", id[:12]) |
There was a problem hiding this comment.
Maybe we actually do not need this warning?
We are compatible with legacy port mapping, so, it is transparent for users - eg: there is no real reason to urge them to recreate containers until we break compatibility / remove the compat layer in 2.X.
Let see what others think about this though ^
There was a problem hiding this comment.
Based on the advice I received below, this warning message is displayed.
If at some stage (release) the logic for obtaining port mapping information from the label is to be deprecated, I think this warning message should be displayed.
However, I would like to hear the opinions of other maintainers as well.
There was a problem hiding this comment.
I think the advice was because at that time your PR was not backward compatible.
Since you did make it backward compatible now, it is less useful.
Anyhow, I don't care, this is a nit, so, fine with it :-)
|
Thanks @haytok - this is good progress on solving this.
This is really ugly ^ - docker does a much better job. I appreciate this aspect would be more complicated and possibly involve additional changes in compose, but yeah, I would much prefer that: |
|
Note, I am not suggesting that we try to reconstitute ranges from discrete ports specs, but that we do store the ranges when ranges are provided |
|
Thanks for comments!!! @apostasie I also think the First, I believe this is caused by the current implementation where the logic for displaying nerdctl/pkg/formatter/formatter.go Lines 115 to 128 in b60fbea I think this behavior occurs even in the latest version, but to fundamentally fix this readability issue, rather than saving ranges when port ranges are specified in For example, when we specify port ranges in Details
> cat compose.yaml
services:
nginx:
image: nginx:latest
container_name: nginx
ports:
- '80-81:80-81'
- '80-81:80-81/udp'
> sudo nerdctl compose up
...
INFO[0000] Running [/usr/local/bin/nerdctl run --cidfile=/tmp/compose-2285964703/cid -l=com.docker.compose.project=ec2-user -l=com.docker.compose.service=nginx -d --name=nginx --pull=never --net=ec2-user_default --hostname=nginx -p=80:80/tcp -p=81:81/tcp -p=80:80/udp -p=81:81/udp --restart=no nginx:latest]
...However, I think fixing this |
Yeah, that works too. Thanks @haytok |
|
Needs rebase |
|
Rebased. Thanks! |
|
Sorry, needs rebase again |
| return &test.Expected{ | ||
| ExitCode: 1, | ||
| Errors: []error{errdefs.ErrInvalidArgument}, | ||
| ExitCode: 0, |
There was a problem hiding this comment.
"Run a container with invalid ports, and then clean up." is no longer true?
Same applies to the test name too.
| return nil, fmt.Errorf("failed to parse label %q=%q: %s", labels.Ports, portsJSON, err.Error()) | ||
| return ports, fmt.Errorf("failed to parse label %q=%q: %s", labels.Ports, portsJSON, err.Error()) | ||
| } | ||
| log.L.Warnf("container %s is using legacy port mapping configuration. To ensure compatibility with the new port mapping logic, please recreate this container. For more details, see: https://github.com/containerd/nerdctl/pull/4290", id[:12]) |
There was a problem hiding this comment.
The name should be printed in addition to the hex ID?
Suppose we have a compose.yaml that allocates a large numbers of ports as
follows.
```
> cat compose.yaml
services:
svc0:
image: alpine
command: "sleep infinity"
ports:
- '32000-32060:32000-32060'
```
When we run `nerdctl compose up -d` using this compose.yaml, we will get
the following error.
```
FATA[0000] create container failed validation: containers.Labels: label key and value length (4711 bytes) greater than maximum size (4096 bytes), key: nerdctl/ports: invalid argument
FATA[0000] error while creating container haytok-svc0-1: error while creating container haytok-svc0-1: exit status 1
```
This issue is reported in the following issue.
- containerd#4027
This issue is considered to be the same as the one with errors when
trying to perform many port mappings, such as `nerdctl run -p 80:80 -p 81:81 ~ -p 1000:1000 ...`
The current implementation is processing to create a container with the
information specified in -p to the label.
And as can be seen from the error message, as the number of ports to be
port mapped increases, the creation of the container fails because it
violates the limit of the maximum number of bytes on the containerd side
that can be allocated for a label.
Therefore, this PR modifies the container creation process so that
containers can be launched without having to assign the information
specified in the -p option to the labels.
Specifically, port mapping information is stored in the following path,
and when port mapping information is required, it is retrieved from this
file.
```
<DATAROOT>/<ADDRHASH>/containers/<NAMESPACE>/<CID>/network-config.json
```
Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
|
I've resolved conflict, but some tests have failed ...
Details
Details
2025-06-25T09:51:34.1214885Z image_push_linux_test.go:282:
2025-06-25T09:51:34.1215101Z
2025-06-25T09:51:34.1215368Z <<<<<<<<<<<<<<<<<<<<
2025-06-25T09:51:34.1215618Z 🖊️ Command should succeed
2025-06-25T09:51:34.1216106Z 👀 testing: `command returned a non-zero exit code`
2025-06-25T09:51:34.1216414Z ❌ FAILED! is `<nil>`
2025-06-25T09:51:34.1216606Z >>>>>>>>>>>>>>>>>>>> |
|
🎉 |
|
Thanks for your reviews and CI retries!!! |
… plugins 1.9.1 Preemptive upgrade chasing the rare transient "nerdctl start failed" flake (never reproduced in ~440 stress runs; kept under observation). Fixes for regressions found after the upgrade: - scanner: nerdctl 2.2+ removed the nerdctl/ports label (containerd/nerdctl#4290), so port mappings no longer show up in container labels. Fall back to the nerdctl networkstore: readNetworkStorePorts() globs /var/lib/nerdctl/*/containers/<ns>/<id>/network-config.json and returns it in the legacy label JSON format. Verified: port forwarding works, survives snapshot resume. - initramfs: add the busybox `find` applet; the guest had no `find` at all, so the boot-time name-store cleanup in myinit was a silent no-op. Comment documents both name-store layouts (depth 4). - images: `docker save <name>` without a tag returned 404. findImageNamespace now matches a tagless ref against docker.io/library/<name>:latest (short-repo comparison), and a `:` in registry host:port is no longer mistaken for a tag separator (strip the tag only when `:` comes after the last `/`, applied in repo() as well). Docs: IMPROVEMENTS.md — follow-up on the stack upgrade and its two regressions, tagless-save fix note, stress results; ARCHITECTURE.md §4.5 — nerdctl 2.3.x supports --health-cmd, note kept for why the custom healthcheck runner stays. Verified live: run/ps/ports, compose build/up/down, docker build --load, buildx remote driver, save/rmi/load round-trip, resume with a running container (port 200 after service-restart), 100/100 stress runs clean.
Suppose we have a compose.yaml that allocates a large numbers of ports as follows.
When we run
nerdctl compose up -dusing this compose.yaml, we will get the following error.This issue is reported in the following issue.
This issue is considered to be the same as the one with errors when trying to perform many port mappings, such as
nerdctl run -p 80:80 -p 81:81 ~ -p 1000:1000 ...The current implementation is processing to create a container with the information specified in
-pto the label.And as can be seen from the error message, as the number of ports to be port mapped increases, the creation of the container fails because it violates the limit of the maximum number of bytes on the containerd side that can be allocated for a label.
Therefore, this PR modifies the container creation process so that containers can be launched without having to assign the information specified in the
-poption to the labels.Specifically, port mapping information is stored in the following path, and when port mapping information is required, it is retrieved from this file.