10000 fix: Assign PodStore from Pod resource until cell migration is completed by dlapcevic · Pull Request #34090 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Assign PodStore from Pod resource until cell migration is completed #34090

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
Aug 13, 2024
Merged

fix: Assign PodStore from Pod resource until cell migration is completed #34090

merged 1 commit into from
Aug 13, 2024

Conversation

dlapcevic
Copy link
Contributor
@dlapcevic dlapcevic commented Jul 30, 2024 •
edited by pchaigno
Loading

Fixes: #32713

Changes:

  1. PodStore from Pod resource is assigned to PodStore of cilium-operator on startup.
  2. PodNodeNameIndex is added to the Pod watcher in operatorK8s.PodResource.

This will be refactored once ciliumNodeSynchronizer is migrated to a cell.

Signed-off-by: Dorde Lapcevic <dordel@google.com>

Fixed bug which prevented IP surge allocation from working

@dlapcevic dlapcevic requested review from a team as code owners July 30, 2024 13:20
@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 Jul 30, 2024
@dlapcevic dlapcevic mentioned this pull request Jul 30, 2024
Closed
3 tasks
tommyp1ckles
tommyp1ckles requested changes Jul 31, 2024
View reviewed changes
@dlapcevic dlapcevic requested a review from tommyp1ckles July 31, 2024 15:18
@dlapcevic
Copy link
Contributor Author

/test

@dlapcevic
Copy link
Contributor Author

/ci-ginkgo

@dlapcevic
fix: Assign PodStore from Pod resource until cell migration is completed
054924f 
Fixes cilium#32713

PodNodeNameIndex is added to the Pod watcher in operatorK8s.Resource.

This will be refactored once `ciliumNodeSynchronizer` is migrated to a cell.

Signed-off-by: Dorde Lapcevic <dordel@google.com>
@dlapcevic
Copy link
Contributor Author

/test

@dlapcevic
Copy link
Contributor Author

/ci-ginkgo

@dlapcevic
Copy link
Contributor Author

/ci-runtime

@dlapcevic
Copy link
Contributor Author

Hi @tommyp1ckles, could we please merge this?

@dlapcevic
Copy link
Contributor Author

Hi @joamaki, could we please merge this?

@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Aug 9, 2024
@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 Aug 9, 2024
@joamaki joamaki enabled auto-merge August 9, 2024 14:57
tommyp1ckles
tommyp1ckles approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor
@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, changes look good to me, thanks for the fix 😄

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 9, 2024
@joamaki joamaki added this pull request to the merge queue Aug 13, 2024
Merged via the queue into cilium:main with commit c3db8a5 Aug 13, 2024
66 checks passed
@dlapcevic dlapcevic mentioned this pull request Aug 15, 2024
Merged
@christarazi christarazi added kind/bug This is a bug in the Cilium logic. area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. area/operator Impacts the cilium-operator component labels Sep 26, 2024
@christarazi
Copy link
Member

@dlapcevic Does this need to be backported to v1.16?

@dlapcevic
Copy link
Contributor Author

@dlapcevic Does this need to be backported to v1.16?

Yes. It was reported in v1.15, so it should be backported there too. #32713

8000
@christarazi christarazi added the needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch label Oct 2, 2024
@giorio94 giorio94 mentioned this pull request Oct 7, 2024
Merged
15 tasks
@giorio94 giorio94 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Oct 7, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Oct 8, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Oct 9, 2024
Merged
@jaredledvina
Copy link
Contributor

👋 Could this be included with the needs-backport/1.15 label?
@thorn3r I see you have cilium/release#267 (or @christarazi) would either of you have permission to do that?
It'd be a shame for folks upgrading to 1.15 to pick up #32713 and lose the surge allocate functionality.

@christarazi
Copy link
Member

@jaredledvina It's not obvious to me how difficult the backport would be to v1.15 given the scope of changes to the code base since then compared to today. Going off of https://docs.cilium.io/en/v1.15/contributing/release/backports/#id1, I'm not sure if this would be considered a major bug. Otherwise, we'd recommend upgrading to the latest OSS version (v1.16).

@jaredledvina
Copy link
Contributor

Ah okay, we're trying to upgrade to 1.15 currently. There's concern that we might be losing Surge Allocation because of this bug. 1.16 will be a ways off until we can finish getting to 1.15 so if a backport is doable that'd be much appreciated.

@antonipp antonipp mentioned this pull request Oct 17, 2024
Merged
@antonipp
Copy link
Contributor

I actually went ahead and did the PR for the 1.15 backport: #35419
We will be running 1.15 for a while and we'd rather not lose the surge allocation feature which is useful in our busier clusters.

@pchaigno
Copy link
Member

I'm a bit confused because this pull request is labeled as release-note/misc, but is being backported. The label suggests that there is no user impact (otherwise it would be release-note/bug), but the backporting suggests it does fix a bug. Is the label incorrect? If so, could we fix it and add a release note in the PR description?

@qmonnet qmonnet added the backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. label Oct 18, 2024
@hemanthmalla hemanthmalla mentioned this pull request Oct 18, 2024
Merged
@aanm aanm added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Oct 21, 2024
@julianwiedmann julianwiedmann added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Oct 21, 2024
@cilium-release-bot cilium-release-bot bot mentioned this pull request Nov 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/operator Impacts the cilium-operator component backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. kind/bug This is a bug in the Cilium logic. kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

operator: pod store is never initialized
0