-
Notifications
You must be signed in to change notification settings - Fork 366
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
[Windows] Update Windows instructions #2456
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2456 +/- ##
===========================================
- Coverage 59.79% 42.18% -17.62%
===========================================
Files 284 148 -136
Lines 22171 18222 -3949
===========================================
- Hits 13258 7687 -5571
- Misses 7490 9840 +2350
+ Partials 1423 695 -728
Flags with carried forward coverage won't be shown. Click here to find out more.
|
0c9737e
to
90cc54b
Compare
90cc54b
to
fe45cb4
Compare
/test-windows-networkpolicy |
1 similar comment
/test-windows-networkpolicy |
docs/windows.md
Outdated
@@ -130,7 +132,7 @@ kube-proxy version. | |||
|
|||
```bash | |||
# Example: | |||
curl -L https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/kube-proxy.yml | sed 's/VERSION/v1.18.0/g' > kube-proxy.yml | |||
curl -L https://github.com/kubernetes-sigs/sig-windows-tools/releases/download/v0.1.5/kube-proxy.yml | sed 's/VERSION/v1.18.0/g' > kube-proxy.yml |
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 it possible to use a varible for the version? You can use v0.1.5
as a default value if it is the version we wanted to pin.
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.
Updated.
hack/windows/Prepare-Node.ps1
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
.DESCRIPTION | |||
This script is only used for test to assist with joining a Windows node to a cluster. | |||
For production environment please follow the antrea windows installation guide and use kubernetes official script: https://github.com/kubernetes-sigs/sig-windows-tools/blob/master/kubeadm/scripts/PrepareNode.ps1 | |||
For production environment please follow the antrea windows installation guide and use kubernetes official script: https://github.com/kubernetes-sigs/sig-windows-tools/releases/download/v0.1.5/PrepareNode.ps1 |
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.
Ditto.
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 the script description so I think we can keep it as it is? I have used variables for others.
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.
maybe just point to the release page, without specifying a version, or we will probably forget to update it...
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.
Updated.
eabbe03
to
29f472a
Compare
/test-windows-networkpolicy |
29f472a
to
f0a1d0f
Compare
hack/windows/Prepare-Node.ps1
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
.DESCRIPTION | |||
This script is only used for test to assist with joining a Windows node to a cluster. | |||
For production environment please follow the antrea windows installation guide and use kubernetes official script: https://github.com/kubernetes-sigs/sig-windows-tools/blob/master/kubeadm/scripts/PrepareNode.ps1 | |||
For production environment please follow the antrea windows installation guide and use kubernetes official script: https://github.com/kubernetes-sigs/sig-windows-tools/releases/download/v0.1.5/PrepareNode.ps1 |
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.
maybe just point to the release page, without specifying a version, or we will probably forget to update it...
New-DirectoryIfNotExist "${AntreaHome}/logs" | ||
New-DirectoryIfNotExist "${KubeProxyLogPath}" | ||
nssm install kube-proxy "${KubernetesHome}/kube-proxy.exe" "--proxy-mode=userspace --kubeconfig=${KubeProxyKubeconfigPath} --log-dir=${KubeProxyLogPath} --logtostderr=false --alsologtostderr" | ||
nssm install antrea-agent "${AntreaHome}/bin/antrea-agent.exe" "--config=${AntreaHome}/etc/antrea-agent.conf --logtostderr=false --log_dir=${AntreaHome}/logs --alsologtostderr --log_file_max_size=100 --log_file_max_num=4" |
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.
I wonder if Antrea logs should go to c:/var/log/antrea
instead, as this would be a less "unexpected" location and it may be easier for users to find the logs there. But there is no need to address this in this PR.
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.
Good idea. I have created an issue to track this: #2472
f0a1d0f
to
410d767
Compare
/skip-all |
/test-windows-networkpolicy |
4 similar comments
/test-windows-networkpolicy |
/test-windows-networkpolicy |
/test-windows-networkpolicy |
/test-windows-networkpolicy |
762cf87
to
fbd60d1
Compare
/test-windows-networkpolicy |
fbd60d1
to
6fed7df
Compare
/test-windows-networkpolicy |
1 similar comment
/test-windows-networkpolicy |
/test-windows-networkpolicy |
33ed771
to
c2e7305
Compare
/test-windows-networkpolicy |
ci/jenkins/test.sh
Outdated
@@ -232,7 +232,7 @@ function deliver_antrea_windows { | |||
chmod -R g-w build/images/ovs | |||
chmod -R g-w build/images/base | |||
DOCKER_REGISTRY="${DOCKER_REGISTRY}" ./hack/build-antrea-ubuntu-all.sh --pull | |||
if [[ "$TESTCASE" =~ "networkpolicy" ]]; then | |||
if [[ "$TESTCASE" =~ "networkpolicy-no" ]]; then |
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.
What is networkpolicy-no for?
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 used to let Antrea Windows agent start as Daemonset for Windows NetworkPolicy test.
ci/jenkins/test.sh
Outdated
@@ -280,13 +282,15 @@ function deliver_antrea_windows { | |||
done | |||
|
|||
# Use a script to run antrea agent in windows Network Policy cases | |||
if [ "$TESTCASE" == "windows-networkpolicy" ]; then | |||
if [ "$TESTCASE" == "windows-networkpolicy-temp-disabled" ]; then |
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.
what do you mean about "windows-networkpolicy-temp-disabled"? what about "windows-networkpolicy-process"?
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.
Updated.
ci/jenkins/test.sh
Outdated
@@ -495,13 +499,13 @@ function run_conformance_windows { | |||
export PATH=$GOROOT/bin:$PATH | |||
export KUBECONFIG=$KUBECONFIG_PATH | |||
|
|||
if [[ "$TESTCASE" == "windows-conformance" ]]; then | |||
if [[ "$TESTCASE" =~ "windows-networkpolicy-temp-disabled" ]]; then |
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.
You use "not" in the "if" statement, but says user wanna process in the content, is it as your design?
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.
I think =~
means equal or include while !=
is no equal.
c2e7305
to
1d57a83
Compare
/skip-e2e /skip-conformance /skip-networkpolicy /test-windows-conformance /test-windows-networkpolicy |
1d57a83
to
5670a61
Compare
docs/windows.md
Outdated
@@ -250,7 +253,8 @@ container. | |||
|
|||
```powershell | |||
# Example: | |||
curl.exe -LO https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/PrepareNode.ps1 | |||
$SigWindowsToolsVersion="v0.1.5" |
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.
ditto
docs/windows.md
Outdated
@@ -130,7 +132,8 @@ kube-proxy version. | |||
|
|||
```bash | |||
# Example: | |||
curl -L https://github.com/kubernetes-sigs/sig-windows-tools/releases/latest/download/kube-proxy.yml | sed 's/VERSION/v1.18.0/g' > kube-proxy.yml | |||
$SigWindowsToolsVersion="v0.1.5" |
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 is a guide document, I think you could have one generic introduction about the example version of variable $SigWindowsToolsVersion
and $kubernetesVersion
we used in the examples, then we can use the variable directly anywhere. And we can explain sig-windows v0.1.5 is tested if you feel necessary.
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.
Used v0.1.5
for sig-windows-tool version. We think we can update kubernetes version in another PR after we verified a newer one than v1.18.0
.
5670a61
to
6da1c60
Compare
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.
LGTM
@antoninbas @tnqn PTAL. This PR wil unblock other PRs' Windows tests. |
/skip-e2e /skip-conformance /skip-networkpolicy /test-windows-conformance /test-windows-networkpolicy |
/test-windows-conformance |
/test-windows-networkpolicy |
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.
How does this PR achieve "Start Antrea Windows agent in Daemonset way for Windows NetworkPolicy test"? Is it because we set "TESTCASE" to "windows-networkpolicy" so it won't mach "windows-networkpolicy-process"? And do we know why "Currently the script way leads to many flaky failures"? why we used different modes for conformance and networkpolicy tests in the beginning?
docs/windows.md
Outdated
@@ -58,6 +58,8 @@ and daemons are pre-installed on the Windows Nodes in the demo. | |||
Antrea on Windows, Antrea provides a test-signed OVS package for you. | |||
See details in [Join Windows worker Nodes](#Join-Windows-worker-nodes) | |||
section. | |||
* Some manifests are from [sig-windows-tool](https://github.com/kubernetes-sigs/sig-windows-tools) repo. Release version |
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.
could you wrap it like other lines?
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.
Updated.
hack/windows/Prepare-Node.ps1
Outdated
@@ -4,7 +4,8 @@ | |||
|
|||
.DESCRIPTION | |||
This script is only used for test to assist with joining a Windows node to a cluster. | |||
For production environment please follow the antrea windows installation guide and use kubernetes official script: https://github.com/kubernetes-sigs/sig-windows-tools/blob/master/kubeadm/scripts/PrepareNode.ps1 | |||
For production environment please follow the antrea windows installation guide and use kubernetes official PrepareNode.ps1 |
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.
For production environment please follow the antrea windows installation guide and use kubernetes official PrepareNode.ps1 | |
For production environment please follow the antrea windows installation guide and use Kubernetes official PrepareNode.ps1 |
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.
Updated.
ci/jenkins/test.sh
Outdated
@@ -495,13 +499,13 @@ function run_conformance_windows { | |||
export PATH=$GOROOT/bin:$PATH | |||
export KUBECONFIG=$KUBECONFIG_PATH | |||
|
|||
if [[ "$TESTCASE" == "windows-conformance" ]]; then | |||
if [[ "$TESTCASE" =~ "windows-networkpolicy-process" ]]; then |
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.
I notice some code use "==" while some use "=~". We don't need regular expression, right?
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 it.
e.g.
Line 576 in 3bbb106
if [[ ${TESTCASE} =~ "windows" ]]; then |
We use
=~
here because windows testcases (windows-e2e, windows-conformance, windows-networpolicy) have windows
in variable ${TESTCASE}
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.
I mean even when it matchs "windows-networkpolicy-process", it sometimes uses "==" and sometimes uses "=~". I think the script should be accurate to express its intention, like what you explained here. We should use "==" when we want exact match.
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.
Used ==
for all "windows-networkpolicy-process"
.
6da1c60
to
3bbb106
Compare
Yes. |
/skip-e2e /skip-conformance /skip-networkpolicy |
* Mkdir kube-proxy log path before installation as a Service Otherwise kube-proxy Service may not work well. * Updated asset links from kubernetes-sigs/sig-windows-tools repo * Retry for Get-WebFileIfNotExist * Have cleaner environment for docker (stop, remove docker.pid, start) * Start Antrea Windows agent in Daemonset way for Windows NetworkPolicy test Currently the script way leads to many flaky failures * Skip SCTP test for Windows NetworkPolicy tests since it requires privilege mode which is not available on Windows Node * Force date sync after Windows VM is reverted to a snapshot Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>
3bbb106
to
bd24d29
Compare
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.
LGTM
/test-windows-conformance /test-windows-networkpolicy |
/test-windows-networkpolicy |
/skip-e2e /skip-conformance /skip-networkpolicy /test-windows-networkpolicy |
@tnqn Please help merge. |
Signed-off-by: Zhecheng Li lzhecheng@vmware.com