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

Delete deprecated CNPStatusUpdates and K8sEventHandover #29395

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Delete deprecated CNPStatusUpdates and K8sEventHandover #29395

merged 1 commit into from
Dec 1, 2023

Conversation

marseel
Copy link
Contributor

@marseel marseel commented Nov 27, 2023 •
edited

Delete deprecated CNPStatusUpdates and K8sEventHandover

Also, we introduce the hidden option "legacy-turn-off-k8s-event-handover".
What was happening before in setups with CEP CRD Disabled and kvstore:

When K8sEventHandover was disabled:

  • We were opening informer for all pods - even though we were watching
    endpoints from kvstore too

When K8sEventHandover was enabled:

  • We were opening informer for all pods
  • Once connected to kvstore, we were closing this informer
  • We were opening node's local pods informer

Now, the second option is the default and hidden option
"legacy-turn-off-k8s-event-handover" allows us to fallback to the first
behavior - not recommended, but just a failsafe in case we need
mitigation.

For now, the option to remove stale CNP statuses is left in case someone needs to clean them up. We can remove that part of the code in the next release.

Related: #24503

Deprecated helm options enableK8sEventHandover/enableCnpStatusUpdates were removed.
Corresponding flag "enable-k8s-event-handover" in Agent and "cnp-status-update-interval" in operator were removed.

@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 Nov 27, 2023
@marseel marseel force-pushed the deprecate_cnp_status branch 6 times, most recently from d793c45 to e361aa4 Compare November 27, 2023 14:21
@marseel
Copy link
Contributor Author

marseel commented Nov 27, 2023

/test

@marseel
Copy link
Contributor Author

marseel commented Nov 28, 2023

/test

@marseel
Copy link
Contributor Author

marseel commented Nov 28, 2023

/test

@marseel
Copy link
Contributor Author

marseel commented Nov 28, 2023

/test

@marseel marseel force-pushed the deprecate_cnp_status branch 2 times, most recently from 529fd77 to 29b8f18 Compare November 28, 2023 11:11
@marseel
Copy link
Contributor Author

marseel commented Nov 28, 2023

/test

@marseel marseel added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Nov 29, 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 Nov 29, 2023
@marseel
Copy link
Contributor Author

marseel commented Nov 29, 2023

/test

@marseel
Copy link
Contributor Author

marseel commented Nov 29, 2023

/test

@marseel marseel marked this pull request as ready for review November 29, 2023 10:45
@marseel marseel requested review from a team as code owners November 29, 2023 10:46
christarazi
christarazi reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but what about the CRD field?

@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/operator Impacts the cilium-operator component labels Nov 29, 2023
@marseel
Copy link
Contributor Author

marseel commented Nov 29, 2023

Mostly LGTM, but what about the CRD field?

I was thinking about leaving that for now and remove it later to be able to clean the existing node statuses:

cilium/operator/cmd/root.go

Line 675 in 23e7c4d

RunCNPStatusNodesCleaner(

lambdanis
lambdanis approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Docs ok, I left one non-blocking suggestion.

Documentation/internals/cilium_operator.rst Show resolved Hide resolved
viktor-kurchenko
viktor-kurchenko approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko left a comment

Choose a reason for hiding this comment

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

LGTM.

@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 Dec 1, 2023
@marseel
Copy link
Contributor Author

marseel commented Dec 1, 2023 •
edited

Rebased onto main, conflict only in Documentation/operations/upgrade.rst

@marseel
Copy link
Contributor Author

marseel commented Dec 1, 2023

/test

@aanm aanm enabled auto-merge December 1, 2023 13:38
@maintainer-s-little-helper
Copy link

Commit 947bfa0 does not match "(?m)^Signed-off-by:".

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

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 1, 2023
@marseel marseel disabled auto-merge December 1, 2023 16:13
@maintainer-s-little-helper
Copy link

Commit 947bfa0 does not match "(?m)^Signed-off-by:".

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

@marseel
Delete deprecated CNPStatusUpdates and K8sEventHandover
59399f0 
Also, we introduce hidden option "legacy-turn-off-k8s-event-handover".
What was happening before in setups with CEP CRD Disabled and kvstore:
When K8sEventHandover was disabled:
- We were opening informer for all pods - even though we were watching
  endpoints from kvstore too
When K8sEventHandover was enabled:
- We were opening informer for all pods
- Once connected to kvstore, we were closing this informer
- We were opening node's local pods informer
Now second options is default and hidden option
"legacy-turn-off-k8s-event-handover" allows us to fallback to first
behaviour - not recommended, but just failsafe in case we need
mitigation

Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 1, 2023
@aanm aanm enabled auto-merge December 1, 2023 16:22
@aanm
Copy link
Member

aanm commented Dec 1, 2023

/test

@aanm aanm added this pull request to the merge queue Dec 1, 2023
Merged via the queue into cilium:main with commit 2835b06 Dec 1, 2023
61 checks passed
@joestringer joestringer mentioned this pull request Apr 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@lambdanis lambdanis lambdanis approved these changes

@christarazi christarazi christarazi approved these changes

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. area/operator Impacts the cilium-operator component 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. 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.

None yet
6 participants
@marseel @aanm @squeed @lambdanis @christarazi @viktor-kurchenko