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

envoy 1.21.0 #90921

Closed
Closed

Conversation

XuehaiPan
Copy link
Contributor

Migrate stand-alone formula envoy to Python 3.10. Part of PR #90716.

@cclauss
Copy link
Contributor

cclauss commented Dec 10, 2021

These tests are being run on Python 3.5 which thinks that Python type hints are a syntax error.

@XuehaiPan XuehaiPan mentioned this pull request Dec 11, 2021
6 tasks
@branchvincent branchvincent added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Dec 16, 2021
@branchvincent
Copy link
Member

File "/tmp/envoy-20211216-11802-y4loia/.brew_home/_bazel/c8017d5c263d650d08e7b606fae0ff95/sandbox/processwrapper-sandbox/2817/execroot/envoy/external/com_googlesource_chromium_v8/wee8/third_party/jinja2/tests.py", line 13, in <module>
      from collections import Mapping
  ImportError: cannot import name 'Mapping' from 'collections' 

@XuehaiPan
Copy link
Contributor Author

XuehaiPan commented Dec 16, 2021

It is issued with the Google V8 Engine:

https://github.com/envoyproxy/envoy/blob/8618d52d9244f62f6931f766faee0d9061f2501d/bazel/repository_locations.bzl#L826-L840

$ wget -O - https://storage.googleapis.com/envoyproxy-wee8/wee8-9.6.180.12.tar.gz | tar -xz

$ grep -r 'from collections import Mapping' wee8
wee8/third_party/jinja2/tests.py:from collections import Mapping
wee8/third_party/jinja2/sandbox.py:from collections import Mapping
wee8/third_party/jinja2/runtime.py:    from collections import Mapping

collections.Mapping was deprecated since Python 3.3 and was removed in Python 3.10. It should change to from collections.abc import Mapping.

jinja fixed this in pallets/jinja@31bf9b7 three years ago. Seems the Google V8 Engine still uses a legacy version of dependencies.

@cclauss
Copy link
Contributor

cclauss commented Dec 16, 2021

This might be better as an Envoy issue. https://github.com/envoyproxy/envoy/issues

@iMichka iMichka removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Dec 16, 2021
@XuehaiPan
Copy link
Contributor Author

XuehaiPan commented Dec 17, 2021

Waiting for pull request envoyproxy/envoy#19244 to be merged and released.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jan 7, 2022
@XuehaiPan
Copy link
Contributor Author

Waiting for a new release of envoy.

@github-actions github-actions bot removed the stale No recent activity label Jan 8, 2022
@cclauss
Copy link
Contributor

cclauss commented Jan 13, 2022

https://github.com/envoyproxy/envoy/releases

@branchvincent branchvincent changed the title envoy: migrate to python@3.10 envoy 1.21.0 Jan 13, 2022
@branchvincent branchvincent added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jan 13, 2022
@chenrui333 chenrui333 added the CI-requeued PR has been re-added to the queue label Jan 13, 2022
@SMillerDev SMillerDev removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jan 13, 2022
@XuehaiPan
Copy link
Contributor Author

XuehaiPan commented Jan 13, 2022

In:
https://github.com/envoyproxy/envoy/blob/8cdfe788027127210beb08ad58bbc8754c71af2d/source/extensions/transport_sockets/tcp_stats/tcp_stats.cc#L12

#include </usr/include/linux/tcp.h>

It's a hardcoded header path.

I can build envoy on Ubuntu 20.04 successfully.


On Ubuntu 16.04 LTS:

struct tcp_info {
	__u8	tcpi_state;
	__u8	tcpi_ca_state;
	__u8	tcpi_retransmits;
	__u8	tcpi_probes;
	__u8	tcpi_backoff;
	__u8	tcpi_options;
	__u8	tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4;

	__u32	tcpi_rto;
	__u32	tcpi_ato;
	__u32	tcpi_snd_mss;
	__u32	tcpi_rcv_mss;

	__u32	tcpi_unacked;
	__u32	tcpi_sacked;
	__u32	tcpi_lost;
	__u32	tcpi_retrans;
	__u32	tcpi_fackets;

	/* Times. */
	__u32	tcpi_last_data_sent;
	__u32	tcpi_last_ack_sent;     /* Not remembered, sorry. */
	__u32	tcpi_last_data_recv;
	__u32	tcpi_last_ack_recv;

	/* Metrics. */
	__u32	tcpi_pmtu;
	__u32	tcpi_rcv_ssthresh;
	__u32	tcpi_rtt;
	__u32	tcpi_rttvar;
	__u32	tcpi_snd_ssthresh;
	__u32	tcpi_snd_cwnd;
	__u32	tcpi_advmss;
	__u32	tcpi_reordering;

	__u32	tcpi_rcv_rtt;
	__u32	tcpi_rcv_space;

	__u32	tcpi_total_retrans;

	__u64	tcpi_pacing_rate;
	__u64	tcpi_max_pacing_rate;
	__u64	tcpi_bytes_acked;    /* RFC4898 tcpEStatsAppHCThruOctetsAcked */
	__u64	tcpi_bytes_received; /* RFC4898 tcpEStatsAppHCThruOctetsReceived */
	__u32	tcpi_segs_out;	     /* RFC4898 tcpEStatsPerfSegsOut */
	__u32	tcpi_segs_in;	     /* RFC4898 tcpEStatsPerfSegsIn */
};

On Ubuntu 20.04 LTS:

struct tcp_info {
	__u8	tcpi_state;
	__u8	tcpi_ca_state;
	__u8	tcpi_retransmits;
	__u8	tcpi_probes;
	__u8	tcpi_backoff;
	__u8	tcpi_options;
	__u8	tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4;
	__u8	tcpi_delivery_rate_app_limited:1;

	__u32	tcpi_rto;
	__u32	tcpi_ato;
	__u32	tcpi_snd_mss;
	__u32	tcpi_rcv_mss;

	__u32	tcpi_unacked;
	__u32	tcpi_sacked;
	__u32	tcpi_lost;
	__u32	tcpi_retrans;
	__u32	tcpi_fackets;

	/* Times. */
	__u32	tcpi_last_data_sent;
	__u32	tcpi_last_ack_sent;     /* Not remembered, sorry. */
	__u32	tcpi_last_data_recv;
	__u32	tcpi_last_ack_recv;

	/* Metrics. */
	__u32	tcpi_pmtu;
	__u32	tcpi_rcv_ssthresh;
	__u32	tcpi_rtt;
	__u32	tcpi_rttvar;
	__u32	tcpi_snd_ssthresh;
	__u32	tcpi_snd_cwnd;
	__u32	tcpi_advmss;
	__u32	tcpi_reordering;

	__u32	tcpi_rcv_rtt;
	__u32	tcpi_rcv_space;

	__u32	tcpi_total_retrans;

	__u64	tcpi_pacing_rate;
	__u64	tcpi_max_pacing_rate;
	__u64	tcpi_bytes_acked;    /* RFC4898 tcpEStatsAppHCThruOctetsAcked */
	__u64	tcpi_bytes_received; /* RFC4898 tcpEStatsAppHCThruOctetsReceived */
	__u32	tcpi_segs_out;	     /* RFC4898 tcpEStatsPerfSegsOut */
	__u32	tcpi_segs_in;	     /* RFC4898 tcpEStatsPerfSegsIn */

	__u32	tcpi_notsent_bytes;
	__u32	tcpi_min_rtt;
	__u32	tcpi_data_segs_in;	/* RFC4898 tcpEStatsDataSegsIn */
	__u32	tcpi_data_segs_out;	/* RFC4898 tcpEStatsDataSegsOut */

	__u64   tcpi_delivery_rate;

	__u64	tcpi_busy_time;      /* Time (usec) busy sending data */
	__u64	tcpi_rwnd_limited;   /* Time (usec) limited by receive window */
	__u64	tcpi_sndbuf_limited; /* Time (usec) limited by send buffer */

	__u32	tcpi_delivered;
	__u32	tcpi_delivered_ce;

	__u64	tcpi_bytes_sent;     /* RFC4898 tcpEStatsPerfHCDataOctetsOut */
	__u64	tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
	__u32	tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
	__u32	tcpi_reord_seen;     /* reordering events seen */

	__u32	tcpi_rcv_ooopack;    /* Out-of-order packets received */

	__u32	tcpi_snd_wnd;	     /* peer's advertised receive window after
				      * scaling (bytes)
				      */
};

Diff:

8a9
> 	__u8	tcpi_delivery_rate_app_limited:1;
47a49,73
> 
> 	__u32	tcpi_notsent_bytes;
> 	__u32	tcpi_min_rtt;
> 	__u32	tcpi_data_segs_in;	/* RFC4898 tcpEStatsDataSegsIn */
> 	__u32	tcpi_data_segs_out;	/* RFC4898 tcpEStatsDataSegsOut */
> 
> 	__u64   tcpi_delivery_rate;
> 
> 	__u64	tcpi_busy_time;      /* Time (usec) busy sending data */
> 	__u64	tcpi_rwnd_limited;   /* Time (usec) limited by receive window */
> 	__u64	tcpi_sndbuf_limited; /* Time (usec) limited by send buffer */
> 
> 	__u32	tcpi_delivered;
> 	__u32	tcpi_delivered_ce;
> 
> 	__u64	tcpi_bytes_sent;     /* RFC4898 tcpEStatsPerfHCDataOctetsOut */
> 	__u64	tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
> 	__u32	tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
> 	__u32	tcpi_reord_seen;     /* reordering events seen */
> 
> 	__u32	tcpi_rcv_ooopack;    /* Out-of-order packets received */
> 
> 	__u32	tcpi_snd_wnd;	     /* peer's advertised receive window after
> 				      * scaling (bytes)
> 				      */

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Jan 16, 2022
@XuehaiPan XuehaiPan force-pushed the envoy-python3.10-migration branch 2 times, most recently from 3824cc3 to 5178fd7 Compare January 16, 2022 14:32
Formula/envoy.rb Outdated
Comment on lines 63 to 68
if OS.linux? && OS.kernel_version < "4.6"
# Extension `tcp_stats` requires Linux headers >= 4.6
# (It's included by absolute path `#include </usr/include/linux/tcp.h>`)
# Disable it when the kernel version requirements not satisfied
args << "--//source/extensions/transport_sockets/tcp_stats:enabled=false"
end

Copy link
Contributor Author

@XuehaiPan XuehaiPan Jan 17, 2022

Choose a reason for hiding this comment

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

The CI container failed to detect the correct kernel version (Linux Kernel for Ubuntu 16.04 is 4.4.0).

Should we always disable this extension on Linux? (#93273 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

My guess is yes. But CC @Homebrew/linux

Copy link
Member

Choose a reason for hiding this comment

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

Instead of looking at the kernel version, can we read the tcp_stats.cc file and make a decision based on that?

Copy link
Contributor Author

@XuehaiPan XuehaiPan Jan 17, 2022

Choose a reason for hiding this comment

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

Search tcpi_data_segs in https://github.com/envoyproxy/envoy/blob/release/v1.21/source/extensions/transport_sockets/tcp_stats/tcp_stats.cc#L12

See #90921 (comment) for full struct definition for struct tcp_info in /usr/include/linux/tcp.h.

$ docker run -it --rm -h ubuntu --pull always --entrypoint cat ghcr.io/homebrew/ubuntu16.04:latest /usr/include/linux/tcp.h > tcp-16.04.h
latest: Pulling from homebrew/ubuntu16.04
Digest: sha256:30574285516868f78fc3d0fa6fd2b6705a8a23ce799b8252e1a66bdbc20e3c82
Status: Image is up to date for ghcr.io/homebrew/ubuntu16.04:latest

$ docker run -it --rm -h ubuntu --pull always --entrypoint cat ghcr.io/homebrew/ubuntu20.04:latest /usr/include/linux/tcp.h > tcp-20.04.h
latest: Pulling from homebrew/ubuntu20.04
Digest: sha256:71779eeb4f5d6d6d7890de20c5fbecbcb705665921de4c31555819a333cf5a3e
Status: Image is up to date for ghcr.io/homebrew/ubuntu20.04:latest

$ diff tcp-16.04.h tcp-20.04.h
...
159a214
> 	__u8	tcpi_delivery_rate_app_limited:1;
198a254,305
> 
> 	__u32	tcpi_notsent_bytes;
> 	__u32	tcpi_min_rtt;
> 	__u32	tcpi_data_segs_in;	/* RFC4898 tcpEStatsDataSegsIn */
> 	__u32	tcpi_data_segs_out;	/* RFC4898 tcpEStatsDataSegsOut */
> 
> 	__u64   tcpi_delivery_rate;
> 
> 	__u64	tcpi_busy_time;      /* Time (usec) busy sending data */
> 	__u64	tcpi_rwnd_limited;   /* Time (usec) limited by receive window */
> 	__u64	tcpi_sndbuf_limited; /* Time (usec) limited by send buffer */
> 
> 	__u32	tcpi_delivered;
> 	__u32	tcpi_delivered_ce;
> 
> 	__u64	tcpi_bytes_sent;     /* RFC4898 tcpEStatsPerfHCDataOctetsOut */
> 	__u64	tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
> 	__u32	tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
> 	__u32	tcpi_reord_seen;     /* reordering events seen */
> 
> 	__u32	tcpi_rcv_ooopack;    /* Out-of-order packets received */
> 
> 	__u32	tcpi_snd_wnd;	     /* peer's advertised receive window after
> 				      * scaling (bytes)
> 				      */
> };
...

The old header doesn't have fields tcpi_data_segs_in and tcpi_data_segs_out.

@XuehaiPan
Copy link
Contributor Author

Need label CI-long-timeout.

@iMichka iMichka added the long build Set a long timeout for formula testing label Jan 17, 2022
@branchvincent branchvincent added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jan 19, 2022
@XuehaiPan
Copy link
Contributor Author

All tests passed.

@iMichka iMichka removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jan 19, 2022
@XuehaiPan
Copy link
Contributor Author

May need to remove the label automerge-skip.

@branchvincent branchvincent removed the automerge-skip `brew pr-automerge` will skip this pull request label Jan 20, 2022
@BrewTestBot
Copy link
Member

:shipit: @branchvincent has triggered a merge.

@XuehaiPan XuehaiPan deleted the envoy-python3.10-migration branch January 20, 2022 14:15
@branchvincent
Copy link
Member

@XuehaiPan thanks for sticking with this one!

@codefromthecrypt
Copy link
Contributor

Thanks, folks. This is great progress

codefromthecrypt pushed a commit to tetratelabs/func-e that referenced this pull request Jan 22, 2022
This update the last known Envoy version to 1.21 now that it exists in
Homebrew. Note: this is the first to work with arm64+darwin.

See Homebrew/homebrew-core#90921

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit to tetratelabs/func-e that referenced this pull request Jan 22, 2022
This update the last known Envoy version to 1.21 now that it exists in
Homebrew. Note: this is the first to work with arm64+darwin.

See Homebrew/homebrew-core#90921

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit to tetratelabs/func-e that referenced this pull request Jan 22, 2022
This update the last known Envoy version to 1.21 now that it exists in
Homebrew. Note: this is the first to work with arm64+darwin.

See Homebrew/homebrew-core#90921

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt mentioned this pull request Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-requeued PR has been re-added to the queue long build Set a long timeout for formula testing no ARM bottle Formula has no ARM bottle python-3.10-migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants