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

ipam/allocator/podcidr: fix old pod cidr logging error #17372

Merged
merged 1 commit into from Sep 29, 2021

Conversation

lrouter
Copy link
Contributor

@lrouter lrouter commented Sep 13, 2021

When releasing a pod cidr of a node, this pod cidr field is appended to
the log cache. But log cache could not be flushed because log is not in
debug mode. So old pod cidr is always in the log cache and shows up in
the log output even after a new pod cidr is allocated.
Fix this error by flushing the log cache.

@lrouter lrouter requested review from a team and twpayne September 13, 2021 02:56
@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 Sep 13, 2021
@twpayne twpayne added the release-note/misc This PR makes changes that have no direct user impact. label Sep 14, 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 Sep 14, 2021
@twpayne
Copy link
Contributor

twpayne commented Sep 14, 2021

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sFQDNTest Restart Cilium validate that FQDN is still working

Failure Output

FAIL: Testapp is not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

@lrouter
Copy link
Contributor Author

lrouter commented Sep 15, 2021

@twpayne My PR just replaces log.Debug with log.Info. it's weird that some unrelated test cases have failed. Please help !

Thanks.

@twpayne
Copy link
Contributor

twpayne commented Sep 15, 2021

We have a few flaky tests at the moment, the failures have nothing to do with your PR. Note also that the test-me-please comment is to trigger CI to run, not a message to you :)

@lrouter
Copy link
Contributor Author

lrouter commented Sep 15, 2021

We have a few flaky tests at the moment, the failures have nothing to do with your PR. Note also that the test-me-please comment is to trigger CI to run, not a message to you :)

👍 . Thanks for explanations.
/mlh new-flake Cilium-PR-K8s-GKE

@lrouter
Copy link
Contributor Author

lrouter commented Sep 22, 2021

@twpayne Hi, does there any update about this PR ? We have found some issues about cilium IPAM which could lead to pod cidr conflict. This is the first try to fix it.

@twpayne
Copy link
Contributor

twpayne commented Sep 27, 2021

Would you mind rebasing this PR on the latest master? We should have fixed some of the flaky tests.

When releasing a pod cidr of a node, this pod cidr field is appended to
the log cache. But log cache could not be flushed because log is not in
debug mode. So old pod cidr is always in the log cache and shows up in
the log output even after a new pod cidr is allocated.
Fix this error by flushing the log cache.

Signed-off-by: kaixi.fan <fankaixi.li@bytedance.com>
Signed-off-by: xiexiaohui.xxh <xiexiaohui.xxh@bytedance.com>
@lrouter lrouter requested a review from a team September 28, 2021 14:25
@lrouter lrouter requested a review from a team as a code owner September 28, 2021 14:25
@lrouter lrouter requested a review from a team September 28, 2021 14:25
@lrouter lrouter requested review from a team as code owners September 28, 2021 14:25
@lrouter lrouter requested a review from a team September 28, 2021 14:25
@lrouter lrouter requested review from a team as code owners September 28, 2021 14:25
@lrouter lrouter requested review from a team September 28, 2021 14:25
@lrouter lrouter requested a review from a team as a code owner September 28, 2021 14:25
@twpayne twpayne added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 28, 2021
@twpayne
Copy link
Contributor

twpayne commented Sep 28, 2021

OK, approved as it's a trivial change and marked ready-to-merge.

I'll still keep an eye on the tests as it's a good barometer PR for our CI :)

@lrouter
Copy link
Contributor Author

lrouter commented Sep 29, 2021

OK, approved as it's a trivial change and marked ready-to-merge.

I'll still keep an eye on the tests as it's a good barometer PR for our CI :)

Thanks. By the way, we are working to build a local cilium CI/CD system. I fount that it's hard to do this. I have to build cilium-runtime again and again, and have to push them to a private repo to be used by following building steps.

1 similar comment
@lrouter
Copy link
Contributor Author

lrouter commented Sep 29, 2021

OK, approved as it's a trivial change and marked ready-to-merge.

I'll still keep an eye on the tests as it's a good barometer PR for our CI :)

Thanks. By the way, we are working to build a local cilium CI/CD system. I fount that it's hard to do this. I have to build cilium-runtime again and again, and have to push them to a private repo to be used by following building steps.

@jibi jibi merged commit cebd290 into cilium:master Sep 29, 2021
@lrouter lrouter deleted the fix_master_podcidr_release_log branch September 29, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

10 participants