-
Notifications
You must be signed in to change notification settings - Fork 181
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
[WIP] tkg test adaptation on tkg-windows #3040
base: master
Are you sure you want to change the base?
[WIP] tkg test adaptation on tkg-windows #3040
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rajguptavm The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @rajguptavm. Thanks for your PR. I'm waiting for a kubernetes-sigs 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-sigs/prow repository. |
Started GC block pre-checkin pipeline... Build Number: 52 |
|
Started GC block pre-checkin pipeline... Build Number: 53 |
|
481e064
to
6e23936
Compare
a4b3093
to
16c089e
Compare
16c089e
to
90dfbf8
Compare
@@ -212,7 +213,8 @@ var _ = ginkgo.Describe("Basic Static Provisioning", func() { | |||
framework.Logf("storageclass name :%s", storageclass.GetName()) | |||
|
|||
ginkgo.By("create resource quota") | |||
setStoragePolicyQuota(ctx, restConfig, storagePolicyName, namespace, rqLimit) | |||
// setStoragePolicyQuota(ctx, restConfig, storagePolicyName, namespace, rqLimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary log and commented lines
"echo 'Hello message from test into Pod1' >> /mnt/volume1/Pod1.html"} | ||
var wrtiecmd []string | ||
if windowsEnv { | ||
wrtiecmd = []string{"exec", pod.Name, "--namespace=" + namespace, "powershell.exe", "Add-Content /mnt/volume1/Pod1.html 'Hello message from test into Pod1'"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use filePathPod1 variable instead of /mnt/volume1/Pod1.html
@@ -435,7 +452,11 @@ var _ = ginkgo.Describe("[csi-guest] Volume Expansion Tests with reclaimation po | |||
_ = getPVCFromSupervisorCluster(svcPVCName) | |||
|
|||
scParameters := make(map[string]string) | |||
scParameters[scParamFsType] = ext4FSType | |||
if windowsEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already present in BeforeEach, no need here
@@ -122,8 +126,11 @@ var _ = ginkgo.Describe("[csi-guest] Volume Expansion Tests with reclaimation po | |||
pvDeleted = false | |||
|
|||
// Replace second element with pod.Name. | |||
cmd = []string{"exec", "", fmt.Sprintf("--namespace=%v", namespace), | |||
"--", "/bin/sh", "-c", "df -Tkm | grep /mnt/volume1"} | |||
if windowsEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass a podName here right? Otherwise we can remove above comment here
defer func() { | ||
// Delete Pod. | ||
ginkgo.By(fmt.Sprintf("Deleting the pod %s in namespace %s", pod.Name, namespace)) | ||
err := fpod.DeletePodWithWait(ctx, client, pod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also verify that pod gets detached from volume
_ = e2ekubectl.RunKubectlOrDie(namespace, "cp", testdataFile, | ||
fmt.Sprintf("%v/%v:/mnt/volume1/testdata", namespace, pod.Name)) | ||
if windowsEnv { | ||
cmdTestData := []string{"exec", pod.Name, "--namespace=" + namespace, "powershell.exe", "$out = New-Object byte[] 536870912; (New-Object Random).NextBytes($out); [System.IO.File]::WriteAllBytes('/mnt/volume1/testdata2.txt', $out)"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some comments here
var testdataFile string | ||
var op []byte | ||
var err error | ||
if !windowsEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can put an if else conditios instead of 2 if conditions
// execCommandOnGcWorker logs into gc worker node using ssh private key and executes command | ||
func execCommandOnGcWorker(sshClientConfig *ssh.ClientConfig, svcMasterIP string, gcWorkerIp string, | ||
svcNamespace string, cmd string) (fssh.Result, error) { | ||
result := fssh.Result{Host: gcWorkerIp, Cmd: cmd} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line required?
@@ -2275,7 +2285,9 @@ var _ = ginkgo.Describe("Volume health check", func() { | |||
replicas := *(statefulset.Spec.Replicas) | |||
// Waiting for pods status to be Ready. | |||
fss.WaitForStatusReadyReplicas(ctx, client, statefulset, replicas) | |||
gomega.Expect(fss.CheckMount(ctx, client, statefulset, mountPath)).NotTo(gomega.HaveOccurred()) | |||
if !windowsEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to check mountPath in case of windows environments?
@@ -3779,7 +3806,11 @@ func invokeTestForExpandVolumeMultipleTimes(f *framework.Framework, client clien | |||
defer cancel() | |||
ginkgo.By("Invoking Test to verify Multiple Volume Expansions on the same volume") | |||
scParameters := make(map[string]string) | |||
scParameters[scParamFsType] = ext4FSType | |||
if windowsEnv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can put this in BeforeEach
cmd := []string{"exec", pod.Name, "--namespace=" + namespace, "--", "/bin/sh", "-c", | ||
"cat /mnt/volume1/Pod1.html "} | ||
if windowsEnv { | ||
cmd = []string{"exec", pod.Name, "--namespace=" + namespace, "powershell.exe", "cat /mnt/volume1/Pod1.html"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use filePathPod1 instead /mnt/volume1/Pod1.html in all the places
svcClient, svNamespace := getSvcClientAndNamespace() | ||
setResourceQuota(svcClient, svNamespace, rqLimitScaleTest) | ||
} | ||
// if guestCluster { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these lines if not needed
_, err = e2eoutput.LookForStringInPodExec(namespace, newPods[0].Name, | ||
[]string{"/bin/cat", "/mnt/volume1/fstype"}, "", time.Minute) | ||
} | ||
// covered in this method createMultiplePods to check fss type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code
gomega.Expect(err).NotTo(gomega.HaveOccurred()) | ||
gomega.Expect(len(op)).To(gomega.BeZero()) | ||
if windowsEnv { | ||
cmdTestData := []string{"exec", pod.Name, "--namespace=" + namespace, "powershell.exe", "((Get-FileHash '/mnt/volume1/testdata2.txt' -Algorithm SHA256).Hash -eq (Get-FileHash '/mnt/volume1/testdata2_pod.txt' -Algorithm SHA256).Hash)"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this path is used in many places /mnt/volume1/testdata2.txt declare it in e2e_common.
PR needs rebase. 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-sigs/prow repository. |
What this PR does / why we need it:
testcase adaptation to support Windows container in Unified TKG
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #testcase adaptation to support Windows container in Unified TKG
Testing done:
In Progress