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.

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

labelsfilter: ignore StatefulSet-related labels by default for CID creation #28003

Merged
merged 1 commit into from Sep 15, 2023

Conversation

tosi3k
Copy link
Contributor

@tosi3k tosi3k commented Sep 7, 2023

Beforehand, every single Pod owned by a StatefulSet object had its own CiliumIdentity because each such Pod had their own unique set of label values (more about these labels can be found here and here).

In large clusters with many StatefulSet-owned Pods it is problematic, especially when the churn of such objects is high, resulting in a high churn of such CIDs that doesn't scale well. In addition, it essentially limited the number of such Pods to the theoretical number of CIDs in a cluster which is 2^16 ~ 65k.

Ignore StatefulSet-specific labels by default for CID creation. This includes the two following labels:
* statefulset.kubernetes.io/pod-name
* apps.kubernetes.io/pod-index

@tosi3k tosi3k requested review from a team as code owners September 7, 2023 15:54
@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 7, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Sep 7, 2023
@maintainer-s-little-helper
Copy link

Commit 0a9a0bb2a26cb3e2f13e43702fe6bae44be7a541 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 Sep 7, 2023
@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 Sep 7, 2023
@tosi3k
Copy link
Contributor Author

tosi3k commented Sep 13, 2023

@nathanjsweet @lambdanis PTAL

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, overall the change looks reasonable to me.

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

@tosi3k CI is unhappy with the long commit message (https://github.com/cilium/cilium/actions/runs/6187912035/job/16803422512?pr=28003):

{[1/2] labelsfilter: ignore StatefulSet-specific labels by default for CID creation}

Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 76)

Could you please reword it?

…eation

This prevents creating a CiliumIdentity object per StatefulSet-owned
Pod. This was especially costly in clusters with large quantities of
StatefulSet pods and basically limited the number of such pods in a
single cluster to a theoretical max number of CIDs (2^16 ~ 65k).

Signed-off-by: Antoni Zawodny <zawodny@google.com>
@tosi3k tosi3k changed the title labelsfilter: ignore StatefulSet-specific labels by default for CID creation labelsfilter: ignore StatefulSet-related labels by default for CID creation Sep 14, 2023
@tosi3k
Copy link
Contributor Author

tosi3k commented Sep 14, 2023

@tosi3k CI is unhappy with the long commit message (https://github.com/cilium/cilium/actions/runs/6187912035/job/16803422512?pr=28003):

{[1/2] labelsfilter: ignore StatefulSet-specific labels by default for CID creation}

Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 76)

Could you please reword it?

Done; thanks for pointing this out.

@lambdanis
Copy link
Contributor

/test

@lambdanis lambdanis self-assigned this Sep 14, 2023
@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 Sep 14, 2023
@julianwiedmann julianwiedmann added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Sep 15, 2023
@julianwiedmann julianwiedmann merged commit 6d6a116 into cilium:main Sep 15, 2023
60 of 61 checks passed
@joestringer joestringer added the upgrade-impact This PR has potential upgrade or downgrade impact. label Apr 30, 2024
@joestringer
Copy link
Member

joestringer commented Apr 30, 2024

Hi @tosi3k, it looks like this PR introduced upgrade impact for users upgrading to Cilium v1.15 because users may have policies that rely on some of the labels that are now ignored. This issue/comment goes into detail about the issues behind this and some suggestions about how to improve: #32213 (comment) . I'll note that while the release-note/minor labeling for this PR does cause a release note to be generated for users, it is easy to miss; for something that requires direct user attention, we would typically document the change for users under Documentation/operations/upgrade.rst. Or, better than that, if we can encode checks into the preflight daemonset then users may find out and take action before they even upgrade, thereby minimizing the impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. 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/policy Impacts whether traffic is allowed or denied based on user-defined policies. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants