Introduce scope configuration for internal caches #133
Conversation
a-hilaly
added
do-not-merge/hold
|
cc @acornett21 |
pkg/runtime/service_controller.go
Outdated
| // NOTE: Maybe we should make this configurable? It's not clear that | ||
| // we'd ever want to watch these namespaces. | ||
| Ignored: []string{ | ||
| "ack-system", |
There was a problem hiding this comment.
Do we really want to go down the path of hiding what is ignored? It gets into a slippery slope. Maybe for some kube namespaces, but I don't think ack-system should be ignored. There are probably users out there that are creating CR's in this namespace since it's the default namespace that the controller runs in.
Edit: It looks like we sort of already had this, just in a different part of the code, which seems odd to me, but I guess because the implementation was that each NS has it's own cache.
There was a problem hiding this comment.
Since we added cross-account resource management to ACK, the caches ignored to the ack-system namespace (or the namespace where the controller is running). Still, the controller will handle resources in that namespace using the controller role. This means that only CARM will not work on ack-system.
Now, looking at the bigger picture, I agree it might be a bit odd, but we might just allow it since there don't seem to be any issues.. and it won't have any side effects
There was a problem hiding this comment.
I still don't think it's a good idea to expose this level of configuration at the helm/flag level of controllers. However i'm down to remove ack-system from the ignored list and only leave kube-system, kube-public and kube-node-lease.
There was a problem hiding this comment.
I still don't think it's a good idea to expose this level of configuration at the helm/flag level of controllers. However i'm down to remove
ack-systemfrom the ignored list and only leavekube-system,kube-publicandkube-node-lease.
I also do not thing adding a configuration for this is needed, that would add more work/confusion and we'd need to change some documentation.
a-hilaly
force-pushed
the
caches-config
branch
from
9726c5b
to
14773a3
Compare
a-hilaly
removed
do-not-merge/hold
a-hilaly
force-pushed
the
caches-config
branch
from
14773a3
to
1bc59e3
Compare
|
/hold |
ack-prow
bot
added
the
do-not-merge/hold
a-hilaly
force-pushed
the
caches-config
branch
from
1bc59e3
to
8fa15f8
Compare
a-hilaly
force-pushed
the
caches-config
branch
from
8fa15f8
to
0eb366b
Compare
This patch enhances the runtime internal caches by introducing a configuration mechanism to tailor their behavior. The caches can now be configured to watch a specific set of namespaces (instead of the previous "all or nothing" style). This is intended to be complete the multi-namspace-watch mode feature. Now when a user configures a controller to watch a set of namespaces the ACK runtime caches will also respect that scope. In addition this commit adds `kube-node-lease` to the set of namespace ignored by default (This namespace is dedicated to node Leases objects). Signed-off-by: Amine Hilaly <hilalyamine@gmail.com>
ack-prow
bot
added
the
do-not-merge/invalid-owners-file
a-hilaly
force-pushed
the
caches-config
branch
from
0eb366b
to
8183528
Compare
ack-prow
bot
removed
the
do-not-merge/invalid-owners-file
|
/lgtm |
ack-prow
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, ack-bot, jonathan-innis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…s#133) This patch enhances the runtime internal caches by introducing a configuration mechanism to tailor their behavior. The caches can now be configured to watch a specific set of namespaces (instead of the previous "all or nothing" style). This is intended to be complete the multi-namspace-watch mode feature. Now when a user configures a controller to watch a set of namespaces the ACK runtime caches will also respect that scope. In addition this commit adds `kube-node-lease` to the set of namespace ignored by default (This namespace is dedicated to node Leases objects). Signed-off-by: Amine Hilaly <hilalyamine@gmail.com> By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
This patch enhances the runtime internal caches by introducing a
configuration mechanism to tailor their behavior. The caches can now be
configured to watch a specific set of namespaces (instead of the
previous "all or nothing" style).
This is intended to be complete the multi-namspace-watch mode feature.
Now when a user configures a controller to watch a set of namespaces the
ACK runtime caches will also respect that scope.
In addition this commit adds
kube-node-leaseto the set of namespaceignored by default (This namespace is dedicated to node Leases objects).
Signed-off-by: Amine Hilaly hilalyamine@gmail.com
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.