Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Iot device registry backed with infinispan #2498

Merged

Conversation

jbtrystram
Copy link
Contributor

I am not sure exatcly how enmasse modules works so here is a draft PR so I can get feedback on this :)

@jbtrystram
Copy link
Contributor Author

@lulf @kornys I changed the tests to use Junit but now I can't get any tests to run no matter how I change the pom file. Maybe it is related to the fact that the tests are extending abstract tests classes in hono, which are built with Junit4 (e.g. https://github.com/eclipse/hono/blob/master/service-base/src/test/java/org/eclipse/hono/service/registration/AbstractCompleteRegistrationServiceTest.java)
What do you think?

@kornys
Copy link
Member

kornys commented Mar 26, 2019

@jbtrystram could you show me your pom with junit5 ?

@jbtrystram
Copy link
Contributor Author

@kornys it looks like [1] at the moment. But I've looked a bit deeper into it, it look like i would need to change all the assertions etc.. Maybe i can just leave the tests in junit4 and use the junit-vintage-engine ?
[1] : https://github.com/jbtrystram/enmasse/blob/infinispan-device-registry/iot/iot-infinispan-device-registry/pom.xml

@kornys
Copy link
Member

kornys commented Mar 26, 2019

@jbtrystram yes you need to change all assertions, test annotation etc to jupiter (junit5), personally I don't want to have vintage module here, junit4 is dead so we should use only junit5.

@lulf
Copy link
Member

lulf commented Mar 27, 2019

@jbtrystram @kornys I think junit4 is fine for now, as there are hono dependencies that use junit4. Longer term, we'll try to get Hono up to junit5 which will resolve the issue.

@kornys
Copy link
Member

kornys commented Mar 29, 2019

@lulf what I don’t personally like is that when we crate a new test it will offers junit 4 tag in idea which is not supported by junit5

@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch 3 times, most recently from cfe237d to 96a542d Compare April 10, 2019 12:41
@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch 2 times, most recently from e9c36e0 to 7b317d1 Compare April 12, 2019 12:04
@jbtrystram jbtrystram changed the title Early preview to get infinispan code in enmasse Iot device registry backed with infinispan Apr 18, 2019
@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch from 7b317d1 to 2fc7a71 Compare April 26, 2019 13:33
@jbtrystram jbtrystram marked this pull request as ready for review April 29, 2019 22:11
@jbtrystram jbtrystram assigned dejanb and ctron and unassigned dejanb and ctron Apr 29, 2019
@jbtrystram jbtrystram requested review from ctron and dejanb April 29, 2019 22:12
@jbtrystram
Copy link
Contributor Author

@lulf @ctron @dejanb I think this is ready for review, as it works now.

Here is how you can test it :
clone the infinispan-operator repo.
then build the operator with make build. you can deploy it with an example CR with :

oc create configmap infinispan-app-configuration --from-file=./config                                                                           oc apply -f deploy/rbac.yaml
oc apply -f deploy/operator.yaml
oc apply -f deploy/crd.yaml

oc apply -f deploy/cr/cr_minimal.yaml

At this point if you build this branch you can deploy it following the @dejanb IoT guide steps. In the docs : "5. Internet of Things (IoT) guide"

Here are some highlights :

  • The infinispan server URL needs to be specified in the cr, see example
  • The iotconfig controller will fail if file and datagrid are set in the same cr.
  • used "infinispan" in the variables and image names through all the code, for consistency. I suppose we'll have "datagrid" only as a downstream deployment.
  • Unit tests on the java app are currently marked with @Ignore due to an infinispan bug

@ctron
Copy link
Contributor

ctron commented Apr 30, 2019

Looking at the deployment files I have a few questions:

  • It looks like there is an "rbac.yaml" file, do we need that as well?
  • The infinispan operator deployment doesn't look too complicated, neither does the CRD file, wouldn't it make sense to simply add that to our deployment?
  • My understanding is that the "cr_minimal" and the configmap belong to the instance of infinispan, which the infinispan operator creates. Wouldn't it make sense to let the enmasse-controller create that for the IoTConfig instance?

@jbtrystram
Copy link
Contributor Author

jbtrystram commented Apr 30, 2019

The infinispan operator deployment doesn't look too complicated, neither does the CRD file, wouldn't it make sense to simply add that to our deployment?

I would agree with you, as it seems straightforward enough. We would have to maintain / keep those files updated though. If we do it we can probably remove the rbac file, as we'll probably match the roles we already use in enmasse.

My understanding is that the "cr_minimal" and the configmap belong to the instance of infinispan, which the infinispan operator creates. Wouldn't it make sense to let the enmasse-controller create that for the IoTConfig instance?

would we be able to manage the infnispan cluster then ? I'm thinking things like scale down / up..

@ctron
Copy link
Contributor

ctron commented Apr 30, 2019

would we be able to manage the infnispan cluster then ? I'm thinking things like scale down / up..

There are two ways to achieve that:

  1. The operator only creates those resources, and restores them should anyone delete them. But it would not overwrite the content if someone else makes changes. This is currently be done for example with the logback configuration map entries of the different Hono components. They are created, restored, but not updated.
  2. We think if what is necessary, and add that to the infinispan/datagrid specific structure, using that to generate the configuration as we require it. The number of instances could by part of that, so we would simply pass it along to the next CR.

And the truth lies in the middle I guess ... having a good balance between enforcing configurations and leaving it alone, not overwriting changes by the admin.

I think in the beginning we don't have much to offer in specialized behavior, so I would suggest to focus on 1. and add specialized fields in the structure when we start to see a need.

@jbtrystram
Copy link
Contributor Author

So what you suggest is we spin up the infinispan operator, generate a cr according to our need and let the infinispan operaor spins up the services. Am I correct ?

Also -> No need for junit4 anymore when eclipse-hono/hono#1195 is merged in hono

@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch from 9343e9b to 7eccb7d Compare May 2, 2019 09:02
lulf
lulf previously approved these changes May 2, 2019
Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jbtrystram !

@jbtrystram
Copy link
Contributor Author

jbtrystram commented May 2, 2019

@lulf @ctron @dejanb There are still a few steps needed before merging I think :

  • figure out how we'll deploy the infnispan cluster
  • Run some user-stories tests against this registry

(and there are still a few things I need to clean up)

@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch from 10c1fc5 to 3ffea6a Compare May 2, 2019 10:01
jbtrystram and others added 5 commits May 7, 2019 11:31
- Updated iot controller to deploy the infinspan image when specified in the iotconfig CR
- Relies on a external / manually deployed infinispan instance.
Signed-off-by: Trystram Jean-Baptiste <[email protected]>
Signed-off-by: Trystram Jean-Baptiste <[email protected]>
@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch from 73c6b10 to a4c1c5d Compare May 7, 2019 09:41
@jbtrystram
Copy link
Contributor Author

jbtrystram commented May 7, 2019

FMPV it's ready to be merge, if we assume the user must deploy a infinispan endpoint on his own, for now. Let's do that for the beginning, so we can progress further toward asserting it works well to our needs.

@ctron @dejanb what do you think ? we can update the documentation to point the user to the infnispan/datagrid operator.

@famartinrh @kornys About the tests :

  • unit tests are still disabled as a bug in infinispan is still unresolved.
  • Jupiter tests are ready, we just need to wait for the next hono release.
  • we modified the tests so it spins up an infinispan server container before running the tests. How do we make sure both file-based and infinispan device registry are covered by the systemtests ?

@famarting
Copy link
Contributor

@jbtrystram about having coverage of infinispan-based and file-based device-registry I'll do some slight modifications to DeviceRegistryTest before or after this PR is merged

@jbtrystram jbtrystram force-pushed the infinispan-device-registry branch from 7936a9a to 37b2a2e Compare May 8, 2019 07:42
@dejanb
Copy link
Contributor

dejanb commented May 8, 2019

Great work @jbtrystram. I think we should go ahead and merge this and continue working on all things mentioned in comments

dejanb
dejanb previously approved these changes May 8, 2019
file:
numberOfDevicesPerTenant: 100000
infinispan:
infinispanServerAddress: example-infinispan:11222
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to extract this value from Infinispan service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment we assume the user will deploy infinispan on his own, hence putting the correct value in here. WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should remove this so the file-based registry stays the default ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah ... And create a new example config for infinispan

@jbtrystram
Copy link
Contributor Author

Great work @jbtrystram. I think we should go ahead and merge this and continue working on all things mentioned in comments

thanks :) Next thing is to document it I suppose

@famarting
Copy link
Contributor

@enmasse-ci run tests profile=shared-iot

@dejanb
Copy link
Contributor

dejanb commented May 8, 2019

thanks :) Next thing is to document it I suppose

We can add basic docs to the iot/examples/README.md while we still working on it. And then we'll move it to the docs, once it's finalized.

@jenmalloy
Copy link
Contributor

jenmalloy commented May 8, 2019

@jbtrystram please let me know if I can help when you're ready to move the "draft" docs to the actual docs

@jbtrystram
Copy link
Contributor Author

@jbtrystram please let me know if I can help when you're ready to move the "draft" docs to the actual docs

@jenmalloy OK. I'll probably do that in a separate PR

@EnMasseProject EnMasseProject deleted a comment from famarting May 8, 2019
@EnMasseProject EnMasseProject deleted a comment from k-wall May 8, 2019
@EnMasseProject EnMasseProject deleted a comment from k-wall May 8, 2019
@jbtrystram
Copy link
Contributor Author

@enmasse-ci run tests profile=shared-iot

@jbtrystram jbtrystram merged commit 1ee73cc into EnMasseProject:master May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants