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

Revert "loader : Log upsert and remove route errors" #15517

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Revert "loader : Log upsert and remove route errors" #15517

merged 1 commit into from
Mar 31, 2021

Conversation

nbusseneau
Copy link
Member

@nbusseneau nbusseneau commented Mar 30, 2021

This reverts commit 32284cd.

It seems like PR #15339 introduced a regression on Kernel 4.9 with the two following tests failing consistently (not flakes):

  • K8sDatapathConfig Encapsulation Check vxlan connectivity with per-endpoint routes
  • K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Fixes #15499

This reverts commit 32284cd.

It seems like PR #15339 introduced a regression on Kernel 4.9 with the
two following tests failing:

- K8sDatapathConfig Encapsulation Check vxlan connectivity with per-endpoint routes
- K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau requested a review from aanm March 30, 2021 22:22
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 30, 2021
@nbusseneau
Copy link
Member Author

Note: if we end up merging this, we should re-open #15282 and ping the author of #15339.

@nbusseneau
Copy link
Member Author

Pinging @nathanjsweet to let him know and avoid confusing him, since he merged the original PR ^^

@nbusseneau
Copy link
Member Author

I'm wondering if the issue might be due to the if/else blocks logic having changed.

Original:

if ep.RequireEndpointRoute() {
upsertEndpointRoute(ep, *ip.IPNet(128))
} else {
removeEndpointRoute(ep, *ip.IPNet(128))
}

Merged in #15339:

if ep.RequireEndpointRoute() {
if err := upsertEndpointRoute(ep, *ip.IPNet(128)); err != nil {
scopedLog.WithError(err).Warn("Failed to upsert route")
} else {
if err := removeEndpointRoute(ep, *ip.IPNet(128)); err != nil {
scopedLog.WithError(err).Warn("Failed to remove route")
}
}
}

If keeping the same blocks, it should have been instead:

if ep.RequireEndpointRoute() {
	if err := upsertEndpointRoute(ep, *ip.IPNet(128)); err != nil {
		scopedLog.WithError(err).Warn("Failed to upsert route")
	}
} else {
	if err := removeEndpointRoute(ep, *ip.IPNet(128)); err != nil {
		scopedLog.WithError(err).Warn("Failed to remove route")
	}
} 

@nbusseneau nbusseneau added the release-note/misc This PR makes changes that have no direct user impact. label Mar 30, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 30, 2021
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

pinging @h3llix @pchaigno. The code point out by @nbusseneau seems the reason why tests were failing.

@aanm
Copy link
Member

aanm commented Mar 31, 2021

it seems now that only are failing:
https://jenkins.cilium.io/job/Cilium-PR-K8s-1.15-kernel-4.9/137 is #15337
https://jenkins.cilium.io/job/Cilium-PR-K8s-1.19-kernel-4.19/5159 is #15455
https://jenkins.cilium.io/job/Cilium-PR-K8s-1.13-net-next/1236/ seems to be a new unrelated flake

The remaining CI is failing because this PR was not rebased after #15487 was merged

merging...

@aanm aanm merged commit 07f6048 into cilium:master Mar 31, 2021
1.10.0 automation moved this from In progress to Done Mar 31, 2021
@pchaigno
Copy link
Member

Oh, whoa! How did I miss that?

@h3llix Do you want to take care of resending a fixed pull request?

@h3llix
Copy link
Contributor

h3llix commented Mar 31, 2021

@pchaigno yes i ll do it . Do i have to rebase it ?

@pchaigno
Copy link
Member

Yep, rebase, fix the issue, and resend :-)

@nbusseneau nbusseneau deleted the pr/revert-15282 branch April 19, 2021 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

CI: K8sDatapathConfig Host firewall With VXLAN and endpoint routes
4 participants