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.

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

[IPsec] Apply a patch to upgrade Python2 code in ovs ovs-monitor-ipsec #1046

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

lzhecheng
Copy link
Contributor

@lzhecheng lzhecheng commented Aug 6, 2020

It is a workaround before upstream/ovs code update in ovs-monitor-ipsec.

#cat /var/log/antrea/openvswitch/ovs-monitor-ipsec.log
2020-08-06T10:38:36.455Z |  17 | ovs-monitor-ipsec | INFO | Tunnel stbed7-1-5b3e08 disappeared from OVSDB
2020-08-06T10:38:36.456Z |  18 | ovs-monitor-ipsec | INFO | Tunnel stbed7-2-b2a7f3 disappeared from OVSDB
2020-08-06T10:38:36.457Z |  19 | ovs-monitor-ipsec | INFO | Refreshing StrongSwan configuration
2020-08-06T10:38:36.573Z |  2  | daemon | INFO | pid 61 died, exit status 0, exiting
2020-08-06T10:38:53.25.Z |  0  | ovs-monitor-ipsec | INFO | Restarting StrongSwan
2020-08-06T10:38:55.201Z |  2  | reconnect | INFO | unix:/var/run/openvswitch/db.sock: connecting...
2020-08-06T10:38:55.202Z |  5  | reconnect | INFO | unix:/var/run/openvswitch/db.sock: connected
2020-08-06T10:38:55.284Z |  10 | ovs-monitor-ipsec | INFO | Tunnel stbed7-1-5b3e08 appeared in OVSDB
2020-08-06T10:38:55.285Z |  12 | ovs-monitor-ipsec | INFO | Tunnel stbed7-2-b2a7f3 appeared in OVSDB
2020-08-06T10:38:55.286Z |  14 | ovs-monitor-ipsec | INFO | Refreshing StrongSwan configuration

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-whole-conformance: to trigger all conformance tests on linux.
  • /skip-whole-conformance: to skip all conformance tests on linux.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-windows-networkpolicy: to trigger windows networkpolicy tests.
  • /skip-windows-networkpolicy: to skip windows networkpolicy tests.
  • /test-hw-offload: to trigger ovs hardware offload test.
  • /skip-hw-offload: to skip ovs hardware offload test.
  • /test-all: to trigger all tests (except whole conformance).
  • /skip-all: to skip all tests (except whole conformance).

These commands can only be run by members of the vmware-tanzu organization.

@lzhecheng lzhecheng changed the title Upgrade Python2 code in ovs-monitor-ipsec [IPsec]Upgrade Python2 code in ovs-monitor-ipsec Aug 6, 2020
@lzhecheng lzhecheng changed the title [IPsec]Upgrade Python2 code in ovs-monitor-ipsec [IPsec] Apply a patch to upgrade Python2 code in ovs ovs-monitor-ipsec Aug 6, 2020
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. But 2 things:

  • if you refer to a commit on your fork like this one, make sure you are never going to amend / remove that commit, or the reference will become invalid
  • I do not believe we should consider IPsec functionality is broken #1043 "fixed" until we make sure that the e2e tests for IPsec can detect such issues in the future

Edit: I also left a review comment for your OVS PR (openvswitch/ovs#331) after taking a look at the patch.

@lzhecheng
Copy link
Contributor Author

Thanks for the patch. But 2 things:

  • if you refer to a commit on your fork like this one, make sure you are never going to amend / remove that commit, or the reference will become invalid
  • I do not believe we should consider IPsec functionality is broken #1043 "fixed" until we make sure that the e2e tests for IPsec can detect such issues in the future

Edit: I also left a review comment for your OVS PR (openvswitch/ovs#331) after taking a look at the patch.

Make sense. I updated the commit link. The new one is on the branch which is not going to merge.
I removed "fixes" in this PR.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Please rebase master, as I made changes to this file when I updated Antrea to use Ubuntu 20.04 and OVS 2.13.1. In particular the if version_get "$OVS_VERSION" "2.13.0" statement is no longer needed and has been removed.

Comment on lines 71 to 72
# OVS ovs-monitor-ipsec is Python2 code but others have upgraded to Python3. Before upstream updates,
# here ovs-monitor-ipsec should be modified.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest this comment instead:
The OVS ovs-monitor-ipsec script has a Python3 shebang but still includes some Python2-specific code. Until the patch which fixes the script is merged upstream, we apply it here, or Antrea IPsec support will be broken.

@antoninbas antoninbas added this to the Antrea v0.9.0 release milestone Aug 12, 2020
It is a workaround before upstream/ovs code update in ovs-monitor-ipsec.
@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-whole-conformance: to trigger all conformance tests on linux.
  • /skip-whole-conformance: to skip all conformance tests on linux.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-windows-networkpolicy: to trigger windows networkpolicy tests.
  • /skip-windows-networkpolicy: to skip windows networkpolicy tests.
  • /test-hw-offload: to trigger ovs hardware offload test.
  • /skip-hw-offload: to skip ovs hardware offload test.
  • /test-all: to trigger all tests (except whole conformance).
  • /skip-all: to skip all tests (except whole conformance).

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM. Please ensure that the commit sha stays valid until this is merged upstream and we can pick up the patch from there. Also update #1043 to indicate that the issue with the ovs-monitor-ipsec daemon was fixed, but we still need to fix the e2e tests to catch issues with IPsec functionality.

@lzhecheng
Copy link
Contributor Author

/test-all

@lzhecheng lzhecheng merged commit 85a3e8b into antrea-io:master Aug 12, 2020
@lzhecheng lzhecheng deleted the ovs-ipsec-fix branch August 12, 2020 03:51
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020
It is a workaround before upstream/ovs code update in ovs-monitor-ipsec.
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 23, 2020
It is a workaround before upstream/ovs code update in ovs-monitor-ipsec.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants