Adding an e2e test on Windows GMSA support #74738
Conversation
|
Hi @wk8. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
@yujuhong @dchen1107 @michmike @PatrickLang : guidance appreciated as to how to make sure that e2e test can run successfully as part of builds (see the |
test/e2e/windows/density.go
Outdated
There was a problem hiding this comment.
Note to reviewers: this has been moved to https://github.com/kubernetes/kubernetes/pull/74738/files#diff-b0ddb1c276f901c22122f6283cc7348c
|
/ok-to-test |
|
/assign dchen1107 |
|
/assign @adelina-t @BCLAU |
|
/sig windows |
|
/lgtm |
|
/assign @dchen1107 |
test/e2e/windows/framework.go
Outdated
There was a problem hiding this comment.
Not a fan of this being hardcoded here.
Also, the pause image here ( the one returned by GetPauseImageName() ):
kubernetes/test/utils/image/manifest.go
Line 128 in b67126e
By passing this env var:
kubernetes/test/utils/image/manifest.go
Line 66 in b67126e
with a file containing the registry names for windows images, the pause image should be fine.
In the upstream tests we use these registries: https://github.com/kubernetes-sigs/windows-testing/blob/master/images/image-repo-list
In fact, all the dockerfiles + documentation of the images we use in upstream e2e windows tests is here:
test/e2e/windows/gmsa.go
Outdated
There was a problem hiding this comment.
Same comment as above. No need to hardcore. We should just use one of the images in "k8s.io/kubernetes/test/utils/image", they are all based on windowsservercore , thus will have powershell. If not, we can just define another image in "k8s.io/kubernetes/test/utils/image", either way I suggest we stick to having all image names and repos there.
|
Thanks for the review @adelina-t :) addressed your comments. |
test/e2e/windows/README.md
Outdated
There was a problem hiding this comment.
Considering that kubernetes-sigs/windows-testing is the official repo, that would make it's image-repo-list file official and up to date. I would just have that file mentioned, no need for both.
test/e2e/windows/gmsa.go
Outdated
There was a problem hiding this comment.
Well, the command and args are unnecessary. It is a pause image after all, it is supposed to do that anyways.
There was a problem hiding this comment.
Good catch, thanks. Changed.
Sorry I forgot changing this as part of 9eac1bebd024ccf510ab126c08d7dfacd1e651d0 (was using image mcr.microsoft.com/powershell before).
|
/test pull-kubernetes-e2e-gce |
test/e2e/windows/README.md
Outdated
There was a problem hiding this comment.
Can this be moved to a separate commit from the main GMSA test commit? Ok to be part of the same PR.
test/e2e/windows/gmsa.go
Outdated
There was a problem hiding this comment.
This needs to be removed/fixed right?
There was a problem hiding this comment.
Yes - but before I remove it I'd like to see it failing where it's supposed to be running, to make sure this test actually runs somewhere... And I haven't seen that yet?
There was a problem hiding this comment.
Discussed with @wk8 offline that these tests won't run as PROW or get picked up by the other testgrid suites right now.
Signed-off-by: Jean Rouge <rougej+github@gmail.com>
This patch is adding an e2e on the Windows GMSA support added in kubernetes#73726 and kubernetes#74737. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
|
/lgtm |
|
/test pull-kubernetes-e2e-gce-100-performance |
|
/approve |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michmike, wk8 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 |
| @@ -0,0 +1,155 @@ | |||
| /* | |||
| Copyright 2018 The Kubernetes Authors. | |||
| f.PodClient().Create(pod) | ||
|
|
||
| ginkgo.By("waiting for the pod and its containers to be running") | ||
| gomega.Eventually(func() bool { |
There was a problem hiding this comment.
You can just use framework.WaitForPodReady()
There are other utilities functions like that. Is there any reason to roll your own?
There was a problem hiding this comment.
No reason other than I didn't spot these functions, apologies. @adelina-t kindly fixed that in #75116
What type of PR is this?
/kind feature
What this PR does / why we need it:
This patch is adding an e2e on the Windows GMSA support added in
#73726 and
#74737.
Which issue(s) this PR fixes:
@yujuhong requested an e2e test on this at #73726 (review).
Special notes for your reviewer:
This e2e test has been verified to pass on a cluster with a Windows node with the
WindowsGMSA=truefeature gate enabled (and the kubelet compiled from#74737); however, I don't believe that
feature g 10000 ate is enabled when running a "regular" build. Would it be possible to:
set the config it needs?
Does this PR introduce a user-facing change?: