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

Strongly tie a observation to a server endpoint #1656

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

sbernard31
Copy link
Contributor

@sbernard31 sbernard31 commented Oct 11, 2024

A registration is now tied to server endpoint. (see #1655)
Then observation should also be tied to 1 server endpoint.

This will avoid Observation Token conflict between different server endpoint. (currently a token should be unique by Leshan Server with this modification a Observation Token should be unique by server endpoint)

So concretely that mean that Observation Identifier has changed.
Before it was just a CoAP token, now this is the (CoAP token + server endpoint Uri used).
Unfortunately this will break RedisRegistrationStore data compatibility. (see #1418 (comment))

@sbernard31 sbernard31 changed the base branch from master to endpoint_registration October 11, 2024 16:07
@sbernard31 sbernard31 force-pushed the endpoint_registration branch from bdfcd4b to 7f870ef Compare October 17, 2024 09:25
@sbernard31 sbernard31 force-pushed the endpoint_observation branch from 9baa80f to 8368971 Compare October 17, 2024 09:26
@sbernard31 sbernard31 marked this pull request as ready for review October 17, 2024 09:28
@sbernard31 sbernard31 force-pushed the endpoint_observation branch from 8368971 to 15786eb Compare October 17, 2024 16:06
@sbernard31
Copy link
Contributor Author

@JaroslawLegierski , @cyril2maq, Oops I did it again. I break the RedisRegistrationStore Data compatibility.

I let you know that as soon as possible, so we can try to anticipate issue about it.

Maybe better to read this comment : #1415 (comment) too.

Let discuss about that here.

@JaroslawLegierski
Copy link
Contributor

@JaroslawLegierski , @cyril2maq, Oops I did it again. I break the RedisRegistrationStore Data compatibility.

I let you know that as soon as possible, so we can try to anticipate issue about it.

Maybe better to read this comment : #1415 (comment) too.

Let discuss about that here.

I tested this change today and as I understand it, the change in Redis is the addition the field e.g. "epUri": "coaps://0.0.0.0:5684", in JSON presented below for the key "OBS#TKN#...." responsible for observation for partitcular LwM2M client

		{
			"id": "d53329b026af5dd2",
			"epUri": "coaps://0.0.0.0:5684",
			"regid": "mIqhc2qKmg",
			"userContext": {},
			"protocolData": {
				"leshan-cf-obs": {
					"request": "48018072d53329b026af5dd260543333303301300435373030622d17",
					"peer": {
						"address": "127.0.0.1",
						"port": 50926,
						"id": "leshan_1",
						"attributes": {
							"*DTLS_HANDSHAKE_MODE": "auto"
						}
					},
					"context": {
						"leshan-endpoint": "leshan1",
						"leshan-path": "/3303/0/5700n",
						"leshan-regId": "mIqhc2qKmg}}"
					},
					"kind": "single",
					"ct": 11543,
					"path": "/3303/0/5700"
				}
			}
		}

So the change only affects leshan clients with observation. Am I right?

@sbernard31
Copy link
Contributor Author

This is a bit more than that : see #1418 (comment)

So the change only affects leshan clients with observation. Am I right?

Those keys are only about observations.
But I don't know what happens if you are using an old registration using previous key with the new code in case where you don't use observations (I didn't test it)

@JaroslawLegierski
Copy link
Contributor

JaroslawLegierski commented Oct 29, 2024

This is a bit more than that : see #1418 (comment)

So the change only affects leshan clients with observation. Am I right?

Those keys are only about observations. But I don't know what happens if you are using an old registration using previous key with the new code in case where you don't use observations (I didn't test it)

I tested the code change on the server side. After retransmissions (we lost InMemoryReadWriteLockConnectionStore in server after server replace), the LwM2M client (Leshan client demo) initiated a new handshake and finally successfully completed the registration update.
leshan_server_code_change

@JaroslawLegierski
Copy link
Contributor

This is a bit more than that : see #1418 (comment)

So the change only affects leshan clients with observation. Am I right?

Those keys are only about observations. But I don't know what happens if you are using an old registration using previous key with the new code in case where you don't use observations (I didn't test it)

I noticed that the secondary index "OBSIDS#REGID#.... in my Redis has always the value null

127.0.0.1:6379> keys *
1) "REG#EP#leshan1"
2) "EP#ADDR#/127.0.0.1:58045"
3) "OBSIDS#REGID#hi8A1xB0n5"
4) "OBS#OBSID#coaps://0.0.0.0:5684##\xcd\xd4z\xc4\xd70\x8c#"
5) "EP#REGID#hi8A1xB0n5"
6) "SEC#EP#leshan1"
7) "EXP#EP"
8) "EP#PSKID"
9) "EP#IDENTITY#{\"type\":\"psk\",\"pskid\":\"leshan_1\"}"
127.0.0.1:6379> mget "OBSIDS#REGID#hi8A1xB0n5"
1) (nil)

Are you also getting this error on your side? (I don't know if the problem is in my Redis or if the lpush method in RedisRegistrationStore is not working properly)

@sbernard31
Copy link
Contributor Author

OBSIDS#REGID#*has a list value. I don't know if MGET command works on it.
In Leshan code we use LRANGE.

@JaroslawLegierski
Copy link
Contributor

JaroslawLegierski commented Oct 31, 2024

OBSIDS#REGID#*has a list value. I don't know if MGET command works on it. In Leshan code we use LRANGE.

Sure I completely forgot that this is list of values. LRAGE reads it. Thx!

@sbernard31 sbernard31 force-pushed the endpoint_registration branch from 7f870ef to c0cc705 Compare November 27, 2024 15:07
@sbernard31 sbernard31 changed the base branch from endpoint_registration to master November 27, 2024 15:42
@sbernard31 sbernard31 merged commit 14fc20e into master Nov 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants