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

clustermesh: fix CEP status patch #16986

Merged
merged 1 commit into from Jul 28, 2021

Conversation

nbusseneau
Copy link
Member

In 0681343 (from PR #15632), we changed CEP CRD schema and removed the status subresource.

This broke clustermesh logic as it was still trying to update CEP using the now removed status subresource. In particular, this resulted in a loss of connectivity in clustermeshes with external workloads: the VM could initially join the cluster but would immediately lose connectivity after failing to update the CEP resource (see #16984 for full context).

We change the clustermesh logic to adhere to the new CEP update CRD schema.

Fixes: 0681343

In 0681343 (from PR cilium#15632), we changed
CEP CRD schema and removed the `status` subresource.

This broke clustermesh logic as it was still trying to update CEP using
the now removed `status` subresource. In particular, this resulted in
a loss of connectivity in clustermeshes with external workloads: the VM
could initially join the cluster but would immediately lose connectivity
after failing to update the CEP resource (see cilium#16984 for full context).

We change the clustermesh logic to adhere to the new CEP update CRD
schema.

Fixes: 0681343

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau added kind/bug This is a bug in the Cilium logic. area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. labels Jul 23, 2021
@nbusseneau nbusseneau requested review from aanm and Weil0ng July 23, 2021 17:19
@nbusseneau nbusseneau requested a review from a team as a code owner July 23, 2021 17:19
@nbusseneau
Copy link
Member Author

This PR supersedes #16984 and is required for #16789.

@nbusseneau
Copy link
Member Author

Thanks for the debugging @Weil0ng :)

Copy link
Contributor

@Weil0ng Weil0ng 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 for the fix!

@nbusseneau
Copy link
Member Author

test-me-please

@nbusseneau
Copy link
Member Author

k8s-1.19-kernel-5.4 failed with #16479:

/home/jenkins/workspace/Cilium-PR-K8s-1.19-kernel-5.4/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:518
Unable to determine private iface
Expected
    <*errors.errorString | 0xc0017391c0>: {
        s: "Failed to retrieve iface by IP addr from: [{\"ifindex\":1,\"ifname\":\"lo\",\"flags\":[\"LOOPBACK\",\"UP\",\"LOWER_UP\"],\"mtu\":65536,\"qdisc\":\"noqueue\",\"operstate\":\"UNKNOWN\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"loopback\",\"address\":\"00:00:00:00:00:00\",\"broadcast\":\"00:00:00:00:00:00\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"127.0.0.1\",\"prefixlen\":8,\"scope\":\"host\",\"label\":\"lo\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"::1\",\"prefixlen\":128,\"scope\":\"host\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":2,\"ifname\":\"enp0s3\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"fq_codel\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"08:00:27:7a:25:75\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"10.0.2.15\",\"prefixlen\":24,\"broadcast\":\"10.0.2.255\",\"scope\":\"global\",\"dynamic\":true,\"label\":\"enp0s3\",\"valid_life_time\":84582,\"preferred_life_time\":84582},{\"family\":\"inet6\",\"local\":\"fe80::a00:27ff:fe7a:2575\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":3,\"ifname\":\"enp0s8\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"fq_codel\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"08:00:27:fb:8c:40\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"192.168.36.11\",\"prefixlen\":24,\"broadcast\":\"192.168.36.255\",\"scope\":\"global\",\"label\":\"enp0s8\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fd04::11\",\"prefixlen\":96,\"scope\":\"global\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fe80::a00:27ff:fefb:8c40\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":4,\"ifname\":\"enp0s9\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"fq_codel\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"08:00:27:bf:ae:9d\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"192.168.37.11\",\"prefixlen\":24,\"broadcast\":\"192.168.37.255\",\"scope\":\"global\",\"label\":\"enp0s9\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fd05::11\",\"prefixlen\":96,\"scope\":\"global\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fe80::a00:27ff:febf:ae9d\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":5,\"ifname\":\"enp0s10\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"fq_codel\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"08:00:27:1b:78:40\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"192.168.38.11\",\"prefixlen\":24,\"broadcast\":\"192.168.38.255\",\"scope\":\"global\",\"label\":\"enp0s10\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fe80::a00:27ff:fe1b:7840\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":6,\"ifname\":\"docker0\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"operstate\":\"UP\",\"group\":\"default\",\"link_type\":\"ether\",\"address\":\"02:42:79:bf:71:cb\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"172.17.0.1\",\"prefixlen\":16,\"broadcast\":\"172.17.255.255\",\"scope\":\"global\",\"label\":\"docker0\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fe80::42:79ff:febf:71cb\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":8,\"link_index\":7,\"ifname\":\"vetha585a2e\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"master\":\"docker0\",\"operstate\":\"UP\",\"group\":\"default\",\"link_type\":\"ether\",\"address\":\"72:31:b3:c8:27:1c\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"link_netnsid\":0,\"addr_info\":[{\"family\":\"inet6\",\"local\":\"fe80::7031:b3ff:fec8:271c\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":34,\"link\":\"cilium_host\",\"ifname\":\"cilium_net\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"NOARP\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"0a:df:a2:c7:4c:e3\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet6\",\"local\":\"fe80::8df:a2ff:fec7:4ce3\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":35,\"link\":\"cilium_net\",\"ifname\":\"cilium_host\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"NOARP\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"4a:d2:70:a6:6c:d5\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet\",\"local\":\"10.0.1.162\",\"prefixlen\":32,\"scope\":\"link\",\"label\":\"cilium_host\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fd04::11\",\"prefixlen\":128,\"scope\":\"global\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295},{\"family\":\"inet6\",\"local\":\"fe80::48d2:70ff:fea6:6cd5\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":104,\"ifname\":\"cilium_vxlan\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"operstate\":\"UNKNOWN\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"c2:c9:dd:55:68:b6\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"addr_info\":[{\"family\":\"inet6\",\"local\":\"fe80::c0c9:ddff:fe55:68b6\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]},{\"ifindex\":106,\"link_index\":105,\"ifname\":\"lxc_health\",\"flags\":[\"BROADCAST\",\"MULTICAST\",\"UP\",\"LOWER_UP\"],\"mtu\":1500,\"qdisc\":\"noqueue\",\"operstate\":\"UP\",\"group\":\"default\",\"txqlen\":1000,\"link_type\":\"ether\",\"address\":\"a2:58:56:78:bc:6c\",\"broadcast\":\"ff:ff:ff:ff:ff:ff\",\"link_netnsid\":1,\"addr_info\":[{\"family\":\"inet6\",\"local\":\"fe80::a058:56ff:fe78:bc6c\",\"prefixlen\":64,\"scope\":\"link\",\"valid_life_time\":4294967295,\"preferred_life_time\":4294967295}]}]\n",
    }
to be nil
/home/jenkins/workspace/Cilium-PR-K8s-1.19-kernel-5.4/src/github.com/cilium/cilium/test/k8sT/DatapathConfiguration.go:704

@nbusseneau
Copy link
Member Author

nbusseneau commented Jul 26, 2021

We're in a bit of an interesting situation due to the zero-flake strategy:

  • This PR is fixing a regression in master as a prerequisite for adding more CI coverage via workflows: add external workload conformance test #16789 (funnily enough, had we had this coverage earlier, it would have caught the regression).
  • It hit a flake that is 100% unrelated:
    • The regression fix PR is not per-se improving CI, so it's not an exception.
    • However the dependent PR is improving CI (as it would catch regressions such as the one the first PR is fixing), so if it had been the one hitting the flake it would be considered an exception.

😁

@nbusseneau
Copy link
Member Author

Fix for #16479 is (hopefully) in #16990. Considering the changes at hand and everything passed otherwise, I don't think it's worth to wait for it to be merged, rebase this PR, and then retrigger the whole CI.

Can I get an approval from @cilium/janitors and then mark as ready-to-merge?

@michi-covalent michi-covalent added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 27, 2021
@michi-covalent
Copy link
Contributor

done

@brb brb merged commit 3b6172b into cilium:master Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants