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.

Sign up for GitHub

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

Jump to bottom

Updates k8s ParseNode() to Handle Multiple IPs #25304

Merged
merged 1 commit into from May 24, 2023
Merged

Updates k8s ParseNode() to Handle Multiple IPs #25304

merged 1 commit into from May 24, 2023

Conversation

danehans
Copy link
Contributor

@danehans danehans commented May 8, 2023

This PR updates ParseNode() in the k8s pkg to emit a warning-level log message when a k8s node contains multiple IPs of the same address type and family.

Fixes: #20787
Supercedes: #20811

When a k8s node contains multiple addresses of the same type and family, Cilium will now emit a warning-level log message stating: "Detected multiple IPs of the same address type, Cilium will only consider the first IP in the Node resource"

@danehans danehans requested review from a team as code owners May 8, 2023 00:40
@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 May 8, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 8, 2023
@danehans
Copy link
Contributor Author

danehans commented May 8, 2023

cc: @christarazi since you reviewed #20811 and assigned me #20787.

@youngnick
Copy link
Contributor

/test

@youngnick youngnick added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 8, 2023
@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 May 8, 2023
asauber
asauber reviewed May 8, 2023
View reviewed changes
pkg/k8s/node.go Outdated Show resolved Hide resolved
@danehans danehans force-pushed the issue_20787_part1 branch 6 times, most recently from d3b9450 to 00cc57c Compare May 11, 2023 00:16
@danehans danehans requested a review from asauber May 11, 2023 00:19
asauber
asauber approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@asauber asauber 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 updates.

christarazi
christarazi reviewed May 16, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you squash the two commits into one? No need for two commits that are incomplete without each other. I realize that the first commit is from a different author so please use Co-authored-by: ... to properly attribute yourself and the original author in the squashed commit.

@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 16, 2023
@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 May 16, 2023
@christarazi
Copy link
Member

/test-runtime

@danehans
Copy link
Contributor Author

Commit 91bc601 was failing checkpatch b/c the co-authored-by was incorrectly formatted. checkpatch is now passing locally.

@danehans
Copy link
Contributor Author

@christarazi not sure why this PR is causing the Ingress/Gateway API conformance tests to fail. Could it be a test flake?

@danehans
Copy link
Contributor Author

@christarazi not sure if the Ingress/Gateway API conformance tests are required and why they would be failing due to this PR.

@christarazi
Copy link
Member

/test

@christarazi
Copy link
Member

Rebased to re-run tests. We had some CI outage over the weekend so better to start fresh. I created a tracking issue for the ConformanceIngress flake failure.

@danehans
Copy link
Contributor Author

@christarazi thanks for helping with the PR. It looks like the Ingress/Gateway API conformance tests are now passing. Let me know if anything else is needed.

@christarazi
Copy link
Member

/test

2 similar comments
@christarazi
Copy link
Member

/test

@christarazi
Copy link
Member

/test

@christarazi
Copy link
Member

/test-vagrant

1 similar comment
@christarazi
Copy link
Member

/test-vagrant

@danehans
Updates k8s ParseNode() to Handle Multiple IPs
ceb3d66 
Co-authored-by: Nikhil Sharma <nikhilsharma230303@gmail.com>

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

The runtime job is failing due to:

	 [18:08:09]     adverts_test.go:146: 
	 [18:08:09]         	Error Trace:	/home/vagrant/go/src/github.com/cilium/cilium/pkg/bgpv1/test/adverts_test.go:146
	 [18:08:09]         	Error:      	Received unexpected error:
	 [18:08:09]         	            	did not receive expected peering state ["ESTABLISHED"], context deadline exceeded
	 [18:08:09]         	Test:       	Test_PodCIDRAdvert
	 [18:08:09] time="2023-05-23T18:08:09Z" level=info msg="GoBGP test instance: stopping"
	 [18:08:09] time="2023-05-23T18:08:09Z" level=info msg="Delete a peer configuration" Key=172.16.100.1 Topic=Peer asn=65011 component=tests.BGP subsys=basic
	 [18:08:09] time="2023-05-23T18:08:09Z" level=debug msg="stop connect loop" Key=172.16.100.1 Topic=Peer asn=65011 component=tests.BGP subsys=basic
	 [18:08:09] time="2023-05-23T18:08:09Z" level=debug msg="freed fsm.h" Key=172.16.100.1 State=BGP_FSM_ACTIVE Topic=Peer asn=65011 component=tests.BGP subsys=basic
	 [18:08:09] level=info msg=Stopping subsys=hive
	 [18:08:09] time="2023-05-23T18:08:09Z" level=info msg="deleting dummy links"
	 [18:08:09] === RUN   Test
	 [18:08:09] level=warning msg="Failed to downgrade endpoint to original ENI datapath. Previous datapath is still intact and endpoint connectivity is not affected." error="failed to create new rule: unable to add new rule: fake error" rule="ip rule 111: from 172.16.1.0/24 to all table 11" subsys=linux-routing
	 [18:08:09] --- FAIL: Test_PodCIDRAdvert (15.14s)

Rebasing 7e150e3 since the bgpv1/test pkg is not part of this PR's commit history.

@danehans
Copy link
Contributor Author

The ^ failing test is passing locally:

$ go test
PASS
ok  	github.com/cilium/cilium/pkg/bgpv1/test	0.033s

@danehans danehans mentioned this pull request May 23, 2023
Merged
@christarazi
Copy link
Member

christarazi commented May 23, 2023 •
edited

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Edit: #25524

@christarazi
Copy link
Member

christarazi commented May 24, 2023 •
edited

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Edit: #23309

@christarazi
Copy link
Member

christarazi commented May 24, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unexpected missed tail calls

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/115/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@danehans
Copy link
Contributor Author

danehans commented May 24, 2023 •
edited

/mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next

@danehans
Copy link
Contributor Author

@christarazi it appears that #25304 (comment) did not create a GH issue. Does this command need to be executed by a maintainer?

@christarazi
Copy link
Member

christarazi commented May 24, 2023 •
edited by maintainer-s-little-helper bot

/mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next

👍 created #25658

@christarazi
Copy link
Member

Yes it does.

@christarazi
Copy link
Member

/test-1.26-net-next

@maintainer-s-little-helper 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 May 24, 2023
@christarazi christarazi merged commit 4535f13 into cilium:main May 24, 2023
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi left review comments

@asauber asauber asauber approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. kind/community-contribution This was a contribution made by a community member. kind/enhancement This would improve or streamline existing functionality. 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. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Better handle when K8s Nodes have more than one IP address per address type in their status field
5 participants
@danehans @youngnick @christarazi @asauber @tommyp1ckles