search

canonical / hardware-observer-operator

A charm to setup prometheus exporter for IPMI, RedFish and RAID devices from different vendors.
Apache License 2.0
7 stars 14 forks source link

When render config fail, we may not aware it. #196

Closed Pjack closed 2 months ago

Pjack commented 4 months ago 
          In the end of the day what is considered to have the exporter installed?

In my understanding reading the code is:

This is very easy to check and not CPU intensive by using Path.exists and I don't see the need at all of using self._stored and by consequence those type ignore.

The juju sdk documentation has an interesting section of uses and limitations with StoredState and the problematic solution looks like a lot with what I'm seeing in this project.

I've spotted another possible issue into the install function:

https://github.com/canonical/hardware-observer-operator/blob/86f0d7e0dd8b0339740518b27eb224fb1fcaf2b9/src/service.py#L132-L149

See that if line 137 fails, it will overwrite the result on line 143 meaning that if for some reason the redfish config file fails, but for the service doesn't, you won't notice.

To say the truth I' would refactor those functions of install and uninstall to return nothing and let to bubble in the charm code here

_Originally posted by @gabrielcocenza in https://github.com/canonical/hardware-observer-operator/pull/192#discussion_r1536122008_

dashmage commented 2 months ago

As part of this PR to add smartctl exporter support for the charm, we are removing the usage of Stored State to check whether an exporter is installed.

In the same PR, we're also using separate variables (render_config_success, render_service_success) and so we won't face this issue in the exporter install function. Refer here.