Skip to content

feat: use registryMirror addon as Containerd mirror #1117

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

Merged
merged 4 commits into from
May 16, 2025

Conversation

dkoshkin
Copy link
Contributor

@dkoshkin dkoshkin commented May 2, 2025

What problem does this PR solve?:
Depends on #1116

Automatically sets the registryMirror as a Containerd mirror. We're not updating the Cluster object because this should not be a user controller configuration and the IP used as the mirror is determined based on the addon handler and the Service CIDRs.

Tested in a Docker cluster:

$ kubectl port-forward \
  --address=0.0.0.0 \
  --namespace registry-mirror-system \
  pod/registry-mirror-docker-registry-0 5000:5000
# Push an image tag that doesn't exist in dockerhub  
$ crane copy nginx:latest 0.0.0.0:5000/library/nginx:dkoshkin --insecure
$ kubectl run nginx-working --image=docker.io/library/nginx:dkoshkin
$ kubectl run nginx-should-be-broken --image=docker.io/library/nginx:dne
$ kubectl get pods 
NAME                                                              READY   STATUS              RESTARTS   AGE
cluster-autoscaler-0196931c-cb53-7abf-aa89-49c82c42ced5-86w5j8c   0/1     ContainerCreating   0          19m
nginx-should-be-broken                                            0/1     ErrImagePull        0          11m
nginx-working                                                     1/1     Running             0          11m

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from a16bb7f to 3f763d1 Compare May 2, 2025 22:53
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon branch from 683a3e3 to 6e6c70b Compare May 7, 2025 18:40
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from 042b7da to ce91230 Compare May 7, 2025 18:42
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon branch 6 times, most recently from dd251db to 816ffaa Compare May 8, 2025 22:44
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch 2 times, most recently from 6a7dde3 to 71bc9ca Compare May 8, 2025 22:57
@dkoshkin dkoshkin requested a review from supershal May 8, 2025 23:18
supershal
supershal previously approved these changes May 9, 2025
Base automatically changed from dkoshkin/feat-registry-mirror-addon to main May 13, 2025 18:35
@dkoshkin dkoshkin dismissed supershal’s stale review May 13, 2025 18:35

The base branch was changed.

Copy link
Contributor

This PR/issue depends on:

@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from 71bc9ca to d1405b5 Compare May 13, 2025 19:07
@dkoshkin dkoshkin requested a review from supershal May 13, 2025 19:08
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from d1405b5 to cd30704 Compare May 13, 2025 19:26
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from cd30704 to e8b1a8d Compare May 14, 2025 15:14
Copy link
Contributor

@supershal supershal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@manoj-nutanix manoj-nutanix left a comment

Choose a reason for hiding this comment

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

one q. if user removes registry config from cluster spec after deployment, will it generate new containerd patch without bootstrap registry config and apply it to live cluster?

@dkoshkin
Copy link
Contributor Author

dkoshkin commented May 15, 2025

one q. if user removes registry config from cluster spec after deployment, will it generate new containerd patch without bootstrap registry config and apply it to live cluster?

Thats correct, let me create a ticket, maybe we should have a validation webhook here to prevent this. Not sure though because there are lots of other input we expect clients to understand what changing it means and nothing prevents it there.

@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from c301a1d to 6cd49d1 Compare May 15, 2025 18:52
@dkoshkin dkoshkin force-pushed the dkoshkin/feat-registry-mirror-addon-as-mirror branch from 6cd49d1 to 0eb39bf Compare May 15, 2025 18:55
@dkoshkin dkoshkin enabled auto-merge (squash) May 15, 2025 19:26
@manoj-nutanix
Copy link
Contributor

manoj-nutanix commented May 16, 2025

one q. if user removes registry config from cluster spec after deployment, will it generate new containerd patch without bootstrap registry config and apply it to live cluster?

Thats correct, let me create a ticket, maybe we should have a validation webhook here to prevent this. Not sure though because there are lots of other input we expect clients to understand what changing it means and nothing prevents it there.

Makes sense. Either we block users to not remove registry config from cluster spec or we should ensure that removal will cleanup everything and it becomes connected site cluster. It would be good to have integration/e2e test for this behavior.

@dkoshkin dkoshkin merged commit f2818d7 into main May 16, 2025
23 checks passed
@dkoshkin dkoshkin deleted the dkoshkin/feat-registry-mirror-addon-as-mirror branch May 16, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants