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

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

Closed
Pjack opened this issue Mar 23, 2024 · 1 comment
Closed

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

Pjack opened this issue Mar 23, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@Pjack
Copy link

Pjack commented Mar 23, 2024

          In the end of the day what is considered to have the exporter installed?

In my understanding reading the code is:

  • exist a file at /etc/systemd/system/hardware-exporter/.service and /etc/hardware-exporter-config.yaml

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:

def install(
self, port: str, level: str, redfish_conn_params: dict, collect_timeout: int
) -> bool:
"""Install the exporter."""
logger.info("Installing %s.", EXPORTER_NAME)
success = self.template.render_config(
port=port,
level=level,
redfish_conn_params=redfish_conn_params,
collect_timeout=collect_timeout,
)
success = self.template.render_service(str(self.charm_dir), str(EXPORTER_CONFIG_PATH))
if not success:
logger.error("Failed to install %s.", EXPORTER_NAME)
return success
systemd.daemon_reload()
logger.info("%s installed.", EXPORTER_NAME)
return success

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 #192 (comment)

@dashmage
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants