Skip to content

Cache only required secrets#903

Merged
metal3-io-bot merged 3 commits into
metal3-io:masterfrom
andfasano:dont-cache-secrets
Jul 8, 2021
Merged

Cache only required secrets#903
metal3-io-bot merged 3 commits into
metal3-io:masterfrom
andfasano:dont-cache-secrets

Conversation

@andfasano
Copy link
Copy Markdown
Member

@andfasano andfasano commented May 31, 2021

This PR limits the BMO secrets caching just to those ones really required, by filtering secrets watch request through a new label selector.
This will fix the problem of the very high memory usage detected when BMO is configured to watch the entire cluster (WATCH_NAMESPACE=""), and moreover will keep in cache just the BMH related secrets.

Fixes #904

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2021
@andfasano
Copy link
Copy Markdown
Member Author

/hold requires controller-runtime 0.9.0

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2021
@andfasano andfasano force-pushed the dont-cache-secrets branch 2 times, most recently from 8525aca to ce32a2c Compare June 1, 2021 11:56
@metal3-io-bot metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2021
@andfasano andfasano force-pushed the dont-cache-secrets branch from 90dd1a4 to 7cf62f1 Compare June 3, 2021 09:05
@andfasano andfasano changed the title [WIP] Cache only required secrets Cache only required secrets Jun 3, 2021
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2021
Comment thread controllers/metal3.io/baremetalhost_controller.go Outdated
Comment thread controllers/metal3.io/baremetalhost_controller.go Outdated
Comment thread controllers/metal3.io/baremetalhost_controller_test.go Outdated
Comment thread go.mod Outdated
@andfasano andfasano force-pushed the dont-cache-secrets branch from 7cf62f1 to ec6e1a1 Compare June 8, 2021 09:26
@andfasano
Copy link
Copy Markdown
Member Author

/hold cancel

controller-runtime 0.9.0 has been released

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2021
@andfasano andfasano force-pushed the dont-cache-secrets branch from ec6e1a1 to fa83ffb Compare June 8, 2021 09:37
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

1 similar comment
@furkatgofurov7
Copy link
Copy Markdown
Member

/test-integration

@andfasano
Copy link
Copy Markdown
Member Author

/hold investigating test-integration failure

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2021
@andfasano
Copy link
Copy Markdown
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2021
@andfasano andfasano force-pushed the dont-cache-secrets branch from 2e407ef to a74ac8d Compare June 9, 2021 17:36
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

@metal3-io-bot
Copy link
Copy Markdown
Contributor

@andfasano: you cannot LGTM your own PR.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

@andfasano andfasano force-pushed the dont-cache-secrets branch 4 times, most recently from c31aaa8 to f145820 Compare June 11, 2021 12:03
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

@andfasano
Copy link
Copy Markdown
Member Author

/assign @zaneb
cc @hardys

Copy link
Copy Markdown
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

There are conflicting changes with this and #917 in terms of go modules

@andfasano
Copy link
Copy Markdown
Member Author

There are conflicting changes with this and #917 in terms of go modules

Which ones? I cross-checked them and it seems that the go.mod files are almost identical (all the changes here are due just the uplift of controller-runtime to 0.9.0). #917 also modifies the api go.mod

@furkatgofurov7
Copy link
Copy Markdown
Member

I cross-checked them and it seems that the go.mod files are almost identical

Yes that is what I meant actually, that both are doing the same changes in case if you were not aware of the other work.

Copy link
Copy Markdown
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

This looks basically good to me

Comment thread controllers/metal3.io/baremetalhost_controller.go Outdated
Comment thread controllers/metal3.io/host_config_data.go Outdated
@andfasano andfasano force-pushed the dont-cache-secrets branch from f145820 to 89aac5d Compare July 1, 2021 07:43
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

1 similar comment
@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jul 7, 2021

/lgtm
/test-integration

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@hardys
Copy link
Copy Markdown
Member

hardys commented Jul 8, 2021

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2021
andfasano added 2 commits July 8, 2021 07:04
Fixed manifests change (linebreaks)
Fixed broken test due changes in handling finalizers in the fake client
The internal cache now filters out secrets without the label "metal3.io.environment":"baremetal". The controller takes care of applying such label to the owned secrets only
@andfasano andfasano force-pushed the dont-cache-secrets branch from 89aac5d to 9d02680 Compare July 8, 2021 12:01
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@andfasano andfasano force-pushed the dont-cache-secrets branch from 9d02680 to 931a00a Compare July 8, 2021 12:05
@andfasano
Copy link
Copy Markdown
Member Author

Rebase was required due the CRD changes introduced in #884

@andfasano
Copy link
Copy Markdown
Member Author

/test-integration

Copy link
Copy Markdown
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

/lgtm

description: Custom deploy method name. This name is specific to the deploy ramdisk used. If you don't have a custom deploy ramdisk, you shouldn't use CustomDeploy.
description: Custom deploy method name. This name is specific
to the deploy ramdisk used. If you don't have a custom deploy
ramdisk, you shouldn't use CustomDeploy.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These changes should really be in the first commit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh right

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@metal3-io-bot metal3-io-bot merged commit bef5ef5 into metal3-io:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

High memory usage when watching all namespaces

5 participants