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

ipam: fix crd mode #16493

Merged
merged 1 commit into from Jun 10, 2021
Merged

ipam: fix crd mode #16493

merged 1 commit into from Jun 10, 2021

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Jun 10, 2021

Fix regression affecting IPAM CRD mode introduced in #15160.

In buildAllocationResult an "ENI" error was wrongly constructed
in fallthrough case affecting all modes not specifically handled
within the switch statement.

Fixes: #16492
Signed-off-by: Jussi Maki jussi@isovalent.com

@joamaki
ipam: fix crd mode
75d4e37 
Fix regression affecting IPAM CRD mode introduced in cilium#15160.

In buildAllocationResult an "ENI" error was wrongly constructed
in fallthrough case affecting all modes not specifically handled
within the switch statement.

Fixes: cilium#16492
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki added kind/bug This is a bug in the Cilium logic. sig/ipam IP address management, including cloud IPAM needs-backport/1.10 labels Jun 10, 2021
@joamaki joamaki requested review from aanm, qmonnet and a team June 10, 2021 13:17
@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 Jun 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot assigned aanm and qmonnet and unassigned qmonnet Jun 10, 2021
@pchaigno pchaigno added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 10, 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 Jun 10, 2021
@joamaki joamaki mentioned this pull request Jun 10, 2021
Closed
pchaigno
pchaigno approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

How did we miss this in CI??

@pchaigno
Copy link
Member

test-me-please

@joamaki
Copy link
Contributor Author

joamaki commented Jun 10, 2021

How did we miss this in CI??

By the virtue of not having tests for this particular IPAM mode. That really should be addressed.

@joamaki
Copy link
Contributor Author

joamaki commented Jun 10, 2021 •
edited

Tested manually in minikube with:

cilium install --ipam crd --agent-image quay.io/cilium/cilium-ci:75d4e37b370f9ee80a786ef6e23b4237279dd0d5 --operator-image quay.io/cilium/operator-generic-ci:75d4e37b370f9ee80a786ef6e23b4237279dd0d5 --version latest

apiVersion: "cilium.io/v2"
kind: CiliumNode
metadata:
  name: "minikube"
spec:
  ipam:
    pool:
      192.168.1.1: {}
      192.168.1.2: {}
      192.168.1.3: {}
      192.168.1.4: {}

@pchaigno
Copy link
Member

How did we miss this in CI??

By the virtue of not having tests for this particular IPAM mode. That really should be addressed.

Arf! We should track this with an issue.

@joamaki
Copy link
Contributor Author

joamaki commented Jun 10, 2021

How did we miss this in CI??

By the virtue of not having tests for this particular IPAM mode. That really should be addressed.

Arf! We should track this with an issue.

Created a placeholder issue: #16496

@pchaigno
Copy link
Member

pchaigno commented Jun 10, 2021 •
edited

Known provisioning issues for these two builds:

test-1.19-5.4

@pchaigno
Copy link
Member

test-runtime

@pchaigno
Copy link
Member

test-1.19-5.4

@pchaigno
Copy link
Member

test-runtime

@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 10, 2021
@joamaki
Copy link
Contributor Author

joamaki commented Jun 10, 2021

Marked ready to merge since relevant tests for this pass and this passed manual test, but feel free to disagree and pull the label and wait for provisioning issues to be solved.

@aditighag aditighag merged commit 4118b04 into cilium:master Jun 10, 2021
@gandro gandro mentioned this pull request Jun 14, 2021
Merged
@aanm aanm mentioned this pull request Jun 16, 2021
Merged
@joamaki joamaki mentioned this pull request Sep 9, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
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/bug This PR fixes an issue in a previous release of Cilium. sig/ipam IP address management, including cloud IPAM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cilium agent not starting with ipam: crd in v1.10.0
6 participants
@joamaki @pchaigno @aanm @qmonnet @gandro @aditighag