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

feat: replace image registry dynamically #8018

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

cjc7373
Copy link
Contributor

@cjc7373 cjc7373 commented Aug 23, 2024

When does image replcaement happen

Bascially, replacement will happen when workload resources (like pod, job, deployment, statefulset) are created.

Specifically, the replacement will inject to:

  • instanceset controller, so that it affects:

    • new cluster creation
    • cluster upgrade

    But it won't affect:

    • cluster scaling
    • instance template
  • addon controller, so that it affects helm install jobs

  • backup/restore controller

Replace Configs

sample config:

registries:
  defaultRegistry: foo.bar
  defaultNamespace: default
  registriyConfig:
  - from: docker.io
    to: docker-mirror.io
  - from: k8s.io
    to: company-registry.io
    registryDefaultNamespace: k8sio
    namespaces:
    - from: special-ns
      to: my-special-ns

The replacement rules are as follows:

  • If there's no specific registry config (registriyConfig field), or registry does not match any specific registry config, defaultRegistry and defaultNamespace will be used.
    • If defaultNamespace is not defined, namespace remains unchanged.
    • If defaultRegistry is not defined, registry remains unchanged.
  • If image's registry matches a specific registry config:
    • from and to should not be empty. Registry to will be used.
    • If there's no namespace config, or namespace does not match any specific namespace config, registryDefaultNamespace will be used. If registryDefaultNamespace is not defined, namespace remains unchanged.
    • If namespace matches a specific namespace config, namespace to will be used.
      • from and to of the namespace may be empty (empty namespace is legal)

Other things to note

One side effect of this change is that omitted image name (e.g. busybox) will be expanded to full format (e.g. docker.io/library/busybox). IMO this will not have any user facing change.

@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines. label Aug 23, 2024
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 74.74747% with 25 lines in your changes missing coverage. Please review.

Project coverage is 62.65%. Comparing base (d550d0c) to head (0d2f2dd).
Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controllerutil/image_util.go 81.60% 15 Missing and 1 partial ⚠️
controllers/apps/transformer_component_workload.go 25.00% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8018      +/-   ##
==========================================
+ Coverage   62.57%   62.65%   +0.07%     
==========================================
  Files         324      325       +1     
  Lines       40290    40387      +97     
==========================================
+ Hits        25213    25305      +92     
- Misses      12775    12786      +11     
+ Partials     2302     2296       -6     
Flag Coverage Δ
unittests 62.65% <74.74%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cjc7373 cjc7373 marked this pull request as ready for review August 26, 2024 03:00
}

if dstNamespace == "" {
dstNamespace = namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

so if namespace is not found, the original namespace rather than the defaultNamespace is used.

Copy link
Contributor

@zjx20 zjx20 Sep 6, 2024

Choose a reason for hiding this comment

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

Maybe add a DefaultNamespace field to the RegistryConfig struct:

type RegistryConfig struct {
	From       string
	To         string
	DefaultNamespace string
	Namespaces []RegistryNamespaceConfig
}

When the namespace is not found in Namespaces, use the DefaultNamespace if it's not empty, otherwise use the original namespace.

Copy link
Contributor Author

@cjc7373 cjc7373 Sep 9, 2024

Choose a reason for hiding this comment

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

I think simply use the global defaultNamespace is enough.

It does seem better.

@cjc7373 cjc7373 requested review from leon-inf and a team as code owners September 9, 2024 09:19
@github-actions github-actions bot added size/XL Denotes a PR that changes 500-999 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Sep 9, 2024
pkg/controllerutil/image_util.go Show resolved Hide resolved
pkg/controllerutil/image_util.go Outdated Show resolved Hide resolved
pkg/controllerutil/image_util.go Outdated Show resolved Hide resolved
pkg/controllerutil/image_util.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/L Denotes a PR that changes 100-499 lines. and removed size/XL Denotes a PR that changes 500-999 lines. labels Sep 10, 2024
@cjc7373
Copy link
Contributor Author

cjc7373 commented Sep 10, 2024

@weicao @zjx20 PTAL

@@ -52,4 +52,6 @@ const (
CfgKBReconcileWorkers = "KUBEBLOCKS_RECONCILE_WORKERS"
CfgClientQPS = "CLIENT_QPS"
CfgClientBurst = "CLIENT_BURST"

CfgRegistries = "registries"
Copy link
Contributor

Choose a reason for hiding this comment

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

A global flag is hard to config and maintain for the user, why not using the kb manager config CM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does uses the manager config CM. This flag is the first level key in the config.

@github-actions github-actions bot added size/XL Denotes a PR that changes 500-999 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Sep 29, 2024
@@ -286,9 +286,11 @@ func main() {
if err := viper.ReadInConfig(); err != nil { // Handle errors reading the config file
setupLog.Info("unable to read in config, errors ignored")
}
intctrlutil.ReloadRegistryConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply these changes to the main function of dataprotection (it runs in a separate process).

defer registriesConfigMutex.Unlock()

registriesConfig = &RegistriesConfig{}
if err := viper.UnmarshalKey(constant.CfgRegistries, &registriesConfig); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it return an error if this key is not found?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Denotes a PR that changes 500-999 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants