8000 IPAM: Add CRD IPAM pool owner check by llhhbc · Pull Request #21379 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ cilium Public

IPAM: Add CRD IPAM pool owner check #21379

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
Merged
merged 1 commit into from
Oct 4, 2022
Merged

IPAM: Add CRD IPAM pool owner check #21379

merged 1 commit into from
Oct 4, 2022

Conversation

llhhbc
Copy link
Contributor
@llhhbc llhhbc commented Sep 21, 2022 •
edited by gandro
Loading 
Signed-off-by: LongHui Li <llhwonderoflighthbc@gamil.com>

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #issue-number

ipam: Support custom owner IPs in CRD IPAM pool

I want to assign the assigned ip to the specified pod through IPAM, when I assign the ip in the ippool, the ip is not assigned the way I expect.
Here is my test config:

    ipam:
      pool:
        10.244.0.1: {}
        10.244.0.2: {}
        10.244.0.3: {}
        10.244.0.4: {}
        10.244.0.5: {}
        10.244.0.6: {}
        10.244.0.7: {}
        10.244.0.8:
          owner: default/po1

@llhhbc llhhbc requested a review from a team as a code owner September 21, 2022 02:34
@llhhbc llhhbc requested a review from gandro September 21, 2022 02:34
@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 21, 2022
gandro
gandro requested changes Sep 21, 2022
View reviewed changes
Copy link
Member
@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks! I guess this makes sense overall. Manual IPAM CRD mode is not something we currently test or support as part of our CI, but this seems simple enough to add.

A few pointers that I think need to be addressed.

@gandro
Copy link
Member
gandro commented Sep 21, 2022

Checkpatch is complaining about the signed-off by line. Please sign it of with the same name and email address as the commit author:

Missing Signed-off-by: line by nominal patch author '"longhui.li" <longhui.li@woqutech.com>'

Also, the commit message is not correct:

HostCope: Add CRD IPAM pool owner check

This does not affect hostscope (i.e. clusterpool) IPAM. Let's rename the commit message to something like ipam: Support custom owner IPs in CRD IPAM pool

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/ipam labels Sep 21, 2022
@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 21, 2022
@llhhbc llhhbc requested a review from gandro September 21, 2022 11:01
gandro
gandro requested changes Sep 21, 2022
View reviewed changes
@llhhbc
ipam: Support custom owner IPs in CRD IPAM pool
5f863c3 
Signed-off-by: LongHui Li <llhwonderoflighthbc@gamil.com>
@llhhbc llhhbc requested a review from gandro September 22, 2022 01:27
gandro
gandro reviewed Sep 22, 2022
View reviewed changes
@gandro
Copy link
Member
gandro commented Sep 22, 2022

/test

@llhhbc
Copy link
Contributor Author
llhhbc commented Sep 25, 2022

/test

@gandro CI failed to run, is it a problem with the code?

@gandro
Copy link
Member
gandro commented Sep 26, 2022

/test

@gandro CI failed to run, is it a problem with the code?

No, it's unrelated to your code. We had an infrastructure issue last week. I'll restart the affected tests.

@gandro
Copy link
Member
gandro commented Sep 26, 2022

/test-1.16-4.9

@gandro
Copy link
Member
gandro commented Sep 26, 2022

/test-1.23-5.4

@gandro
8000 Copy link
Member
gandro commented Sep 26, 2022

/test-1.24-4.19

@gandro
Copy link
Member
gandro commented Sep 26, 2022

/test-1.25-net-next

@gandro
Copy link
Member
gandro commented Sep 26, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test-runtime

Job 'Cilium-PR-K8s-1.23-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-n5ckh

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

@llhhbc
Copy link
Contributor Author
llhhbc commented Sep 30, 2022 •
edited by maintainer-s-little-helper bot
Loading

/mlh new-flake Cilium-PR-K8s-1.23-kernel-5.4

👍 created #21519
8000

@llhhbc llhhbc requested a review from gandro September 30, 2022 02:19
gandro
gandro approved these changes Oct 3, 2022
View reviewed changes
Copy link
Member
@gandro gandro left a comment

Choose a reason for hiding this comment

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

Marking as ready to merge, the failed CI suite is unrelated

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 3, 2022
@ti-mo ti-mo merged commit a5b44f4 into cilium:master Oct 4, 2022
@llhhbc llhhbc mentioned this pull request Oct 12, 2022
Closed
7 tasks
llhhbc added a commit to llhhbc/cilium that referenced this pull request Oct 13, 2022
@llhhbc
IPAM: fix ipam owner check
< 8000 a href="/llhhbc/cilium/commit/9b30a7f222339364e281e31f2e8d0649d9e34338" class="Link--secondary">9b30a7f 
Fix bug with incomplete check in MR cilium#21379

Signed-off-by: LongHui Li <longhui.li@woqutech.com>
@llhhbc llhhbc mentioned this pull request Oct 13, 2022
Merged
7 tasks
aanm pushed a commit that referenced this pull request Oct 21, 2022
@llhhbc @aanm
IPAM: fix ipam owner check
5facf8a 
Fix bug with incomplete check in MR #21379

Signed-off-by: LongHui Li <longhui.li@woqutech.com>
@xgp01 xgp01 mentioned this pull request Jul 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@llhhbc @gandro @ti-mo
0