etcd: use MCR v2 image - #66
Conversation
ef66810 to
c548ce4
Compare
Garrett Settles (gsettles01)
changed the title
Garrett Settles (gsettles01)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The wrapper now hardcodes an entrypoint path that can unintentionally break existing ETCD_IMAGE override usage unless the entrypoint remains configurable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the ACL etcd container runtime wiring to use the MCR v2 etcd image by default while preserving the existing service contract and version/tag substitution mechanism.
Changes:
- Switch the default
ETCD_IMAGE_URLfromquay.io/coreos/etcdtomcr.microsoft.com/oss/v2/etcd-io/etcdin the systemd unit. - Update the docker
--entrypointused by the wrapper from/usr/local/bin/etcdto/usr/bin/etcd.
File summaries
| File | Description |
|---|---|
| sdk_container/src/third_party/coreos-overlay/app-admin/etcd-wrapper/files/etcd-wrapper | Updates the container entrypoint used when launching etcd via the wrapper. |
| sdk_container/src/third_party/coreos-overlay/app-admin/etcd-wrapper/files/etcd-member.service | Changes the default etcd image registry/repo to MCR v2 while keeping the existing tag templating. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The registry move looks right, but I think the arch-suffixed tag needs to move with it.
local tag="v${PV}"
if [[ "${ARCH}" != "amd64" ]]; then
tag+="-${ARCH}"
fiSo on ARM64 the unit ends up with
With The good news is no per-arch tag is needed - --- a/sdk_container/src/third_party/coreos-overlay/app-admin/etcd-wrapper/etcd-wrapper-3.5.16.ebuild
+++ b/sdk_container/src/third_party/coreos-overlay/app-admin/etcd-wrapper/etcd-wrapper-3.5.16.ebuild
src_install() {
- local tag="v${PV}"
- if [[ "${ARCH}" != "amd64" ]]; then
- tag+="-${ARCH}"
- fi
+ # MCR publishes a single multi-arch index per version; no per-arch tag suffix.
+ local tag="v${PV}"Also curious how the targeted ARM64 run in the test methodology passed - was that using an explicit |
There was a problem hiding this comment.
🔵 Needs a closer look
The entrypoint path is now hard-coded, which can break existing image override scenarios that previously worked with the old default image layout.
Review details
Suppressed comments (1)
sdk_container/src/third_party/coreos-overlay/app-admin/etcd-wrapper/files/etcd-wrapper:36
- The container entrypoint path is now hard-coded to
/usr/bin/etcd. This can break existingETCD_IMAGE/ETCD_IMAGE_URLoverrides that point at images where the etcd binary lives elsewhere (including the previously defaultquay.io/coreos/etcd, which this wrapper previously invoked via/usr/local/bin/etcd). Consider making the entrypoint configurable via an env var (defaulting to/usr/bin/etcd) so overrides remain functional.
/usr/libexec/sdnotify-proxy /run/etcd-notify /usr/bin/docker run --name etcd-member --network=host --ipc=host -u $(id -u ${ETCD_USER}):$(id -g ${ETCD_USER}) -v /run:/run -v /usr/share/ca-certificates:/usr/share/ca-certificates:ro -v ${etcd_data_dir}:/var/lib/etcd:rw -v ${ETCD_SSL_DIR}:/etc/ssl/certs:ro --env-file <(env; echo PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin; echo NOTIFY_SOCKET=/run/etcd-notify) --entrypoint /usr/bin/etcd ${ETCD_IMAGE:-${ETCD_IMAGE_URL}:${ETCD_IMAGE_TAG}} "$@"
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Good catch—you’re right. The CPU architecture suffix was specific to Quay’s tag layout. I removed that conditional, so both AMD64 and ARM64 now use the MCR image tag v3.5.16. Docker will automatically select the correct architecture from that multi-architecture image on MCR. Regarding your question about why the earlier ARM64 test passed, the code that would add the arm64 suffix never ran - the pipeline run took the RPM path instead of portage. To cover the gap, I asked AI to build a small script to check the ebuild tag render logic without doing a full Portage build. That and I overrode asking for it to explicitly run the etcd tests in addition. Here's that commit and pipeline run. The pipeline run shows Thanks for catching the tag coverage gap and helping tighten this up :- ) |
Per Copilot's PR #66 review, custom images may use a different etcd path.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a core control-plane dependency’s image source/entrypoint and the PR metadata indicates CI/full validation is still pending, so it needs final human verification.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
0bb32a9 to
6e8832d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the default runtime image source and the etcd container entrypoint path, which is a high-impact boot/service behavior change that benefits from final human validation (and CI completion) before approval.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Move the default etcd image from Quay to MCR v2. Update the wrapper entrypoint to /usr/bin/etcd to match the MCR v2 image layout.
6e8832d to
607eaef
Compare
Summary
Move ACL's default etcd image from
quay.io/coreos/etcdtomcr.microsoft.com/oss/v2/etcd-io/etcd, retainingv3.5.16and existing service behavior. MCR v2 providesv3.5.16andv3.5.22for AMD64 and ARM64. Update the entrypoint to/usr/bin/etcd.Change Log
ETCD_IMAGE_URLinetcd-member.service./usr/local/bin/etcdto/usr/bin/etcd.Type of Change
Does this affect the image build?
Associated Issues
Related PRs
Test Methodology
git diff --checkpass.Merge Checklist