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

Use imageutils instead of hardcoded image paths #81093

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

johnSchnake
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
A number of tests were using hardcoded image paths instead of
going through the imageutils package. The reason for centralizing
the logic there is to keep an eye on what images we use and where
they come from.

Which issue(s) this PR fixes:
Fixes #76617

Special notes for your reviewer:
Found one hardcoded variable here that was duplicating a registry that was already specified elsewhere. This would lead to 2 of the packages pulling from their own, hardcoded location instead of respecting the user overrides if provided.

In addition, there seemed to be one image being pulled from someone's ( @humblec ) private docker registry. This seems like an issue to me and should be remedied in its own PR; the point of this PR was just to centralize the image/registry lists.

@humblec maybe you can comment on why it is using that image instead of being integrated into the k8s registries?

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

/sig testing
/priority important-soon
/area e2e-test-framework

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/testing Categorizes an issue or PR as relevant to SIG Testing. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework labels Aug 7, 2019
@davidz627
Copy link
Contributor

/assign @wongma7
/cc @msau42

@@ -65,6 +67,7 @@ func initReg() RegistryList {
registry := RegistryList{
GcAuthenticatedRegistry: "gcr.io/authenticated-image-pulling",
DockerLibraryRegistry: "docker.io/library",
DockerHumbleC: "docker.io/humblec",
Copy link
Member

Choose a reason for hiding this comment

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

@humblec can you point us to a public registry where we can pull the gluster provisioner image from?

Copy link
Contributor

@humblec humblec Aug 8, 2019

Choose a reason for hiding this comment

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

@msau42 @johnSchnake while developing the e2e I was trying to use gcr or some other common repo where we host of our images, but somehow I didnt get the push access. Is there a common repo path where I can push this image ? If yes, please share it, so thatI can move or copy this container to that common path and update it via PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spiffxp Could you comment on the issue above?

I'm not sure really where the image comes from, but I would assume he needs to put any code to build the image into test/images then just ask someone in to bulid/push it?

Copy link
Member

Choose a reason for hiding this comment

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

/hold

we cannot be adding personal repos!

Copy link
Member

Choose a reason for hiding this comment

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

@humblec please work with @jsafrane and @childsb to push your image to the quay repo for kubernetes-incubator/external-storage projects

Copy link
Member

Choose a reason for hiding this comment

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

@humblec why not say https://hub.docker.com/u/gluster ? (in other words, why should it be in the k8s gcr repo?)

Copy link
Member

Choose a reason for hiding this comment

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

All the external-provisioners developed under kubernetes-incubator/external-storage are using quay.io/external-storage to store their images. I think that is the most obvious place to put the gluster image

Copy link
Contributor

Choose a reason for hiding this comment

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

@msau42 working with Jan and Brad on this , Hopefully this will be sorted out soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like @childsb is on pto. Jan dont have access of the repo. @wongma7 is it something you can help ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@msau42 @johnSchnake @dims Discussed with @childsb and as per him, it is better to host it under gluster org in docker hub. I have moved the image to gluster org and updated the code by #82115.

@msau42
Copy link
Member

msau42 commented Aug 7, 2019

/assign @humblec

@johnSchnake
Copy link
Contributor Author

/retest

@johnSchnake
Copy link
Contributor Author

I'm confused by the failure in the gce-csi run; it has failed multiple times in the same way so it seems like either the a 3rd party item isn't working, test-infra has an issue, or my change somehow caused the breakage.

However, it shows that tests pass and then the scripts fail to move logs around and then e2e-down times out.

W0807 20:40:25.792] Specify --start=40588 in the next get-serial-port-output invocation to get only the new output starting from here.
W0807 20:40:30.049] scp: /var/log/cluster-autoscaler.log*: No such file or directory
W0807 20:40:30.121] scp: /var/log/fluentd.log*: No such file or directory
W0807 20:40:30.121] scp: /var/log/kubelet.cov*: No such file or directory
W0807 20:40:30.121] scp: /var/log/startupscript.log*: No such file or directory
W0807 20:40:30.131] ERROR: (gcloud.compute.scp) [/usr/bin/scp] exited with return code [1].

If anyone has an idea/lead on this I'd appreciate it.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2019
@johnSchnake
Copy link
Contributor Author

I have another PR for something completely different with the same error (for gce-csi) so I think it is a test-infra issue of some sort. Going to do some searching in slack and such to see if this is a known issue.

@johnSchnake
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-csi-serial

@spiffxp
Copy link
Member

spiffxp commented Aug 21, 2019

/retest
needs rebase

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2019
@k8s-ci-robot k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 27, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 27, 2019
A number of tests were using hardcoded image paths instead of
going through the imageutils package. The reason for centralizing
the logic there is to keep an eye on what images we use and where
they come from.
@alejandrox1
Copy link
Contributor

alejandrox1 commented Sep 29, 2019

ping @dims @msau42 @johnSchnake
any updates on this?
/remove-area e2e-test-framework
/milestone v1.17

@k8s-ci-robot
Copy link
Contributor

@alejandrox1: Those labels are not set on the issue: area/e2e-testframework

In response to this:

ping @dims @msau42 @johnSchnake
any updates on this?
/remove-area e2e-testframework
/milestone v1.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 29, 2019
@k8s-ci-robot k8s-ci-robot removed the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Sep 29, 2019
@johnSchnake
Copy link
Contributor Author

Just needs approved/lgtm now. Let me know if other changes are needed.

@msau42
Copy link
Member

msau42 commented Oct 1, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnSchnake, msau42

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 Oct 1, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

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. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix: Use image util package to grab registry instead of static image path
10 participants