Add log when allocate nodecidr failure #13299

konghui · 2020-09-27T04:34:50Z

If user enconter an error when ipam allocate nodecidr failure where. It hang at log
required IPv4 pod CIDR not present in node resource
like this issue: #12956

I think it should return a error and show it to the user.

[Edit] See also related PR for IPAM: cilium/ipam#1

maintainer-s-little-helper · 2020-09-27T04:34:52Z

Commit 98871ec does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

qmonnet

Thanks a lot for the PR!

Please find some feedback inline below.
Please also add a Signed-off-by: tag for your commits as explained in the documentation linked by the bot.

pkg/ipam/allocator/podcidr/podcidr.go

aanm

@konghui thank you for the changes. What I don't understand is how you end up with that "Waiting for CRD" error message. Was it because you had an error on the IPAM that cilium-agent could not start and therefore register the CRDs?

maintainer-s-little-helper · 2020-10-03T13:57:04Z

Commit 98871ec does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

konghui · 2020-10-03T14:15:45Z

@aanm I am sorry. It should be error required IPv4 pod CIDR not present in node resource not Waiting for CRD. It's my fault. I was update the issue on the top.

maintainer-s-little-helper · 2020-10-03T14:30:19Z

Commit 98871ec does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

aanm · 2020-10-03T15:56:17Z

@konghui that makes more sense. Please squash all commits together and sign-off your commit as explained here #13299 (comment)

qmonnet

Some more nits on the error message, but looks good otherwise, thanks!

pkg/ipam/allocator/podcidr/podcidr.go

konghui · 2020-10-09T04:08:34Z

@konghui that makes more sense. Please squash all commits together and sign-off your commit as explained here #13299 (comment)

done

qmonnet

Looks good, thank you! Please make sure to address the CI checks too, but for my part I'm good with the current version.

qmonnet · 2020-10-12T08:22:30Z

pkg/ipam/allocator/podcidr/podcidr.go

+
+	for index, cidrAllocator := range cidrAllocators {
+		cidrAllocatorsInfo.WriteString(fmt.Sprintf("%s", cidrAllocator.String()))
+		if index < cidrAllocatorsLength - 1 {


Please format your code with gofmt -s to pass the style checks. In that case, I think it simply expects no space around the minus sign:

Suggested change

if index < cidrAllocatorsLength - 1 {

if index < cidrAllocatorsLength-1 {

aanm · 2020-10-15T13:24:59Z

@konghui can you take a look at the failures? Thanks!

qmonnet · 2020-10-16T08:01:52Z

test-me-please

Previously, the nil check was useless because the variable is never going to be nil, as it was initialized to an empty `nodeCIDRs` instance. This made the "Unable to allocate node CIDR" error impossible to surface. This commit fixes this by making the variable not a pointer, but rather a value, and reworking the further checks to ensure that the error in allocating a node CIDR can be surfaced. Fixes: ef6ecbd ("add error log When ipam allocate nodecidr failure") Fixes: cilium#13299 Signed-off-by: Chris Tarazi <[email protected]>

Previously, the nil check was useless because the variable is never going to be nil, as it was initialized to an empty `nodeCIDRs` instance. This made the "Unable to allocate node CIDR" error impossible to surface. This commit fixes this by making the variable not a pointer, but rather a value, and reworking the further checks to ensure that the error in allocating a node CIDR can be surfaced. Fixes: ef6ecbd ("add error log When ipam allocate nodecidr failure") Fixes: #13299 Signed-off-by: Chris Tarazi <[email protected]>

[ upstream commit 8c5c737 ] Previously, the nil check was useless because the variable is never going to be nil, as it was initialized to an empty `nodeCIDRs` instance. This made the "Unable to allocate node CIDR" error impossible to surface. This commit fixes this by making the variable not a pointer, but rather a value, and reworking the further checks to ensure that the error in allocating a node CIDR can be surfaced. Fixes: ef6ecbd ("add error log When ipam allocate nodecidr failure") Fixes: #13299 Signed-off-by: Chris Tarazi <[email protected]> Signed-off-by: Jarno Rajahalme <[email protected]>

[ upstream commit 8c5c737 ] Previously, the nil check was useless because the variable is never going to be nil, as it was initialized to an empty `nodeCIDRs` instance. This made the "Unable to allocate node CIDR" error impossible to surface. This commit fixes this by making the variable not a pointer, but rather a value, and reworking the further checks to ensure that the error in allocating a node CIDR can be surfaced. Fixes: ef6ecbd ("add error log When ipam allocate nodecidr failure") Fixes: #13299 Signed-off-by: Chris Tarazi <[email protected]> Signed-off-by: Tom Payne <[email protected]>

konghui requested a review from a team September 27, 2020 04:34

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 27, 2020

qmonnet requested changes Sep 28, 2020

View reviewed changes

pkg/ipam/allocator/podcidr/podcidr.go Outdated Show resolved Hide resolved

pkg/ipam/allocator/podcidr/podcidr.go Outdated Show resolved Hide resolved

qmonnet added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. and removed kind/bug This is a bug in the Cilium logic. labels Sep 28, 2020

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 28, 2020

qmonnet self-assigned this Sep 28, 2020

aanm requested changes Oct 1, 2020

View reviewed changes

konghui force-pushed the add_log_when_allocate_nodecidr_failure branch from 66c06b3 to 459a8e4 Compare October 3, 2020 13:57

konghui force-pushed the add_log_when_allocate_nodecidr_failure branch from 459a8e4 to 9dad234 Compare October 3, 2020 14:30

qmonnet requested changes Oct 5, 2020

View reviewed changes

pkg/ipam/allocator/podcidr/podcidr.go Outdated Show resolved Hide resolved

pkg/ipam/allocator/podcidr/podcidr.go Outdated Show resolved Hide resolved

konghui force-pushed the add_log_when_allocate_nodecidr_failure branch from 9dad234 to 9351248 Compare October 9, 2020 04:07

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 9, 2020

konghui force-pushed the add_log_when_allocate_nodecidr_failure branch from 9351248 to cad2d6d Compare October 10, 2020 10:06

qmonnet approved these changes Oct 12, 2020

View reviewed changes

konghui force-pushed the add_log_when_allocate_nodecidr_failure branch from cad2d6d to 0c25069 Compare October 16, 2020 05:53

konghui requested a review from aanm October 16, 2020 06:03

aanm added the needs-backport/1.9 label Oct 19, 2020

aanm approved these changes Oct 20, 2020

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 20, 2020

aanm merged commit ef6ecbd into cilium:master Oct 20, 2020

This was referenced Oct 21, 2020

v1.8 backport 2020-10-20 #13657

Merged

v1.9 backports 2020-10-21 #13688

Merged

pchaigno added backport-pending/1.8 and removed needs-backport/1.8 labels Oct 22, 2020

pchaigno added backport-done/1.8 and removed backport-pending/1.8 labels Oct 22, 2020

nathanjsweet added backport-pending/1.9 and removed needs-backport/1.9 labels Oct 22, 2020

christarazi mentioned this pull request Oct 28, 2020

Prepare for release v1.8.5 #13803

Merged

joestringer added backport-done/1.9 and removed backport-pending/1.9 labels Oct 31, 2020

joestringer mentioned this pull request Nov 3, 2020

Prepare for release v1.9.0-rc3 #13864

Merged

christarazi mentioned this pull request Nov 5, 2020

ipam/allocator: Fix nil check on node CIDR #13916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log when allocate nodecidr failure #13299

Add log when allocate nodecidr failure #13299

konghui commented Sep 27, 2020 •

edited by qmonnet

Loading

maintainer-s-little-helper bot commented Sep 27, 2020

qmonnet left a comment

aanm left a comment

maintainer-s-little-helper bot commented Oct 3, 2020

konghui commented Oct 3, 2020 •

edited

Loading

maintainer-s-little-helper bot commented Oct 3, 2020

aanm commented Oct 3, 2020 •

edited

Loading

qmonnet left a comment

konghui commented Oct 9, 2020

qmonnet left a comment

qmonnet Oct 12, 2020

aanm commented Oct 15, 2020

qmonnet commented Oct 16, 2020

	if index < cidrAllocatorsLength - 1 {
	if index < cidrAllocatorsLength-1 {

Add log when allocate nodecidr failure #13299

Add log when allocate nodecidr failure #13299

Conversation

konghui commented Sep 27, 2020 • edited by qmonnet Loading

maintainer-s-little-helper bot commented Sep 27, 2020

qmonnet left a comment

Choose a reason for hiding this comment

aanm left a comment

Choose a reason for hiding this comment

maintainer-s-little-helper bot commented Oct 3, 2020

konghui commented Oct 3, 2020 • edited Loading

maintainer-s-little-helper bot commented Oct 3, 2020

aanm commented Oct 3, 2020 • edited Loading

qmonnet left a comment

Choose a reason for hiding this comment

konghui commented Oct 9, 2020

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet Oct 12, 2020

Choose a reason for hiding this comment

aanm commented Oct 15, 2020

qmonnet commented Oct 16, 2020

konghui commented Sep 27, 2020 •

edited by qmonnet

Loading

konghui commented Oct 3, 2020 •

edited

Loading

aanm commented Oct 3, 2020 •

edited

Loading