search

jvm-operators / abstract-operator

Library/SDK for creating the operators for Kubernetes and Openshift.
Apache License 2.0
60 stars 17 forks source link

Adding namespace to the resources #39

Closed blublinsky closed 5 years ago

blublinsky commented 5 years ago

Describe the problem:

Steps to reproduce:

  1. Add namespace to EntityInfo class
  2. Create resources
  3. Log desired resources during reconciliation

Observed Results:

Expected Results:

jkremser commented 5 years ago

Namespace should be inferred automatically from the context. If user creates custom resource of CRD "type" in namespace X and provided the operator watches for event in this namespace, it should get notified.

One can set the environment variable WATCH_NAMESPACE to comma separated list of namespaces in which the operator will watch for events (it basically starts n those watchers where each watcher will have the namespace private field correctly set).

other allowed values for WATCH_NAMESPACE are * and ~.

~ ... run the watcher in the same namespace where the operator is deployed * ... watch for the events in all the available namespaces

The case with * is little bit tricky. It doesn't start n threads/watchers for each exisiting namespace (it would miss the newly created namespaces and would be inefficient). Instead, it runs just one watcher that does some magic under the covers.

For the * case the operator's service account requires ClusterRole that would allow the cross-namespace "watching".

If the value of WATCH_NAMESPACE is empty, it behaves as in ~-case.

Could you please post here your yaml file that you use for deploying the operator? Specifically, what's the value of WATCH_NAMESPACE in your case.

blublinsky commented 5 years ago

Thanks Jirka, I figured all this. What i was trying to do is to get the namespace value of the resourse during reconsiliation. Trying to implement reconsiliation for * that you are skipping iñ spark

Get Outlook for Androidhttps://aka.ms/ghei36

From: Jirka Kremser notifications@github.com Sent: Monday, March 25, 2019 7:10:58 AM To: jvm-operators/abstract-operator Cc: Boris Lublinsky; Author Subject: Re: [jvm-operators/abstract-operator] Adding namespace to the resources (#39)

Namespace should be inferred automatically from the context. If user creates custom resource of CRD "type" in namespace X and provided the operator watches for event in this namespace, it should get notified.

One can set the environment variable WATCH_NAMESPACE to comma separated list of namespaces in which the operator will watch for events (it basically starts n those watchers where each watcher will have the namespace private fieldhttps://github.com/jvm-operators/abstract-operator/blob/62937b4e9f9a399574c8295da18963df26426385/src/main/java/io/radanalytics/operator/common/AbstractWatcher.java#L35 correctly set).

other allowed values for WATCH_NAMESPACE are * and ~.

~ ... run the watcher in the same namespace where the operator is deployed

The case with * is little bit tricky. It doesn't start n threads/watchers for each exisiting namespace (it would miss the newly created namespaces and would be inefficient). Instead, it runs just onehttps://github.com/jvm-operators/abstract-operator/blob/62937b4e9f9a399574c8295da18963df26426385/src/main/java/io/radanalytics/operator/common/AbstractWatcher.java#L127 watcher that does some magichttps://github.com/jvm-operators/abstract-operator/blob/62937b4e9f9a399574c8295da18963df26426385/src/main/java/io/radanalytics/operator/common/AbstractOperator.java#L163:L174 under the covers.

For the * case the operator's service account requires ClusterRole that would allow the cross-namespace "watching".

If the value of WATCH_NAMESPACE is empty, it behaveshttps://github.com/jvm-operators/abstract-operator/blob/62937b4e9f9a399574c8295da18963df26426385/src/main/java/io/radanalytics/operator/common/OperatorConfig.java#L79 as in ~-case.

Could you please post here your yaml file that you use for deploying the operator? Specifically, what's the value of WATCH_NAMESPACE in your case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/jvm-operators/abstract-operator/issues/39#issuecomment-476169276, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYHWG9cZHQ4CbiKc-r9X8iHJiFr2UU2aks5vaLzSgaJpZM4cFRWR.

jkremser commented 5 years ago

I see. There is that getDesiredSet method that can help with the full reconciliation, however the entities wont have the actual namespace on them and it wasn't actually designed for the "*". You can always fall back to low-lvl operations using the fabric8 client in your full reconciliation method and maintain some hash map of your entities (including the namespace). Drawback is that running an expensive full reconciliation method (where you query multiple namespaces, etc.) can have negative performance impact, but it'c certainly doable.

If you think the getDesiredSet can be done better for the "*-case", feel free to open PR. Help is always welcome.

blublinsky commented 5 years ago

Jirka, I created PR, but I do not have a permission to add it. Basically I have modified 2 classes:

I tested this and it works great. With this in place, I can easily implement reconciliation for the namespace “*” cause I can get not only the name, but namespace as well

Let me know if this is enough or you need something else

Boris Lublinsky FDP Architect boris.lublinsky@lightbend.com https://www.lightbend.com/

On Mar 25, 2019, at 7:19 AM, Jirka Kremser notifications@github.com wrote:

I see. There is that getDesiredSet https://github.com/jvm-operators/abstract-operator/blob/master/src/main/java/io/radanalytics/operator/common/AbstractOperator.java#L348 method that can help with the full reconciliation, however the entities wont have the actual namespace on them and it wasn't actually designed for the "*". You can always fall back to low-lvl operations using the fabric8 client in your full reconciliation method and maintain some hash map of your entities (including the namespace). Drawback is that running an expensive full reconciliation method (where you query multiple namespaces, etc.) can have negative performance impact, but it'c certainly doable.

If you think the getDesiredSet can be done better for the "*-case", feel free to open PR. Help is always welcome.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jvm-operators/abstract-operator/issues/39#issuecomment-476217281, or mute the thread https://github.com/notifications/unsubscribe-auth/AYHWG297NOLBu5jVCncFGLbyi8LyRm5eks5vaNrfgaJpZM4cFRWR.

jkremser commented 5 years ago

Jirka, I created PR, but I do not have a permission to add it.

I can't see the PR, you should have permissions to open a pull request. You need to fork the repository first, push the commits to your forked version and then open the PR against this repo (https://help.github.com/en/articles/creating-a-pull-request-from-a-fork)

... or post a patch here in the comment and I can test it and possibly apply

blublinsky commented 5 years ago

Here are the 2 changes that I had to make

Archive.zip

blublinsky commented 5 years ago

Jiri, Any progress on this?

jkremser commented 5 years ago

well, it would be much faster, if you just opened the pull request :) let me look into this..

jkremser commented 5 years ago

ok, so the Archive.zip ^ contains two files and they are the exact copies of what is currently in this repository, check:

Seriously, if you want the change to get in, either fist consult this and then open the PR or send only the patch/diff. Because even if the zip archive had the right files with your change, I'd have to create the PR from it myself.

blublinsky commented 5 years ago

Oh, I am sorry, wrong file Archive.zip

blublinsky commented 5 years ago

And I am sorry again, here is the right one Archive.zip In the EntityInfo, namespace is added and in the custom resource watcher, convert method is updated to get namespace from CRD and map it to the resource. Both are tested. Sorry about confusion

jkremser commented 5 years ago

Thanks for the change, the PR - https://github.com/jvm-operators/abstract-operator/pull/41 You will find your changes in the next release.