vmware-archive / liota

Other
336 stars 120 forks source link

MQTT DCC comms fails with manually constructed MqttMessagingAttributes #143

Open bfrggit opened 7 years ago

bfrggit commented 7 years ago

When initializing MQTT DCC comms, I was trying to provide an MqttMessagingAttributes object I constructed myself, using

iotcc = IotControlCenter(MqttDccComms(
    edge_system_name=edge_system.name,
    url=config['BrokerIP'],
    port=config['BrokerPort'],
    identity=identity,
    tls_conf=tls_conf,
    enable_authentication=True,
    mqtt_msg_attr=MqttMessagingAttributes( # manually construct attribute object
        edge_system_name=edge_system.name,
        pub_qos=0
    )
))

Got this error:

Traceback (most recent call last):
  File "./mqtt_debug_plus_qos.py", line 183, in <module>
    pub_qos=0
  File "/usr/local/lib/python3.5/dist-packages/liota-0.3-py3.5.egg/liota/dcc_comms/mqtt_dcc_comms.py", line 113, in __init__
  File "/usr/local/lib/python3.5/dist-packages/liota-0.3-py3.5.egg/liota/dcc_comms/mqtt_dcc_comms.py", line 122, in _connect
  File "/usr/local/lib/python3.5/dist-packages/liota-0.3-py3.5.egg/liota/lib/transports/mqtt.py", line 177, in __init__
  File "/usr/local/lib/python3.5/dist-packages/paho/mqtt/client.py", line 491, in __init__
    raise ValueError('A client id must be provided if clean session is False.')
ValueError: A client id must be provided if clean session is False.
Venkat2811 commented 7 years ago

Yes. This is valid and has been intentionally left for paho mqtt library to raise error instead of raising at liota transports . If clean_session is False, a unique client_id must be provided.

Also in IoTCC, we use auto-generated topic and client_id. Please refer to README for various mqtt configurations available.

bfrggit commented 7 years ago

@Venkat2811 Well, my question may have been confusing, but I was just trying to set up a QoS level in MqttMessagingAttributes.

I do not understand why I am supposed to provide client ID when I try to specify messaging attributes, which is basically QoS, retain, etc.

Venkat2811 commented 7 years ago

@bfrggit You have to provide client_id if clean_session=False. By default, it is False in MqttDccComms's __init__(). You can either change it to True or provide your own client_id.

MqttDccComms is designed in such a way that, if you provide MqttMessagingAttributes as part of __init__(), client_id won't be auto-generated and clean_session is False by default. That's why you are getting this error.

Venkat2811 commented 7 years ago

For IoTCC, we are looking at QoS as 1 and auto-generated topics, client_id's. If you are looking at perf_benchmark with QoS as 0, there are two options.

1) Use different MQTT broker and publish stats to it. Please refer to this example

2) If IoTCC has to be tested then, in MqttDccComms's send(), change line 162 to:

self.client.publish(
msg_attr.pub_topic if msg_attr.pub_topic else self.msg_attr.pub_topic,
message,
msg_attr.pub_qos if msg_attr.pub_qos else self.msg_attr.pub_qos,
msg_attr.pub_retain if msg_attr.pub_retain else self.msg_attr.pub_retain
)

and pass QoS=0 in each metric's MessagingAttribute

bfrggit commented 7 years ago

In MqttDccComms.init starting Ln 77:

        if mqtt_msg_attr is None:
            #  pub-topic and sub-topic will be auto-generated
            log.info("pub-topic and sub-topic is auto-generated")
            self.msg_attr = MqttMessagingAttributes(edge_system_name)
            if self.client_id is None or self.client_id == "":
                #  local_uuid generated will be the client ID
                self.client_id = systemUUID().get_uuid(edge_system_name)
                log.info("generated local uuid will be the client ID")
            else:
                log.info("Client ID is provided by user")
            # Storing edge_system name and generated local_uuid which will be used in auto-generation of pub-sub topic
            store_edge_system_uuid(entity_name=edge_system_name,
                                   entity_id=self.client_id,
                                   reg_entity_id=None)
        elif isinstance(mqtt_msg_attr, MqttMessagingAttributes):
            log.info("User configured pub-topic and sub-topic")
            self.msg_attr = mqtt_msg_attr
        else:
            log.error("mqtt_mess_attr should either be None or of type MqttMessagingAttributes")
            raise TypeError("mqtt_mess_attr should either be None or of type MqttMessagingAttributes")

In init, there are two separate parameters, mqtt_msg_attr and client_id. What is the consideration that it does not generate client ID automatically, when it is not given, under the elif (mqtt_msg_attr is not None) section?

Venkat2811 commented 7 years ago

According to MQTT spec, if clean_session = False, client_id must be provided by the user.

So the consideration is: If clean_session is False user MUST provide client_id. Since user is providing mqtt_msg_attr he should also provide client_id. else clean_session should be changed to True

bfrggit commented 7 years ago

But now, those checks went under the condition switching for mqtt_msg_attr instead of that for clean_session. At the same time, nothing happens to check whether clean_session is set to True or False.

Does it mean that if I provide mqtt_msg_attr, clean_session should not be set to True? Why? I failed to understand why I cannot provide mqtt_msg_attr to start a clean session with auto generated client_id.

Now it is like:

If user does not specify mqtt_msg_attr:
    Auto generate mqtt_msg_attr
    If user does not specify client_id, either:
        Auto generate client_id
Else: # i.e. user specified mqtt_msg_attr:
    Set mqtt_msg_attr with the user specified object
    Never generate client_id

Nothing checks value of clean_session

Should not it be like: (?)

If user does not specify mqtt_msg_attr:
    Auto generate mqtt_msg_attr
Else:
    Set mqtt_msg_attr with the user specified object

If user set clean_session=True:
    If user does not specify client_id:
        Auto generate client_id
Venkat2811 commented 7 years ago

One correction in your question before I explain. It should be: ''' Does it mean that if I provide mqtt_msg_attr, clean_session should not be set to False? ''' We both are looking at this in a different perspective. local_uuid is used in IoTCC scenarios. For IoTCC we want clean_session=False. MQTT mandates client_id if clean_session=False. So, we are using local_uuid as client_id in this case. And for IoTCC scenarios, MqttMessagingAttributes() is not provided by the user.

So, if user provides mqtt_msg_attr and if he wants clean_session=False, then user should provide client_id.

However, I agree that there is no harm in using local_uuid as client_id for this case also, if user doesn't provide client_id of his own instead of raising an error.

bfrggit commented 7 years ago

Glad you finally got what I was talking about.

I think, It is supposed to use local_uuid as client_id for this case also, if user doesn't provide client_id of his own instead of raising an error.

MQTTDCCComms should be able to handle other cases than IoTCC itself. Its internal logic should not be limited to IoTCC use case.

Also, why in IoTCC scenarios, MqttMessagingAttributes is not provided by user? Any harm if user provides it? It sounds like we are providing some interface that cannot be used?

Venkat2811 commented 7 years ago

Agreed. We'll use local_uuid as client_id if user doesn't provide his own.

Few things are fixed in LIOTA - > MQTT Broker -> vROps interaction.

Moreover, this interface is provided considering non IoTCC usecases.