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

Add registry prefix option #477

Merged
merged 1 commit into from
Jul 2, 2019

Conversation

saschagrunert
Copy link
Member

This allows to pre-pull the images and running benchmarks against a
local registry.

Beside this, the succinct output for benchmarks seems more reasonable
to me, so we put it on per default.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 26, 2019
@@ -297,6 +297,7 @@ func ListImage(c internalapi.ImageManagerService, filter *runtimeapi.ImageFilter

// PullPublicImage pulls the public image named imageName.
func PullPublicImage(c internalapi.ImageManagerService, imageName string, podConfig *runtimeapi.PodSandboxConfig) string {
imageName = TestContext.RegistryPrefix + imageName
Copy link
Member

Choose a reason for hiding this comment

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

the imageName may already contains the image registry, e.g. "gcr.io/cri-tools/test-image-1:latest". So adding another prefix may not work. I think we should parse the image first and replace the registry with TestContext.RegistryPrefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I changed the implementation to parse the image name before. Since the parsing expects a fully qualified image name, we now fallback to the intial imageName if the parsing fails. If not, we split up the domain and replace it.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 27, 2019
@saschagrunert
Copy link
Member Author

Please take a look again, I had to change the benchmark images to be fully qualified to make the registry prefix replacement work.

@feiskyer
Copy link
Member

Tests failed:

/home/travis/gopath/src/github.com/kubernetes-sigs/cri-tools/pkg/framework/framework.go:72
  SeccompProfilePath
  /home/travis/gopath/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/security_context_linux.go:483
    should support seccomp unconfined on the container [AfterEach]
    /home/travis/gopath/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/security_context_linux.go:529
    failed to pull image: rpc error: code = Unknown desc = invalid reference format
    Unexpected error:
        <*status.statusError | 0xc000401fb0>: {
            Code: 2,
            Message: "invalid reference format",
            Details: nil,
        }
        rpc error: code = Unknown desc = invalid reference format
    occurred

@saschagrunert saschagrunert force-pushed the registry-prefix branch 4 times, most recently from c43c4aa to caeccfb Compare June 28, 2019 06:50
@saschagrunert
Copy link
Member Author

I fixed the failing CI, in the end we would now need fully qualified images for every test. WDYT?

@feiskyer
Copy link
Member

I fixed the failing CI, in the end we would now need fully qualified images for every test. WDYT?

Could we default to docker.io if the registry is not included in the image?

@saschagrunert
Copy link
Member Author

Could we default to docker.io if the registry is not included in the image?

Yeah I think so, I will make docker.io/library the default prefix and if that not changes, then the tests should need no further adaption.

It's a bit tricky, sorry for all the different kind of approaches 😇

This allows to pre-pull the images and running benchmarks against a
local registry.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@feiskyer
Copy link
Member

feiskyer commented Jul 2, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, saschagrunert

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit 022d90a into kubernetes-sigs:master Jul 2, 2019
@saschagrunert saschagrunert deleted the registry-prefix branch July 2, 2019 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants