-
Notifications
You must be signed in to change notification settings - Fork 522
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
docker, containerd: image registry mirrors #1629
docker, containerd: image registry mirrors #1629
Conversation
Taking into draft since I forgot to write a new migration for the setting. |
41fc9ed
to
676865a
Compare
sources/api/migration/migrations/v1.1.3/image-registry-mirrors/src/main.rs
Outdated
Show resolved
Hide resolved
676865a
to
7c5460c
Compare
Push below and above addresses @tjkirch's comments. |
7c5460c
to
87b6393
Compare
LGTM assuming migrations test OK. |
README.md
Outdated
@@ -407,6 +407,19 @@ These settings can be changed at any time. | |||
Supported values are `debug`, `info`, `warn`, `error`, and `crit`, and the default is `info`. | |||
* `settings.ecs.enable-spot-instance-draining`: If the instance receives a spot termination notice, the agent will set the instance's state to `DRAINING`, so the workload can be moved gracefully before the instance is removed. Defaults to `false`. | |||
|
|||
#### Image registry settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talk about OS images in this readme too, so it might be worthwhile to call out that this is specific to container images.
#### Image registry settings | |
#### Container image registry settings |
README.md
Outdated
@@ -407,6 +407,19 @@ These settings can be changed at any time. | |||
Supported values are `debug`, `info`, `warn`, `error`, and `crit`, and the default is `info`. | |||
* `settings.ecs.enable-spot-instance-draining`: If the instance receives a spot termination notice, the agent will set the instance's state to `DRAINING`, so the workload can be moved gracefully before the instance is removed. Defaults to `false`. | |||
|
|||
#### Image registry settings | |||
|
|||
The following setting is optional and allows you to configure image registry mirrors and pull-through caches for your orchestrated containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only cover orchestrated containers and not host containers? Should it cover both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this currently only covers orchestrated containers (containers via cri-containerd).
For host-containers it's not immediately clear to me how I can configure the containerd client to use registry mirrors. I suspect I'd have to modify resolver to accomplish that? I can create a separate issue to track registry mirrors for host-containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be something that would have to be done in host-ctr
. I'm not sure if we should have the setting called settings.container-registry.mirrors
if it only controls orchestrated containers though; how would we change this to handle host containers in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it's just a bug that host-ctr
and host containers wouldn't support the mirror settings - same as if a component doesn't support the system-wide proxy settings.
87b6393
to
663d141
Compare
663d141
to
82ac125
Compare
Push above rebases onto develop and fixes conflicts |
82ac125
to
21a1abe
Compare
Push above addresses @bcressey 's comment.
tested things and they still work. |
21a1abe
to
18c21bd
Compare
Missed some dangling references to |
18c21bd
to
e10e82e
Compare
Rebases onto develop and fixes conflicts with migrations. |
Force push addresses @samuelkarp 's comments.
Force push fixes whitespacing issues with the JSON config template and other small typos. |
a185a00
to
7d949ee
Compare
Push above reverts the change to the configuration file format from TOML to JSON. For example, for the following template:
The
I tried a bunch of variations of setting the condition, but it just comes down to We technically can write a new helper that conditionally renders the Tested changes and everything still work as described in the PR testing description. |
7d949ee
to
a05aabb
Compare
I already wrote one I though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple nits, but nothing that need to block this.
a05aabb
to
2243f84
Compare
Push above addresses @arnaldo2792 and @samuelkarp 's comments. Will rebase shortly to fix conflicts |
2243f84
to
ea32d34
Compare
Push above rebases onto develop and fixes conflicts. |
"(1.1.4, 1.2.0)" = [ | ||
"migrate_v1.2.0_container-registry-mirrors.lz4", | ||
"migrate_v1.2.0_container-registry-config-restarts.lz4", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to fix up migrations in a different commit? If not then these migrations should be with the hostname ones.
packages/os/host-ctr-registry-config
Outdated
@@ -0,0 +1,6 @@ | |||
{{~#if settings.container-registry.mirrors}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it might be better to call this file host-ctr.toml
to make room for other potential settings - but I'm also happy to defer that until we need more settings.
Adds a new setting `settings.container-registry.mirrors` that lets users set pull-through caches for container image registries.
ea32d34
to
6c72595
Compare
Push above just rebases onto develop and fixes conflicts. |
Adds support for configuring container image registrys for host-containers and bootstrap containers. A lot of the logic is borrowed from containerd-cri plugin's implementation of image registry mirrors.
6c72595
to
3d45ba3
Compare
Push above addresses comments from @samuelkarp and @bcressey
|
Adds migrations for the new `container-registry` setting and the new docker daemon config file and the new config file for host-containers and bootstrap containers.
3d45ba3
to
62e69aa
Compare
Fixed up Release.toml migrations grouping. |
Issue number:
Resolves #1577
Description of changes:
Testing done:
Built AMIs and launched an ECS host and an K8S host.
Set up a pull-through cache for docker hub on a separate host:
Set the image registry mirror to the pull-through cache host on both ECS and K8S hosts:
Tried running a nginx task on in the ECS cluster, observed that the image was pulled from the cache
The pull-through cache registry is responding to the requests for manifests and blobs with HTTP status code 200:
Ran an nginx pod in my K8S cluster that has a single node:
The image got pulled from the pull-through cache registry just like above:
For host-containers and bootstrap-containers:
Started host-container that just runs nginx:
As soon as the host-container started, I observed traffic through the pull-through cache from my instance:
For bootstrap-containers, I defined a bootstrap container that just runs nginx along with the image registry config in my userdata.
Launched instance and bootstrap container started as expected and I observed corresponding traffic through the pull through cache.
Tested migrations and they work as expected
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.