Enable single operator to monitor all namespaces #188

objectiser · 2019-01-31T14:28:19Z

Resolves #24

This PR demonstrates how a single operator can be used to create Jaeger instances in different namespaces.

For example,

kubectl create -n observability -f deploy/crds/io_v1alpha1_jaeger_crd.yaml
kubectl create -n observability -f deploy/service_account.yaml
kubectl create -n observability -f deploy/role.yaml
kubectl create -n observability -f deploy/role_binding.yaml
kubectl create -n observability -f deploy/operator.yaml

kubectl apply -n other -f deploy/examples/simplest.yaml

The problem with this approach is that the role_binding.yaml has to have the namespace hardcoded. Not sure if this can reference the namespace as a parameter, as with the env vars?

Options:

leave the current operator deployment focused on a single namespace and provide documentation about how to change them to watch all namespaces
if there is no way to provide a reference to the namespace in the role_binding.yaml, then maybe we would need a helm chart to install the operator and use {{ .Release.Namespace }}

Signed-off-by: Gary Brown gary@brownuk.com

jpkrohling · 2019-01-31T14:28:24Z

This change is

codecov · 2019-01-31T14:31:18Z

Codecov Report

Merging #188 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #188   +/-   ##
======================================
  Coverage    96.7%   96.7%           
======================================
  Files          32      32           
  Lines        1639    1639           
======================================
  Hits         1585    1585           
  Misses         41      41           
  Partials       13      13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd89ceb...a89f76c. Read the comment docs.

jpkrohling

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

a discussion (no related file):

leave the current operator deployment focused on a single namespace and provide documentation about how to change them to watch all namespaces

I think the most common case will be one operator for multiple namespaces. Perhaps not "all", but certainly "multiple".

if there is no way to provide a reference to the namespace in the role_binding.yaml, then maybe we would need a helm chart to install the operator and use {{ .Release.Namespace }}

Not quite sure I understood. The only namespace there is for the service account, stating where to find it. Are you having some specific permission issue?

objectiser

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jpkrohling)

a discussion (no related file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

leave the current operator deployment focused on a single namespace and provide documentation about how to change them to watch all namespaces

I think the most common case will be one operator for multiple namespaces. Perhaps not "all", but certainly "multiple".

if there is no way to provide a reference to the namespace in the role_binding.yaml, then maybe we would need a helm chart to install the operator and use {{ .Release.Namespace }}

Not quite sure I understood. The only namespace there is for the service account, stating where to find it. Are you having some specific permission issue?

The problem is that the role_binding.yaml has to hardcode the namespace of the service account - so if a user is installing jaeger-operator from the deployment files in this repo, then they would need to deploy the jaeger-operator in that predefined namespace - or else edit the role_binding.yaml to specify the actual namespace where they deployed the operator.

jpkrohling

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, objectiser (Gary Brown) wrote…

The problem is that the role_binding.yaml has to hardcode the namespace of the service account - so if a user is installing jaeger-operator from the deployment files in this repo, then they would need to deploy the jaeger-operator in that predefined namespace - or else edit the role_binding.yaml to specify the actual namespace where they deployed the operator.

Right, sorry. Makes sense now. Are these files copied over to the helm chart, or are they reused? I think it would be OK to have this specific file duplicated there, as I don't think it's possible to leave this field out (haven't tested, though).

objectiser

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Right, sorry. Makes sense now. Are these files copied over to the helm chart, or are they reused? I think it would be OK to have this specific file duplicated there, as I don't think it's possible to leave this field out (haven't tested, though).

The deployment files are copied when used in the Istio helm chart, so its not a problem there. This is about what we should provide for users who are deploying directly from the files in this repo.

We could add a namespace to all of the files and then users would have to edit them if they wanted to use a different namespace? (e.g. jaeger namespace)

objectiser

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, objectiser (Gary Brown) wrote…

The deployment files are copied when used in the Istio helm chart, so its not a problem there. This is about what we should provide for users who are deploying directly from the files in this repo.

We could add a namespace to all of the files and then users would have to edit them if they wanted to use a different namespace? (e.g. jaeger namespace)

You are correct, the namespace must be provided. If the namespace is left out, then the operator can only be deployed to default namespace.

jpkrohling

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, objectiser (Gary Brown) wrote…

You are correct, the namespace must be provided. If the namespace is left out, then the operator can only be deployed to default namespace.

Yes, adding the namespace to everything with a default (observability) value is my preference, as it reduces the risk of something not working. Most people will be OK in following our basic instructions, few of them would want to deviate. For those cases, it's OK to expect them to customize the YAML files. We just need to make sure to test the alternative and tell them what needs to be done.

objectiser

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Yes, adding the namespace to everything with a default (observability) value is my preference, as it reduces the risk of something not working. Most people will be OK in following our basic instructions, few of them would want to deviate. For those cases, it's OK to expect them to customize the YAML files. We just need to make sure to test the alternative and tell them what needs to be done.

SGTM - will update files and instructions.

objectiser · 2019-02-07T16:18:44Z

@jpkrohling Should be ready for final review.

jpkrohling

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

a discussion (no related file):
Don't we need to change the files from deploy/olm-catalog as well (you might need a rebase to see that directory)?

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2019-02-08T11:37:15Z

@jpkrohling Removed the namespace from the CRD as not necessary - changed to WIP as need to check OLM.

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2019-02-08T15:18:28Z

deploy/olm-catalog/jaeger-operator.csv.yaml

@@ -98,19 +148,15 @@ spec:
              containers:
              - args:
                - start
-                - --platform=openshift


@jpkrohling I ran the command as described here - just wondering if the removal of the --platform=openshift is right?

cc @awgreene

Doesn't look correct. OLM is, AFAIK, OpenShift-specific, so, it requires the --platform=openshift.

Don't think OLM is supposed to be OpenShift specific - question is why it automatically removes this arg, but leaves the start argument? Maybe it is just using the operator.yaml.

Might be worth addressing this by checking for Route or other OpenShift specific resource to determine platform, to avoid separate deploy file?

Hello @jpkrohling and @objectiser

OLM and Marketplace will be able to run on vanilla Kubernetes. The Operator-sdk gen will use the operator.yaml by default. Ideally, there would be a single deployment file called operator.yaml and checks for the platform would be done within the running operator.

@awgreene Ok thanks.

@jpkrohling So should we just merge this PR and work on detecting the platform automatically in a separate PR?

jpkrohling approved these changes Feb 7, 2019

View reviewed changes

objectiser commented Feb 7, 2019

View reviewed changes

jpkrohling approved these changes Feb 7, 2019

View reviewed changes

objectiser commented Feb 7, 2019

View reviewed changes

jpkrohling approved these changes Feb 7, 2019

View reviewed changes

objectiser commented Feb 7, 2019

View reviewed changes

objectiser changed the title ~~WIP: Enable single operator to monitor all namespaces~~ Enable single operator to monitor all namespaces Feb 7, 2019

jpkrohling reviewed Feb 8, 2019

View reviewed changes

objectiser added 3 commits February 8, 2019 10:13

Enable single operator to monitor all namespaces

bf0c796

Signed-off-by: Gary Brown <gary@brownuk.com>

Update to use observability namespace by default

2201049

Signed-off-by: Gary Brown <gary@brownuk.com>

Namespace not required in crd

f061f86

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser changed the title ~~Enable single operator to monitor all namespaces~~ WIP: Enable single operator to monitor all namespaces Feb 8, 2019

Ran the operator-sdk olm-catalog gen-csv --csv-version 1.9.0 command

a89f76c

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser changed the title ~~WIP: Enable single operator to monitor all namespaces~~ Enable single operator to monitor all namespaces Feb 8, 2019

objectiser commented Feb 8, 2019

View reviewed changes

objectiser merged commit fab8f32 into jaegertracing:master Feb 11, 2019

objectiser deleted the allnamespace branch February 11, 2019 09:22

jpkrohling mentioned this pull request Feb 11, 2019

Cloned resources to run the operator on its own namespace for tests #205

Merged

danielmbarlow mentioned this pull request Aug 13, 2019

[stable/jaeger-operator] Chart needs updating to use RoleBinding instead of ClusteRoleBinding helm/charts#16269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable single operator to monitor all namespaces #188

Enable single operator to monitor all namespaces #188

objectiser commented Jan 31, 2019

jpkrohling commented Jan 31, 2019

codecov bot commented Jan 31, 2019 •

edited

Loading

jpkrohling left a comment

objectiser left a comment

jpkrohling left a comment

objectiser left a comment

objectiser left a comment

jpkrohling left a comment

objectiser left a comment

objectiser commented Feb 7, 2019

jpkrohling left a comment

objectiser commented Feb 8, 2019

objectiser Feb 8, 2019

jpkrohling Feb 8, 2019

objectiser Feb 8, 2019 •

edited

Loading

awgreene Feb 8, 2019 •

edited

Loading

objectiser Feb 8, 2019

jpkrohling Feb 11, 2019

Enable single operator to monitor all namespaces #188

Enable single operator to monitor all namespaces #188

Conversation

objectiser commented Jan 31, 2019

jpkrohling commented Jan 31, 2019

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

objectiser commented Feb 7, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

objectiser commented Feb 8, 2019

objectiser Feb 8, 2019

Choose a reason for hiding this comment

jpkrohling Feb 8, 2019

Choose a reason for hiding this comment

objectiser Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

awgreene Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

objectiser Feb 8, 2019

Choose a reason for hiding this comment

jpkrohling Feb 11, 2019

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2019 •

edited

Loading

objectiser Feb 8, 2019 •

edited

Loading

awgreene Feb 8, 2019 •

edited

Loading