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

Use slimmer protobuf definitions on k8s structures #11326

Merged
merged 4 commits into from May 6, 2020
Merged

Use slimmer protobuf definitions on k8s structures #11326

merged 4 commits into from May 6, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented May 4, 2020 •
edited

With this slimmer Pod structure, Cilium won't need to convert a Pod into
another structure that Cilium understands. This conversion will
automatically happen when doing the unmarshal of k8s event. Thus,
providing better memory management as well as CPU consumption since
Cilium will only unmarshal the fields that actually need.

Note for reviewers:

  • a lot of the code is auto-generated
  • please take a look at pkg/k8s/types/slim/README.md

Benchmark for 50k pods (memory allocated):
base

alloc: 589.30MB (617927992 bytes)
total-alloc: 1.92GB (2057608072 bytes)
sys: 942.65MB (988440577 bytes)

changes

alloc: 215.13MB (225575000 bytes)
total-alloc: 1.47GB (1578649424 bytes)
sys: 340.07MB (356588484 bytes)

@aanm aanm added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. kind/epic Features that are considered beyond amazing labels May 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 4, 2020
@aanm
Copy link
Member Author

aanm commented May 4, 2020

test-me-please

@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch from a05d731 to a6d0b8e Compare May 4, 2020 21:19
@aanm
Copy link
Member Author

aanm commented May 4, 2020

test-me-please

@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch from a6d0b8e to bd2804d Compare May 5, 2020 07:28
@aanm
Copy link
Member Author

aanm commented May 5, 2020

test-me-please

@coveralls
Copy link

coveralls commented May 5, 2020 •
edited

Coverage Status

Coverage decreased (-2.0%) to 42.48% when pulling 16ce14e on pr/use-slimmer-protobuf-definitions into dd99958 on master.

@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch from bd2804d to e642d21 Compare May 5, 2020 10:17
@aanm
Copy link
Member Author

aanm commented May 5, 2020

test-me-please

@aanm aanm marked this pull request as ready for review May 5, 2020 13:37
@aanm aanm requested a review from a team as a code owner May 5, 2020 13:37
@aanm aanm requested a review from a team May 5, 2020 13:37
@aanm aanm requested review from a team as code owners May 5, 2020 13:37
@aanm aanm requested a review from a team May 5, 2020 13:37
@aanm aanm requested a review from a team as a code owner May 5, 2020 13:37
tklauser
tklauser approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Impressive memory improvements!

Didn't review in depth, but mainly mainly checked the code generation parts and usage of the new types.

tklauser
tklauser reviewed May 6, 2020
View reviewed changes
pkg/k8s/types/slim/client/clientset/versioned/clientset.go Outdated
// See the License for the specific language governing permissions and
// limitations under the License.

// Code generated by client-gen. DO NOT EDIT.
Copy link
Member

Choose a reason for hiding this comment

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

Not related to the contents of this PR, just something that puzzled me during review:

I was wondering why GitHub didn't recognize this file as being auto-generate despite this line being present while other files with the same comment are correctly recongnized. Turns out that github-linguist only checks the first 10 lines of a file for the Code generated by line:

https://github.com/github/linguist/blob/309acde92e04c86466441afc12992274be938473/lib/linguist/generated.rb#L305

However, https://golang.org/s/generatedcode states that the comment may appear anywhere in the file. I wonder whether it'd be possible to improve github-linguist in that regard /cc @pchaigno

Copy link
Member

Choose a reason for hiding this comment

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

You're right, Linguist performs a best-effort detection of generated files in that regard. We impose a limit because we want to avoid searching potentially huge files. Nevertheless, in the present case, I think the limit could be bumped to match the longest license text that could be included here (probably BSD 4-clause, so about 40 lines).

If you want to send a quick fix for that, I can make sure it gets included in the next release (usually once a month).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for elaborating @pchaigno. I agree that it wouldn't make sense to search the whole file. Will send a quick fix bumping the limit.

Copy link
Member

Choose a reason for hiding this comment

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

Sent PR github-linguist/linguist#4857

Copy link
Member

Choose a reason for hiding this comment

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

We could also switch to SPDX to minimize boilerplate and not have to search so much :-)

Copy link
Member

Choose a reason for hiding this comment

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

We could also switch to SPDX to minimize boilerplate and not have to search so much :-)

Please do, so we have it for our code-base everywhere, not just C code. :-)

@tklauser tklauser mentioned this pull request May 6, 2020
Merged
@aanm aanm removed the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label May 6, 2020
@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch from e642d21 to 7c473aa Compare May 6, 2020 11:27
@aanm
Copy link
Member Author

aanm commented May 6, 2020

test-me-please

1 similar comment
@aanm
Copy link
Member Author

aanm commented May 6, 2020

test-me-please

@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch 2 times, most recently from 5b4ed49 to 25eae2c Compare May 6, 2020 13:23
aanm added 2 commits May 6, 2020 15:23
@aanm
api/v1: move protc-gen* to tools/
6355779 
Signed-off-by: André Martins <andre@cilium.io>
@aanm
types/slim: add slim packages for k8s structures
c4f67fb 
With these slimmer structures Cilium will only unmarshal the fields
that it uses as opposed to unmarshal all fields and later on ignore the
unused fields.

For now, only Pod will be added as a PoC, later on all other k8s
structures will have a similar change.

Signed-off-by: André Martins <andre@cilium.io>
aanm added 2 commits May 6, 2020 15:23
@aanm
Use NamespaceNameGetter interface instead of metav1.Object
7b76557 
Signed-off-by: André Martins <andre@cilium.io>
@aanm
use slimmer Pod structure across code base
16ce14e 
With this slimmer Pod structure, Cilium won't need to convert a Pod into
another structure that Cilium understands. This conversion will
automatically happen when doing the unmarshal of k8s event. Thus,
providing better memory management as well as CPU consumption since
Cilium will only unmarshal the fields that actually need.

Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member Author

aanm commented May 6, 2020

test-me-please

@aanm aanm force-pushed the pr/use-slimmer-protobuf-definitions branch from 25eae2c to 16ce14e Compare May 6, 2020 13:24
@aanm aanm merged commit a7f8fc8 into master May 6, 2020
1.8.0 automation moved this from In progress to Merged May 6, 2020
@aanm aanm deleted the pr/use-slimmer-protobuf-definitions branch May 6, 2020 19:54
@aanm aanm mentioned this pull request May 6, 2020
Merged
lildude pushed a commit to github-linguist/linguist that referenced this pull request May 20, 2020
@tklauser
Bump line limit for generated_go (#4857)
e431970 
In some cases (e.g. the Cilium project) the `Code generated by` comment
appears after a license header. This header might be longer than 10
lines which generated_go currently searches for this comment.

Bump the limit to 40 lines (roughly corresponding to the length of the
BSD 4-clause header + some slack) to detect these cases.

Suggested by Paul Chaignon in
cilium/cilium#11326 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@borkmann borkmann borkmann left review comments

@joestringer joestringer joestringer left review comments

@pchaigno pchaigno pchaigno left review comments

Assignees
No one assigned
Labels
kind/epic Features that are considered beyond amazing release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aanm @coveralls @tklauser @borkmann @joestringer @pchaigno