Skip to content

Adds test which pulls dockerhub Windows image#72777

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
claudiubelu:tests/hardcoded-docker-image
Feb 16, 2019
Merged

Adds test which pulls dockerhub Windows image#72777
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
claudiubelu:tests/hardcoded-docker-image

Conversation

@claudiubelu
Copy link
Copy Markdown
Contributor

@claudiubelu claudiubelu commented Jan 10, 2019 •
edited
Loading

What type of PR is this?

/kind failing-test
/sig testing
/sig windows

What this PR does / why we need it:

Updates the test "should be able to pull image from docker hub" to pull
the golang image instead of alpine, which can also be used on Windows,
allowing the test to pass on both OSes.

Which issue(s) this PR fixes:

Related: #69871

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 10, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jan 10, 2019

Hi @BCLAU. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2019
@dims
Copy link
Copy Markdown
Member

dims commented Jan 10, 2019

@BCLAU problem is that it will not match the description "should be able to pull image from docker hub"

@claudiubelu
Copy link
Copy Markdown
Contributor Author

claudiubelu commented Jan 10, 2019

@dims Well, the busybox image is pulled from the "dockerLibraryRegistry"[1], so it would still be on docker hub.

The image used should be configurable, somehow, since it is OS dependent. Not only that, but for Windows Server 2019, we cannot run containers with images based on Windows Server 1803 and vice-versa, so I couldn't simply hardcode an image name like "microsoft/windowservercore:1803" in a test.

[1] https://github.com/kubernetes/kubernetes/blob/master/test/utils/image/manifest.go#L99

@PatrickLang
Copy link
Copy Markdown
Contributor

PatrickLang commented Jan 23, 2019 •
edited
Loading

@dims since upstream busybox doesn't publish Windows images, we could pick another image.

https://hub.docker.com/_/golang seems to have extremely broad support

On Windows 10 (with Docker for Windows - Linux mode)

docker version
Client: Docker Engine - Community
 Version:           18.09.0
 API version:       1.39
 Go version:        go1.10.4
 Git commit:        4d60db4
 Built:             Wed Nov  7 00:47:51 2018
 OS/Arch:           windows/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          18.09.0
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.10.4
  Git commit:       4d60db4
  Built:            Wed Nov  7 00:55:00 2018
  OS/Arch:          linux/amd64
  Experimental:     true

C:\Users\plang\Source\enhancements\keps\sig-windows>docker pull golang
Using default tag: latest
latest: Pulling from library/golang
cd8eada9c7bb: Pull complete
c2677faec825: Pull complete
fcce419a96b1: Pull complete
045b51e26e75: Pull complete
70a7003e9fe9: Pull complete
751b6f060321: Pull complete
462a907acb44: Pull complete
Digest: sha256:0c7ba0b5eff462068cb8c16d94248ed44463c30418fac91ee46283e34f483547
Status: Downloaded newer image for golang:latest

On Windows Server 2019

docker pull golang
Using default tag: latest
latest: Pulling from library/golang
65014b3c3121: Already exists
2ac060f1ef06: Already exists
409ca3908f9b: Pull complete
05c8e42e8c40: Pull complete
f39532532c4d: Pull complete
0faa97f71daa: Pull complete
5a99d3cca2b4: Pull complete
75bc32531793: Pull complete
6e7d996a4bf1: Pull complete
3e248d960d69: Pull complete
87723d90b848: Pull complete
0851883a4fe9: Pull complete
fcf9ba83111c: Pull complete
Digest: sha256:0c7ba0b5eff462068cb8c16d94248ed44463c30418fac91ee46283e34f483547
Status: Downloaded newer image for golang:latest

C:\Users\vagrant>docker version
Client:
 Version:           18.09.1
 API version:       1.39
 Go version:        go1.10.6
 Git commit:        20b67756d0
 Built:             unknown-buildtime
 OS/Arch:           windows/amd64
 Experimental:      false

Server:
 Engine:
  Version:          18.09.1
  API version:      1.39 (minimum version 1.24)
  Go version:       go1.10.6
  Git commit:       20b67756d0
  Built:            01/09/2019 17:09:57
  OS/Arch:          windows/amd64
  Experimental:     false

@PatrickLang
Copy link
Copy Markdown
Contributor

PatrickLang commented Jan 23, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Jan 23, 2019
@claudiubelu claudiubelu force-pushed the tests/hardcoded-docker-image branch from d0c1faa to a0d83bf Compare January 24, 2019 14:28
@claudiubelu
Copy link
Copy Markdown
Contributor Author

claudiubelu commented Jan 24, 2019

@PatrickLang nice find! I've updated the PR to pull the golang image instead. The test passes on WS 2019 nodes. It will require the --node-os-distro flag to be passed though, since we can't run the same command on Windows as on Linux (it was possible with the busybox image though).

@PatrickLang
Copy link
Copy Markdown
Contributor

PatrickLang commented Jan 24, 2019

@BCLAU wouldn't ping -t localhost be ok on Linux too?

@dims
Copy link
Copy Markdown
Member

dims commented Jan 24, 2019

@PatrickLang you mean ping -t 1 localhost? (-t is the TTL)

@PatrickLang
Copy link
Copy Markdown
Contributor

PatrickLang commented Jan 24, 2019

@dims Hmm, no. -t on windows makes it keep pinging until it gets a ctrl-c. Looks like that won't work on both platforms

@claudiubelu
Copy link
Copy Markdown
Contributor Author

claudiubelu commented Jan 25, 2019

Indeed. I couldn't find a proper command which does the same thing and be valid on both OSes.

@claudiubelu claudiubelu changed the title Pulls dockerhub busybox image from configured repo Pulls dockerhub golang image instead of alpine Jan 25, 2019
@benmoss
Copy link
Copy Markdown
Member

benmoss commented Jan 25, 2019

curl --retry 20 --retry-connrefused --retry-delay 900 localhost:9999 looks like it should suffice if all you need is a Linux & Windows cross-platform "runs (more or less) forever" command

@PatrickLang
Copy link
Copy Markdown
Contributor

PatrickLang commented Jan 26, 2019

ah yes good idea @benmoss

One caveat - be sure not to do this if you're using a Windows image with the shell set to PowerShell. It has an alias from curl to Invoke-WebRequest which has different syntax. You can get around that by doing curl.exe but that would break Linux.

@michmike
Copy link
Copy Markdown
Contributor

michmike commented Jan 28, 2019

is there anyone that still has objections on this PR and the implementation of ping or the choice of the image?
adding a hold, but also approving it in the meantime
/hold

@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 Jan 28, 2019
@michmike
Copy link
Copy Markdown
Contributor

michmike commented Jan 28, 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 Jan 28, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 12, 2019
@claudiubelu claudiubelu changed the title Pulls dockerhub golang image instead of alpine Adds test which pulls dockerhub Windows image Feb 12, 2019
@claudiubelu
Copy link
Copy Markdown
Contributor Author

claudiubelu commented Feb 12, 2019

So, if we are to add a test with our own image which has manifest lists, it would look something like this. Ofc, the repo name will be changed to "e2eteam" after we've verified that they're good and after they've been pushed to the "e2eteam" repo.

neolit123
neolit123 reviewed Feb 12, 2019
View reviewed changes
test/e2e/common/runtime.go Outdated
Copy link
Copy Markdown
Member

@neolit123 neolit123 Feb 12, 2019

Choose a reason for hiding this comment

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

so i guess we need to hold this PR until:

  • we can have this image in a official account somewhere?
  • add an [WindowsOnly] alternative to the test case "should be able to pull image from gcr.io [LinuxOnly]","

is this correct?

Copy link
Copy Markdown
Contributor Author

@claudiubelu claudiubelu Feb 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the official account, the e2eteam repo is what we've created to be used by CIs ( https://testgrid.k8s.io/sig-windows#Conformance%20acs-engine%20on%20Azure ) and they are actively maintained and kept up-to-date, so I think that's a good enough.

Regarding the 2nd point, We could push the busybox image to gcr.io as well, I can look into it, but that could also be done in a separate PR.

Updated the repo name to e2eteam.

Copy link
Copy Markdown
Member

@neolit123 neolit123 Feb 14, 2019

Choose a reason for hiding this comment

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

that could also be done in a separate PR.

ok, agreed.

could you please add TODO about the GCR test for Windows?

Copy link
Copy Markdown
Contributor Author

@claudiubelu claudiubelu Feb 15, 2019

Choose a reason for hiding this comment

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

Done.

@claudiubelu claudiubelu force-pushed the tests/hardcoded-docker-image branch from 4413b87 to 40bfa62 Compare February 14, 2019 16:35
@michmike
Copy link
Copy Markdown
Contributor

michmike commented Feb 14, 2019

it looks like we are close to finalizing this. great job @BCLAU . i will approve this with a hold so that you can remove the hold when ready to merge. please make sure to create the second PR for pushing the image to gcr.io.

/hold

@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 Feb 14, 2019
@michmike
Copy link
Copy Markdown
Contributor

michmike commented Feb 14, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019
@neolit123
Copy link
Copy Markdown
Member

neolit123 commented Feb 14, 2019

/assign @timothysc @vishh
we can get an approve here please.

this also needs a follow up PR to support GCR too.

@claudiubelu claudiubelu force-pushed the tests/hardcoded-docker-image branch from 40bfa62 to 609b5c3 Compare February 15, 2019 11:28
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2019
@claudiubelu
Copy link
Copy Markdown
Contributor Author

claudiubelu commented Feb 15, 2019

So, it seems that storing and pulling images from gcr.io is not quite free [0], so I didn't upload any image. I am assuming that someone is sponsoring some gcr.io repos, since there are 2 repos used in E2E tests (gcr.io/kubernetes-e2e-test-images, gcr.io/k8s-authenticated-test). Any idea how to upload a manifest list there, or who could?

[0] https://cloud.google.com/storage/pricing

@neolit123
Copy link
Copy Markdown
Member

neolit123 commented Feb 15, 2019

/lgtm
on the latest change.

I didn't upload any image. I am assuming that someone is sponsoring some gcr.io repos

@BCLAU gcr.io hosting is sponsored by google, at the moment. we need to coordinate the push to the registry with maintainers on slack.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2019
@neolit123
Copy link
Copy Markdown
Member

neolit123 commented Feb 15, 2019

/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 Feb 15, 2019
@claudiubelu claudiubelu mentioned this pull request Feb 15, 2019
Closed
timothysc
timothysc approved these changes Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, michmike, timothysc

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

The pull request process is described here

Details 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 Feb 15, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 15, 2019 •
edited
Loading

@BCLAU: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws e6ce56366628a56d8aeaf2bd3f0ec881f8c1e102 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@michmike
Copy link
Copy Markdown
Contributor

michmike commented Feb 15, 2019

/test pull-kubernetes-verify

@k8s-ci-robot k8s-ci-robot merged commit d5b890e into kubernetes:master Feb 16, 2019
@BenTheElder BenTheElder mentioned this pull request Feb 28, 2019
Closed
@claudiubelu claudiubelu deleted the tests/hardcoded-docker-image branch April 19, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenTheElder BenTheElder BenTheElder left review comments

@MaciekPytel MaciekPytel Awaiting requested review from MaciekPytel

@mikedanese mikedanese Awaiting requested review from mikedanese

+3 more reviewers

@neolit123 neolit123 neolit123 approved these changes

@timothysc timothysc timothysc approved these changes

@michmike michmike michmike approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@timothysc timothysc

@neolit123 neolit123

@michmike michmike

@vishh vishh

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. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v1.14

Development

Successfully merging this pull request may close these issues.