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

support attach metadata in PullImage CRI interface #1190

Conversation

diannaowa
Copy link
Contributor

Ⅰ. Describe what this PR does

Support attach metadata in PullImage CRI interface during ImagePulljobs;

apiVersion: apps.kruise.io/v1alpha1
kind: ImagePullJob
metadata:
  name: job-with-always
spec:
  image: nginx
  sandboxConfig:
    annotations:
      io.kubernetes.image.metrics.tags: "app=foo"
    labels:
      app: foo
  parallelism: 10

NodeImage:

apiVersion: apps.kruise.io/v1alpha1
kind: NodeImage
metadata:
  creationTimestamp: "2023-02-24T17:26:55Z"
  generation: 4
  labels:
    beta.kubernetes.io/arch: amd64
    beta.kubernetes.io/os: linux
    env: test-containerd
    kubernetes.io/arch: amd64
    kubernetes.io/hostname: vm-0-10-ubuntu
    kubernetes.io/os: linux
  name: vm-0-10-ubuntu
  resourceVersion: "23916206"
  uid: 8416e50e-7dd0-4cce-8d13-9425c3afd020
spec:
  images:
    nginx:
      sandboxConfig:
        annotations:
          io.kubernetes.image.metrics.tags: "app=foo"
        labels:
          app: foo
      tags:
      - createdAt: "2023-02-24T17:44:03Z"
        ownerReferences:
        - apiVersion: apps.kruise.io/v1alpha1
          kind: ImagePullJob
          name: job-with-always
          namespace: default
          uid: 64f5b96a-8596-4688-8e15-c1e7adddd209
        pullPolicy:
          backoffLimit: 3
          timeoutSeconds: 600
          ttlSecondsAfterFinished: 2246
        tag: latest
       ......

Ⅱ. Does this pull request fix one issue?

fixes #1155

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@kruise-bot kruise-bot added the size/M size/M: 30-99 label Feb 24, 2023
@diannaowa diannaowa force-pushed the support_attach_metadata_pullimage_CRI_1155 branch from 36c7ef4 to 47c7729 Compare February 24, 2023 18:06
@kruise-bot kruise-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Feb 24, 2023
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@furykerry
Copy link
Member

@diannaowa plz squash multiple commits into one

@diannaowa
Copy link
Contributor Author

@diannaowa plz squash multiple commits into one

Will do

@diannaowa diannaowa force-pushed the support_attach_metadata_pullimage_CRI_1155 branch from a2f97e4 to 75c6d5d Compare March 8, 2023 11:26
@kruise-bot kruise-bot removed the lgtm label Mar 8, 2023
@diannaowa diannaowa requested review from furykerry and removed request for zmberg March 8, 2023 11:27
@diannaowa
Copy link
Contributor Author

Squash multiple commits into one.
Please review it. @furykerry

@diannaowa
Copy link
Contributor Author

@zmberg

@zmberg
Copy link
Member

zmberg commented Mar 9, 2023

@diannaowa Current containerd.go's PullImage function can't meet this scenarios, and i think we can use cri.go in containerd. So i suggest as follows in factory.go :

switch cfg.runtimeType {
case ContainerRuntimeDocker:
	imageService, err = runtimeimage.NewDockerImageService(cfg.runtimeURI, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
case ContainerRuntimePouch:
	imageService, err = runtimeimage.NewPouchImageService(cfg.runtimeURI, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
case ContainerRuntimeContainerd, ContainerRuntimeCommonCRI:
	addr, _, err := kubeletutil.GetAddressAndDialer(cfg.runtimeRemoteURI)
	if err != nil {
		klog.Warningf("Failed to get address for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
	imageService, err = runtimeimage.NewCRIImageService(addr, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
}

@diannaowa
Copy link
Contributor Author

diannaowa commented Mar 9, 2023

@diannaowa Current containerd.go's PullImage function can't meet this scenarios, and i think we can use cri.go in containerd. So i suggest as follows in factory.go :

switch cfg.runtimeType {
case ContainerRuntimeDocker:
	imageService, err = runtimeimage.NewDockerImageService(cfg.runtimeURI, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
case ContainerRuntimePouch:
	imageService, err = runtimeimage.NewPouchImageService(cfg.runtimeURI, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
case ContainerRuntimeContainerd, ContainerRuntimeCommonCRI:
	addr, _, err := kubeletutil.GetAddressAndDialer(cfg.runtimeRemoteURI)
	if err != nil {
		klog.Warningf("Failed to get address for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
	imageService, err = runtimeimage.NewCRIImageService(addr, accountManager)
	if err != nil {
		klog.Warningf("Failed to new image service for %v (%s, %s): %v", cfg.runtimeType, cfg.runtimeURI, cfg.runtimeRemoteURI, err)
		continue
	}
}

Thanks for your review @zmberg .
That is a good idea. My thought was that after this feature was done, we would look at optimizing commonCRI and containerd, since much same behavior in during pullimge with common CRI and containerd.
If we need do this, I immediately follow this code to commit a new commit. And then the Progress of pull image will be not work when runtime is Containerd.

@zmberg
Copy link
Member

zmberg commented Mar 9, 2023

If we need do this, I immediately follow this code to commit a new commit. And then the Progress of pull image will be not work when runtime is Containerd.

Yes, you can do this.

@diannaowa
Copy link
Contributor Author

diannaowa commented Mar 9, 2023

If we need do this, I immediately follow this code to commit a new commit. And then the Progress of pull image will be not work when runtime is Containerd.

Yes, you can do this.

Done,Please review. @zmberg

@codecov-commenter
Copy link

Codecov Report

Merging #1190 (23ff863) into master (07c51b9) will increase coverage by 0.28%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   49.76%   50.04%   +0.28%     
==========================================
  Files         138      143       +5     
  Lines       19554    19912     +358     
==========================================
+ Hits         9731     9965     +234     
- Misses       8755     8851      +96     
- Partials     1068     1096      +28     
Flag Coverage Δ
unittests 50.04% <ø> (+0.28%) ⬆️

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

Impacted Files Coverage Δ
pkg/webhook/sidecarset/validating/utils.go 56.66% <0.00%> (-18.34%) ⬇️
...alidating/advancedcronjob_create_update_handler.go 23.57% <0.00%> (-1.06%) ⬇️
pkg/control/sidecarcontrol/util.go 53.48% <0.00%> (-0.39%) ⬇️
pkg/controller/sidecarset/sidecarset_processor.go 68.61% <0.00%> (-0.13%) ⬇️
...troller/sidecarterminator/kill_container_action.go 82.45% <0.00%> (ø)
...oller/sidecarterminator/virtual_kubelet_manager.go 75.00% <0.00%> (ø)
...troller/sidecarterminator/inplace_update_action.go 71.73% <0.00%> (ø)
...sidecarterminator/sidecar_terminator_controller.go 44.85% <0.00%> (ø)
...terminator/sidecar_terminator_pod_event_handler.go 91.04% <0.00%> (ø)
...set/validating/sidecarset_create_update_handler.go 58.92% <0.00%> (+0.62%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@diannaowa diannaowa force-pushed the support_attach_metadata_pullimage_CRI_1155 branch 2 times, most recently from bf73dbc to 41e1a8f Compare March 9, 2023 12:27
Signed-off-by: liuzhenwei <dui_zhang@163.com>

rerun e2e
@diannaowa diannaowa force-pushed the support_attach_metadata_pullimage_CRI_1155 branch from 41e1a8f to b69fa93 Compare March 9, 2023 13:09
@zmberg
Copy link
Member

zmberg commented Mar 10, 2023

/lgtm

@zmberg
Copy link
Member

zmberg commented Mar 13, 2023

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 54079d1 into openkruise:master Mar 13, 2023
ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024
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.

[feature request] Support attach metadata in PullImage CRI interface during ImagePulljobs
5 participants