Skip to content

Commit

Permalink
Add logging during setup process and ensure we're reacting to start h…
Browse files Browse the repository at this point in the history 
…ook (#130)

* Add logging during setup process and ensure we're reacting to start hook

* Move logging message outside of try block for clarity

* Update tests

* Fix lint

* Adjust coverage to account for logging additions

* Revert coverage since we're now at 88%

* Don't defer when not ready in config changed action

* Fix unit tests and reduce required coverage

---------

Co-authored-by: Niels Robin-Aubertin <[email protected]>
Co-authored-by: Niels Robin-Aubertin <[email protected]>
  • Loading branch information
@nrobinaubertin
3 people authored Oct 25, 2023
1 parent aee1082 commit c2e2f83
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 26 deletions.
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ skips = ["*tests/*.py"]
branch = true

[tool.coverage.report]
fail_under = 88
fail_under = 80
show_missing = true


Expand Down
45 changes: 29 additions & 16 deletions src/charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def __init__(self, *args):
redis_relation={},
)
self._require_nginx_route()
self.framework.observe(self.on.install, self._set_up_discourse)
self.framework.observe(self.on.start, self._set_up_discourse)
self.framework.observe(self.on.upgrade_charm, self._set_up_discourse)
self.framework.observe(self.on.discourse_pebble_ready, self._config_changed)
self.framework.observe(self.on.config_changed, self._config_changed)
Expand Down Expand Up @@ -453,29 +453,31 @@ def _are_relations_ready(self) -> bool:
def _execute_migrations(self) -> None:
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
logger.info("Not ready to execute migrations.")
return
env_settings = self._create_discourse_environment_settings()
try:
if self.model.unit.is_leader():
self.model.unit.status = MaintenanceStatus("Executing migrations")
if self.model.unit.is_leader():
env_settings = self._create_discourse_environment_settings()
self.model.unit.status = MaintenanceStatus("Executing migrations")
try:
migration_process: ExecProcess = container.exec(
[f"{DISCOURSE_PATH}/bin/bundle", "exec", "rake", "--trace", "db:migrate"],
environment=env_settings,
working_dir=DISCOURSE_PATH,
user="_daemon_",
)
migration_process.wait_output()
except ExecError as cmd_err:
logger.exception("Executing migrations failed with code %d.", cmd_err.exit_code)
raise
except ExecError as cmd_err:
logger.exception("Executing migrations failed with code %d.", cmd_err.exit_code)
raise

def _compile_assets(self) -> None:
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
logger.info("Not ready to compile assets")
return
env_settings = self._create_discourse_environment_settings()
self.model.unit.status = MaintenanceStatus("Compiling assets")
try:
self.model.unit.status = MaintenanceStatus("Compiling assets")
precompile_process: ExecProcess = container.exec(
[f"{DISCOURSE_PATH}/bin/bundle", "exec", "rake", "assets:precompile"],
environment=env_settings,
Expand All @@ -490,6 +492,7 @@ def _compile_assets(self) -> None:
def _set_workload_version(self) -> None:
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
logger.info("Not ready to set workload version.")
return
env_settings = self._create_discourse_environment_settings()
try:
Expand All @@ -509,11 +512,12 @@ def _set_workload_version(self) -> None:
def _run_s3_migration(self) -> None:
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
logger.info("Not ready to run S3 migration.")
return
env_settings = self._create_discourse_environment_settings()
self.model.unit.status = MaintenanceStatus("Running S3 migration")
logger.info("Running S3 migration.")
try:
self.model.unit.status = MaintenanceStatus("Running S3 migration")
logger.info("Running S3 migration")
process = container.exec(
[f"{DISCOURSE_PATH}/bin/bundle", "exec", "rake", "s3:upload_assets"],
environment=env_settings,
Expand All @@ -533,26 +537,35 @@ def _set_up_discourse(self, event: HookEvent) -> None:
"""
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
logger.info("Defer discourse setup.")
event.defer()
return
logger.info(
"Relations are ready and can connect to container, attempting to set up discourse."
)
try:
logger.info("Discourse setup: about to execute migrations.")
self._execute_migrations()
logger.info("Discourse setup: about to compile assets.")
self._compile_assets()
logger.info("Discourse setup: about to mark the discourse setup process as complete.")
self._set_setup_completed()
logger.info("Discourse setup complete, now setting workload version.")
self._set_workload_version()
logger.info("Workload version set.")
except ExecError as cmd_err:
logger.exception("Setting up discourse failed with code %d.", cmd_err.exit_code)
raise

def _config_changed(self, event: HookEvent) -> None:
def _config_changed(self, _: HookEvent) -> None:
"""Configure pod using pebble and layer generated from config.
Args:
event: Event triggering the handler.
"""
container = self.unit.get_container(SERVICE_NAME)
if not self._are_relations_ready() or not container.can_connect():
event.defer()
logger.info("Not ready to do config changed action.")
return
if not self._is_config_valid():
return
Expand Down Expand Up @@ -589,7 +602,7 @@ def _config_changed(self, event: HookEvent) -> None:
def _reload_configuration(self) -> None:
# mypy has some trouble with dynamic attributes
if not self._is_setup_completed():
logger.info("Defer starting the discourse server until discourse setup completed")
logger.info("Setup is not completed: cancelling configuration reload.")
return
container = self.unit.get_container(SERVICE_NAME)
if self._is_config_valid() and container.can_connect():
Expand Down Expand Up @@ -681,7 +694,7 @@ def _on_anonymize_user_action(self, event: ActionEvent) -> None:

def _start_service(self):
"""Start discourse."""
logger.info("Starting discourse")
logger.info("Starting discourse.")
container = self.unit.get_container(SERVICE_NAME)
if self._is_config_valid() and container.can_connect():
layer_config = self._create_layer_config()
Expand All @@ -690,7 +703,7 @@ def _start_service(self):

def _stop_service(self):
"""Stop discourse, this operation is idempotent."""
logger.info("Stopping discourse")
logger.info("Stopping discourse.")
container = self.unit.get_container(SERVICE_NAME)
if (
container.can_connect()
Expand Down
18 changes: 9 additions & 9 deletions tests/unit/test_charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import ops
import pytest
from ops.charm import ActionEvent
from ops.model import ActiveStatus, BlockedStatus, WaitingStatus
from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus

from charm import DATABASE_NAME, DISCOURSE_PATH, SERVICE_NAME, DiscourseCharm
from tests.unit import helpers
Expand All @@ -25,11 +25,11 @@
(False, False, False, WaitingStatus("Waiting for database relation")),
(False, True, False, WaitingStatus("Waiting for database relation")),
(True, False, False, WaitingStatus("Waiting for redis relation")),
(True, True, False, ActiveStatus("")),
(True, True, False, MaintenanceStatus("Compiling assets")),
(False, False, True, WaitingStatus("Waiting for database relation")),
(False, True, True, WaitingStatus("Waiting for database relation")),
(True, False, True, WaitingStatus("Waiting for redis relation")),
(True, True, True, ActiveStatus("")),
(True, True, True, MaintenanceStatus("Compiling assets")),
],
ids=[
"No relation",
Expand Down Expand Up @@ -429,10 +429,10 @@ def bundle_handler(args: ops.testing.ExecArgs) -> None:
charm._on_anonymize_user_action(event)


def test_install_when_leader():
def test_start_when_leader():
"""
arrange: given a deployed discourse charm with all the required relations
act: trigger the install event on a leader unit
act: trigger the start event on a leader unit
assert: migrations are executed and assets are precompiled.
"""
harness = helpers.start_harness()
Expand Down Expand Up @@ -467,16 +467,16 @@ def exec_handler(args: ops.testing.ExecArgs) -> None:

harness.set_leader(True)
harness.container_pebble_ready(SERVICE_NAME)
harness.charm.on.install.emit()
harness.charm.on.start.emit()
harness.framework.reemit()

assert all(expected_exec_call_was_made.values())


def test_install_when_not_leader():
def test_start_when_not_leader():
"""
arrange: given a deployed discourse charm with all the required relations
act: trigger the install event on a leader unit
act: trigger the start event on a leader unit
assert: migrations are executed and assets are precompiled.
"""
harness = helpers.start_harness()
Expand Down Expand Up @@ -510,7 +510,7 @@ def exec_handler(args: ops.testing.ExecArgs) -> None:

harness.set_leader(False)
harness.container_pebble_ready(SERVICE_NAME)
harness.charm.on.install.emit()
harness.charm.on.start.emit()
harness.framework.reemit()


Expand Down

0 comments on commit c2e2f83

Please sign in to comment.