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.

Sign up for GitHub

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

Jump to bottom

[CI][Windows] Support Windows tests with proxyAll enabled #2899

Merged
merged 1 commit into from
Oct 22, 2021
Merged

[CI][Windows] Support Windows tests with proxyAll enabled #2899

merged 1 commit into from
Oct 22, 2021

Conversation

lzhecheng
Copy link
Contributor

Signed-off-by: Zhecheng Li [email protected]

tnqn
tnqn reviewed Oct 19, 2021
View reviewed changes
CONTRIBUTING.md Outdated
@@ -157,6 +157,9 @@ Here are the trigger phrases for individual checks:
* `/test-windows-e2e`: Windows IPv4 e2e tests
* `/test-windows-conformance`: Windows IPv4 conformance tests
* `/test-windows-networkpolicy`: Windows IPv4 networkpolicy tests
* `/test-windows-proxyall-e2e`: Windows IPv4 e2e tests with proxyAll enabled
* `/test-windows-proxyall-conformance`: Windows IPv4 conformance tests with proxyAll enabled
* `/test-windows-proxyall-networkpolicy`: Windows IPv4 networkpolicy tests with proxyAll enabled
Copy link
Member

Choose a reason for hiding this comment

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

I think e2e has a full set of test cases can cover Service test, it may be not necessary to test proxyall with conformance and networkpolicy. In the future, we may make the default windows e2e, conformance, networkpolicy run with proxyAll enabled directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@lzhecheng lzhecheng force-pushed the ci-win-proxyall branch 2 times, most recently from 8a041ad to 450ffc5 Compare October 19, 2021 06:10
@lzhecheng
Copy link
Contributor Author

/test-windows-proxyall-e2e

@lzhecheng lzhecheng added this to the Antrea v1.4 release milestone Oct 19, 2021
@lzhecheng lzhecheng requested a review from tnqn October 19, 2021 08:54
tnqn
tnqn reviewed Oct 19, 2021
View reviewed changes
ci/jenkins/test.sh Outdated
@@ -270,6 +278,15 @@ function deliver_antrea_windows {
export KUBECONFIG=$KUBECONFIG_PATH
export_govc_env_var

echo "====== If proxyAll is enabled, update yaml files ======"
Copy link
Member

Choose a reason for hiding this comment

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

Having a log saying "If proxyAll is enabled" doesn't help track how the script runs. I think it's more meaningful to move the log to the branch of the "if" below and just say "Updating yaml files to enabled proxyAll".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@tnqn
Copy link
Member

tnqn commented Oct 19, 2021

I didn't see the test succeed

@lzhecheng
Copy link
Contributor Author

I didn't see the test succeed

I checked the cluster and failures are 1) kube-proxy-windows cannot start and 2) agent-windows crashed. They result from 1) no kube-proxy configmap and daemonset; 2) bug in #2905. So in this PR I skip kube-proxy-windows if proxyAll enabled.

@lzhecheng
Copy link
Contributor Author

@tnqn I tested changes in this PR and NetNeighbor powershell fix in another PR #2906. It is successful.

tnqn
tnqn reviewed Oct 20, 2021
View reviewed changes
ci/jenkins/test.sh Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #2899 (e9bc1e8) into main (784dc4c) will decrease coverage by 20.59%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2899       +/-   ##
===========================================
- Coverage   61.44%   40.85%   -20.60%     
===========================================
  Files         283      157      -126     
  Lines       23587    19624     -3963     
===========================================
- Hits        14493     8017     -6476     
- Misses       7528    10842     +3314     
+ Partials     1566      765      -801
Flag Coverage Δ
kind-e2e-tests ?
unit-tests 40.85% <ø> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/ovs/openflow/default.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/util/runtime/runtime.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/agent/nodeportlocal/k8s/annotations.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/agent/cniserver/pod_configuration_linux.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ovs/openflow/logs.go 9.52% <0.00%> (-90.48%) ⬇️
pkg/apis/controlplane/register.go 0.00% <0.00%> (-90.00%) ⬇️
pkg/agent/agent_linux.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/agent/client.go 0.00% <0.00%> (-77.42%) ⬇️
pkg/ovs/ovsconfig/ovs_client_linux.go 0.00% <0.00%> (-76.93%) ⬇️
pkg/flowaggregator/certificate.go 0.00% <0.00%> (-76.58%) ⬇️
... and 231 more

tnqn
tnqn approved these changes Oct 21, 2021
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM
@lzhecheng can this be merged? or it needs to wait for the other two PRs merged first?

@lzhecheng
Copy link
Contributor Author

LGTM @lzhecheng can this be merged? or it needs to wait for the other two PRs merged first?

I suggest merging it now. I can debug with other PRs.

@lzhecheng
Copy link
Contributor Author

/skip-all

@tnqn tnqn merged commit 6ea909a into antrea-io:main Oct 22, 2021
@lzhecheng lzhecheng deleted the ci-win-proxyall branch October 22, 2021 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Antrea v1.4 release
Development

Successfully merging this pull request may close these issues.
3 participants
@lzhecheng @tnqn @codecov-commenter