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

Upgrade Antrea base image to ubuntu:22.04 #4459

Merged

Conversation

antoninbas
Copy link
Contributor

@antoninbas antoninbas commented Dec 8, 2022

Ubuntu 20.04 comes with iptables 1.8.4, which seems to have several bugs
when used with nft. In particular, we observe that iptables-restore
sometimes segfaults when restoring the ANTREA-NODE-PORT-LOCAL chain in
the nat table. Ubuntu 22.04 comes with a more recent iptables version, 1.8.7.

As part of this change, we change the tag format for base images
(antrea/openvswitch and antrea/base-ubuntu). We no longer use the OVS
version as the tag, instead we use the Antrea minor version number.

Fixes #4435

Signed-off-by: Antonin Bas [email protected]

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #4459 (77d5acb) into main (2f2d4d2) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4459      +/-   ##
==========================================
+ Coverage   67.70%   67.90%   +0.19%     
==========================================
  Files         402      402              
  Lines       57253    57253              
==========================================
+ Hits        38764    38875     +111     
+ Misses      15818    15681     -137     
- Partials     2671     2697      +26     
Flag Coverage Δ
integration-tests 34.63% <ø> (-0.02%) ⬇️
kind-e2e-tests 47.92% <ø> (+0.86%) ⬆️
unit-tests 56.43% <ø> (-0.03%) ⬇️
Impacted Files Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go 70.27% <0.00%> (-6.76%) ⬇️
pkg/agent/ipassigner/responder/arp_responder.go 72.94% <0.00%> (-3.53%) ⬇️
pkg/agent/querier/querier.go 83.05% <0.00%> (-3.39%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 75.25% <0.00%> (-3.02%) ⬇️
pkg/agent/controller/networkpolicy/fqdn.go 73.78% <0.00%> (-2.34%) ⬇️
.../flowexporter/connections/conntrack_connections.go 81.42% <0.00%> (-1.91%) ⬇️
pkg/agent/util/iptables/iptables.go 45.07% <0.00%> (-1.56%) ⬇️
pkg/ipam/ipallocator/allocator.go 88.14% <0.00%> (-1.55%) ⬇️
pkg/ipam/poolallocator/allocator.go 73.57% <0.00%> (-1.43%) ⬇️
pkg/agent/client.go 76.74% <0.00%> (-1.17%) ⬇️
... and 31 more

@antoninbas antoninbas marked this pull request as ready for review December 9, 2022 21:01
@antoninbas
Copy link
Contributor Author

/test-all

jianjuns
jianjuns previously approved these changes Dec 10, 2022
tnqn
tnqn previously approved these changes Dec 12, 2022
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

@antoninbas
Copy link
Contributor Author

/test-e2e

1 similar comment
@antoninbas
Copy link
Contributor Author

/test-e2e

@antoninbas antoninbas dismissed stale reviews from tnqn and jianjuns via 027f7ac December 13, 2022 03:02
@antoninbas antoninbas force-pushed the upgrade-antrea-base-image-to-ubuntu-22.04 branch from acb94cf to 027f7ac Compare December 13, 2022 03:02
@antoninbas
Copy link
Contributor Author

@tnqn @luolanzone could you take another look?
I realized that if we wanted to keep using the same Ubuntu version for older Antrea versions, we would need to add the Ubuntu version to the tag for antrea/openvswitch and antrea/base-ubuntu. For example: antrea/openvswitch:20.04-ovs2.17.3 and antrea/base-ubuntu:20.04-ovs2.17.3. Instead of antrea/openvswitch:2.17.3 and antrea/base-ubuntu:2.17.3.

tnqn
tnqn previously approved these changes Dec 13, 2022
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

@tnqn
Copy link
Member

tnqn commented Dec 13, 2022

I realized that if we wanted to keep using the same Ubuntu version for older Antrea versions, we would need to add the Ubuntu version to the tag for antrea/openvswitch and antrea/base-ubuntu.

We could use a more specific tag but I assume we need to backport this to older Antrea versions? Otherwise runner's ubuntu version needs to be pinned and user would meet the issue when running Antrea on Jammy if they don't upgrade to 1.10.

@tnqn
Copy link
Member

tnqn commented Dec 13, 2022

/test-all

In previous test, ovs-monitor-ipsec failed to start:

[�[1;34m2022-12-12T21:15:34Z �[0;32mINFO �[0;36mantrea-ovs�[0m]: Started ovs-vswitchd
[�[1;34m2022-12-12T21:15:34Z �[0;32mINFO �[0;36mantrea-ovs�[0m]: Started the loop that checks OVS status every 30 seconds
[�[1;34m2022-12-12T21:17:03Z �[0;32mINFO �[0;36mantrea-ipsec�[0m]: Checking for IPsec prerequisites
[�[1;34m2022-12-12T21:17:03Z �[0;32mINFO �[0;36mantrea-ipsec�[0m]: Starting ovs-monitor-ipsec and strongswan agents
Traceback (most recent call last):
  File "/usr/share/openvswitch/scripts/ovs-monitor-ipsec", line 25, in <module>
    import ovs.daemon
ModuleNotFoundError: No module named 'ovs'

@tnqn
Copy link
Member

tnqn commented Dec 13, 2022

/test-all

luolanzone
luolanzone previously approved these changes Dec 13, 2022
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM overall, one nit. And you may need to update the PR's summary as well.

build/images/ovs/build.sh Outdated Show resolved Hide resolved
@antoninbas antoninbas dismissed stale reviews from luolanzone and tnqn via e452270 December 14, 2022 00:35
@antoninbas antoninbas force-pushed the upgrade-antrea-base-image-to-ubuntu-22.04 branch from 027f7ac to e452270 Compare December 14, 2022 00:35
@antoninbas
Copy link
Contributor Author

I realized that if we wanted to keep using the same Ubuntu version for older Antrea versions, we would need to add the Ubuntu version to the tag for antrea/openvswitch and antrea/base-ubuntu.

We could use a more specific tag but I assume we need to backport this to older Antrea versions? Otherwise runner's ubuntu version needs to be pinned and user would meet the issue when running Antrea on Jammy if they don't upgrade to 1.10.

By "more specific tag", I assume you mean something based on the current Antrea version?

I also consider using a hash (md5sum) of all dependency versions as the build tag (that's what we do for Windows), but decided against it in the end.

For the backporting, we have the choice. I think we should at least backport to 1.9, we can also do 1.8 and 1.7. The iptables issue that this is trying to fix is pretty specific: only when using NodePortLocal and only for a specific scenario (sequence of rules). What do you think?

@antoninbas
Copy link
Contributor Author

I have squashed commits

@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas force-pushed the upgrade-antrea-base-image-to-ubuntu-22.04 branch 4 times, most recently from dceeb06 to b1ac2d4 Compare December 14, 2022 22:58
@antoninbas
Copy link
Contributor Author

@tnqn I changed the tag format, it is now 22.04_v1.10_f26634ec4c.

I just remembered as I was working on this that we always build all base images from scratch when we generate an image for a patch release. I introduced this mechanism a long time ago to avoid such issues. In other words, we only push base images to the Docker registry when we build the "latest" Antrea image, and this is for caching purposes only.

I still decided to update the tag, as I don't think it makes sense to keep using the OVS version as the tag forever. In theory we could just use latest since it is for caching purposes only, but maybe it's not that great.

The reason for including the Antrea version is if we want to identify and delete old images from the registry.

If this is merged, to add Suricata please:

  1. add build/images/deps/suricata-version with the correct version number
  2. update build/images/chains/ubi.def and build/images/chains/ubuntu.def to indicate that Suricata is included in the image

@antoninbas antoninbas requested a review from tnqn December 14, 2022 23:01
@antoninbas
Copy link
Contributor Author

/test-all

@tnqn
Copy link
Member

tnqn commented Dec 15, 2022

/test-latest-all

1 similar comment
@tnqn
Copy link
Member

tnqn commented Dec 15, 2022

/test-latest-all

@tnqn
Copy link
Member

tnqn commented Dec 15, 2022

@tnqn I changed the tag format, it is now 22.04_v1.10_f26634ec4c.

I just remembered as I was working on this that we always build all base images from scratch when we generate an image for a patch release. I introduced this mechanism a long time ago to avoid such issues. In other words, we only push base images to the Docker registry when we build the "latest" Antrea image, and this is for caching purposes only.

I still decided to update the tag, as I don't think it makes sense to keep using the OVS version as the tag forever. In theory we could just use latest since it is for caching purposes only, but maybe it's not that great.

The reason for including the Antrea version is if we want to identify and delete old images from the registry.

If this is merged, to add Suricata please:

  1. add build/images/deps/suricata-version with the correct version number
  2. update build/images/chains/ubi.def and build/images/chains/ubuntu.def to indicate that Suricata is included in the image

@antoninbas thanks for the explanation. It also reminds me the original purpose of the base images. Then does it make sense to just use the major + minor parts of antrea VERSION as the tag? Then there will be a "latest" image for each release and we don't need to care about the changes of dependencies and the base images generated during the development cycle. I see it's also a little difficult to predict the image tag in various scripts, for instance, the tag used in build/images/test/Dockerfile. Using a stable tag should resolve it.

But to avoid confusion caused by the tag, "openvswitch:1.10", perhaps we could have some prefix to indicate it's not OVS version, like "openvswitch:antrea-1.10", "base-ubuntu:antrea-1.10"?

build/images/test/Dockerfile Outdated Show resolved Hide resolved
@antoninbas antoninbas force-pushed the upgrade-antrea-base-image-to-ubuntu-22.04 branch from b1ac2d4 to f7ab437 Compare December 15, 2022 18:20
@antoninbas
Copy link
Contributor Author

@tnqn the simpler approach works for me. I changed the tag to antrea-v1.10. It helps to have the same tag for both build chains (ubuntu and ubi).

@antoninbas
Copy link
Contributor Author

/test-latest-all

@antoninbas antoninbas requested a review from tnqn December 15, 2022 18:25
Ubuntu 20.04 comes with iptables 1.8.4, which seems to have several bugs
when used with nft. In particular, we observe that iptables-restore
sometimes segfaults when restoring the ANTREA-NODE-PORT-LOCAL chain in
the nat table. Ubuntu 22.04 comes with a more recent iptables version,
1.8.7.

As part of this change, we change the tag format for base images
(antrea/openvswitch and antrea/base-ubuntu). We no longer use the OVS
version as the tag, instead we use the Antrea minor version number.

Fixes antrea-io#4435

Signed-off-by: Antonin Bas <[email protected]>
@antoninbas antoninbas force-pushed the upgrade-antrea-base-image-to-ubuntu-22.04 branch from f7ab437 to 77d5acb Compare December 15, 2022 19:48
@antoninbas
Copy link
Contributor Author

/test-latest-all

@tnqn tnqn added this to the Antrea v1.10 release milestone Dec 16, 2022
@tnqn
Copy link
Member

tnqn commented Dec 16, 2022

/test-conformance

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, thanks

@tnqn tnqn merged commit c9c1b2c into antrea-io:main Dec 16, 2022
@xliuxu xliuxu added the action/backport Indicates a PR that requires backports. label Dec 16, 2022
@antoninbas antoninbas deleted the upgrade-antrea-base-image-to-ubuntu-22.04 branch December 16, 2022 04:57
gran-vmv added a commit to gran-vmv/antrea that referenced this pull request Dec 19, 2022
Fix issue caused by antrea-io#4459
Some images rely on ubuntu:20.04, keep the manual pulling step in e2e.

Signed-off-by: gran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flaky test] Kind TestNodePortLocal/testNPLChangePortRangeAgentRestart
5 participants