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

docs(user guide) - Add MultiNamespacedCacheBuilder option to manager section #2010

Closed

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Oct 3, 2019

Description of the change:

  • Add MultiNamespacedCacheBuilder option to manager section
  • update metrics doc which is outdated.

Motivation for the change:
Closes: #2008

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2019
MetricsBindAddress: fmt.Sprintf("%s:%d", metricsHost, metricsPort),
})
```
Also, it can restrict a list of namespaces that all controllers will watch for resources:
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I remember @shawn-hurley saying that it turns out that multi-namespace watches are non-performant, that support for them probably should not have been added, and that we should tell users either not to use them or be extremely careful if they do.

IIRC, the root of the problem is that the Kubernetes API has to create an individual watch against etcd for each namespace being watched due to limitations of the data model and etcd. So if operators have a use case like "watch every namespace except for kube-system, and the operator developer has never tested or used the operator in a large scale cluster (think 1000's of namespaces), then the operator could cause performance issues in etcd and the apiserver.

So long story short, I'm not sure we want to add prominent documentation about multi-namespace support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @joelanford,

Thank you for raise this concern. If it can cause performance issues I think we should add it in the doc instead of not add this info at all.

The reason for that is because in my understanding in a lot of scenarios users would like to use it because of security reasons. I mean, it is a way to restrict the namespaces which the operator should watch/do actions.

I will try to address your comment/concern.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on board with adding something to the docs about the potential problems with multi-namespace watches. Since that's a feature of controller-runtime, I think it would be better suited upstream, probably in the controller-runtime godoc and perhaps also in the kubebuilder book.

Since we're trying to consolidate documentation, I think we should avoid creating docs about controller-runtime features in the SDK repo, and instead make sure we have prominent links to upstream docs.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Oct 3, 2019

Choose a reason for hiding this comment

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

I understand your point. The problem in POV about we do not add the info here is:

So, IMHO: just address there will still making this subject came back in the thread channels :-(

Copy link
Member

Choose a reason for hiding this comment

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

It shows one of the 10 questions

Are you referring to this: #767 (comment)? If so, please link to that issue in your description for #2008 and #2010.

very hard for the users and us indeed to know that exists

This is kind of what I'm getting at 🙂. My thought is that we probably shouldn't make multi-namespace support obvious to novice users in our user guide because they're not as likely to understand that it can be used incorrectly, even if we include the disclaimers.

More broadly, this section about the manager is really controller-runtime documentation, not SDK documentation. We should not maintain separate documentation from controller-runtime about controller-runtime APIs because it is extremely likely to get out of date.

Like I said, I'm okay with documenting the multi-namespace cache, but I think it should be in upstream docs, which we can link to.

I think if we update the GoDoc here and here to improve the description, add the disclaimers, and include an example that would be a great start. At that point, maybe we could add a line about it in the Advanced Topics section.

@shawn-hurley what are your thoughts on this? Was there/is there discussion about backing multi-namespace watches out of controller-runtime?

doc/user-guide.md Show resolved Hide resolved
doc/user/metrics/README.md Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
doc/user/metrics/README.md Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title Issue 2008 Add MultiNamespacedCacheBuilder option to manager section Oct 3, 2019
@camilamacedo86 camilamacedo86 changed the title Add MultiNamespacedCacheBuilder option to manager section docs(user guide) -Add MultiNamespacedCacheBuilder option to manager section Oct 3, 2019
@camilamacedo86 camilamacedo86 changed the title docs(user guide) -Add MultiNamespacedCacheBuilder option to manager section docs(user guide) - Add MultiNamespacedCacheBuilder option to manager section Oct 3, 2019

The Manager will automatically register the scheme for all custom resources defined under `pkg/apis/...` and run all controllers under `pkg/controller/...`.

The Manager can restrict the namespace that all controllers will watch for resources:
```Go
mgr, err := manager.New(cfg, manager.Options{Namespace: namespace})
// Create a new Cmd to provide shared dependencies and start components
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed this on my last review. I don't think this comment is necessary. Ditto for the all namespaces example.

- [Overview](#overview)
- [Metrics in Operator SDK](#metrics-in-operator-sdk)
- [General metrics](#general-metrics)
- [Usage:](#usage)
Copy link
Member

Choose a reason for hiding this comment

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

Extraneous colon. We can remove it from the section header as well.

@@ -91,3 +110,4 @@ The operator uses [Prometheus][prometheus] to expose a number of metrics by defa
[ownerref-permission]: https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement
[ksm]: https://github.com/kubernetes/kube-state-metrics
[controller-metrics]: https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/internal/controller/metrics
[kube-prometheus]: https://github.com/coreos/kube-prometheus
Copy link
Member

Choose a reason for hiding this comment

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

I think we should link here:

Suggested change
[kube-prometheus]: https://github.com/coreos/kube-prometheus
[prometheus-operator]: https://github.com/coreos/prometheus-operator

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Oct 3, 2019

Choose a reason for hiding this comment

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

I need use these days and the https://github.com/coreos/prometheus-operator was not working when we follow the steps then I spoke with its maintainers they said that they are no longer really maintainer and using it and suggested me use the https://github.com/coreos/kube-prometheus which actually as very straight forward.

IMHO: Is very confuse keep both links and can lead people to try to install it following the https://github.com/coreos/prometheus-operator. So, wdyt about we just use links for https://github.com/coreos/kube-prometheus?


[Prometheus][prometheus] is an open-source systems monitoring and alerting toolkit. Below is the overview of the different helpers that exist in Operator SDK to help setup metrics in the generated operator.

> **NOTE**: You can install the [Prometheus][prometheus] via the [kube-prometheus Operator][kube-prometheus].
Copy link
Member

Choose a reason for hiding this comment

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

See my other comment about changing this link

Comment on lines 59 to +71
// Add to the below struct any other metrics ports you want to expose.
servicePorts := []v1.ServicePort{
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
}

// Create Service object to expose the metrics port.
_, err = metrics.CreateMetricsService(context.TODO(), cfg, servicePorts)
if err != nil {
// handle error
}

...
servicePorts := []v1.ServicePort{
{Port: metricsPort, Name: metrics.OperatorPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: metricsPort}},
{Port: operatorMetricsPort, Name: metrics.CRPortName, Protocol: v1.ProtocolTCP, TargetPort: intstr.IntOrString{Type: intstr.Int, IntVal: operatorMetricsPort}},
}

// Create Service object to expose the metrics port(s).
service, err := metrics.CreateMetricsService(ctx, cfg, servicePorts)
if err != nil {
// handle error here
}

...
Copy link
Member

Choose a reason for hiding this comment

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

This snippet is about creating the service that exposes the metrics. So maybe we separate this out into a section about exposing metrics and include the snippet that sets up the ServiceMonitor?

With that in mind, we might need to tweak the doc organization to use the following sections:

  1. Rename General metrics -> Configuring controller-runtime metrics
  2. Rename Custom resource specific metrics -> Configuring metrics about custom resources
  3. Move and rename Expose Custom Metrics -> Subsection of Configuring controller-runtime metrics called Defining custom metrics
  4. New section called Exposing metrics, that includes snippets for service creation and service monitor creation, and includes the Garbage collection section since that is referring to the service that gets created.

Would that make this doc flow a little better? WDYT?

MetricsBindAddress: fmt.Sprintf("%s:%d", metricsHost, metricsPort),
})
```
Also, it can restrict a list of namespaces that all controllers will watch for resources:
Copy link
Member

Choose a reason for hiding this comment

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

It shows one of the 10 questions

Are you referring to this: #767 (comment)? If so, please link to that issue in your description for #2008 and #2010.

very hard for the users and us indeed to know that exists

This is kind of what I'm getting at 🙂. My thought is that we probably shouldn't make multi-namespace support obvious to novice users in our user guide because they're not as likely to understand that it can be used incorrectly, even if we include the disclaimers.

More broadly, this section about the manager is really controller-runtime documentation, not SDK documentation. We should not maintain separate documentation from controller-runtime about controller-runtime APIs because it is extremely likely to get out of date.

Like I said, I'm okay with documenting the multi-namespace cache, but I think it should be in upstream docs, which we can link to.

I think if we update the GoDoc here and here to improve the description, add the disclaimers, and include an example that would be a great start. At that point, maybe we could add a line about it in the Advanced Topics section.

@shawn-hurley what are your thoughts on this? Was there/is there discussion about backing multi-namespace watches out of controller-runtime?

@camilamacedo86 camilamacedo86 force-pushed the Issue_2008 branch 2 times, most recently from f1f14e2 to b4546c6 Compare October 3, 2019 18:41
@openshift-ci-robot
Copy link

@camilamacedo86: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/unit c3d7de4 link /test unit
ci/prow/sanity c3d7de4 link /test sanity

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
3 participants