Some comments inline. Looks like there's a text conflict in the Dockerfile, fwiw.
Diff comments: > diff --git a/README.md b/README.md > index d6e4fa4..b2b4d3f 100644 > --- a/README.md > +++ b/README.md > @@ -19,11 +19,13 @@ details on using Juju with MicroK8s for easy local > testing [see here](https://ju > To deploy the charm and relate it to the [MariaDB K8s > charm](https://charmhub.io/mariadb) within a Juju > Kubernetes model: > > + juju deploy nginx-ingress-integrator ingress > juju deploy charmed-osm-mariadb-k8s mariadb > - juju deploy wordpress-k8s > + juju deploy wordpress-k8s --resource > wordpress-image=wordpresscharmers/wordpress:bionic-5.7 You shouldn't need the resource since you'll associate the resource with the charm version at release time. > juju relate wordpress-k8s mariadb:mysql > + juju relate wordpress-k8s ingress:ingress This should be able to just be "juju relate wordpress-k8s ingress" I think? > > -It will take about 5 to 10 minutes for Juju hooks to discover the site is > live > +It will take about 2 to 5 minutes for Juju hooks to discover the site is live Let's just say "It will take a few minutes" as this is going to vary based on local setup. > and perform the initial setup for you. Once the "Workload" status is > "active", > your WordPress site is configured. > > diff --git a/src/charm.py b/src/charm.py > index cba136a..0017686 100755 > --- a/src/charm.py > +++ b/src/charm.py > @@ -79,81 +34,383 @@ class WordpressInitialiseEvent(EventBase): > pass > > > +class WordpressStaticDatabaseChanged(EventBase): > + """Custom event for static Database configuration changed. > + > + WordpressStaticDatabaseChanged provides the same interface as the > + db.on.database_changed event which enables the WordPressCharm's > + on_database_changed handler to update state for both relation and static > + database configuration events. > + """ > + > + @property > + def database(self): > + return self.model.config["db_name"] > + > + @property > + def host(self): > + return self.model.config["db_host"] > + > + @property > + def user(self): > + return self.model.config["db_user"] > + > + @property > + def password(self): > + return self.model.config["db_password"] > + > + @property > + def model(self): > + return self.framework.model > + > + > class WordpressCharmEvents(CharmEvents): > """Register custom charm events. > > - WordpressCharmEvents registers the custom WordpressInitialiseEvent > - event to the charm. > + WordpressCharmEvents registers the custom WordpressFirstInstallEvent > + and WordpressStaticDatabaseChanged event to the charm. > """ > > - wordpress_initialise = EventSource(WordpressInitialiseEvent) > + wordpress_initial_setup = EventSource(WordpressFirstInstallEvent) > + wordpress_static_database_changed = > EventSource(WordpressStaticDatabaseChanged) > > > class WordpressCharm(CharmBase): > + > + _container_name = "wordpress" > + _default_service_port = 80 > + > state = StoredState() > - # Override the default list of event handlers with our > WordpressCharmEvents subclass. > on = WordpressCharmEvents() > > - def __init__(self, *args): > - super().__init__(*args) > + def __init__(self, *args, **kwargs): > + super().__init__(*args, **kwargs) > > self.leader_data = LeadershipSettings() > > - self.framework.observe(self.on.start, self.on_config_changed) > + logger.debug("registering framework handlers...") > + > + self.framework.observe(self.on.wordpress_pebble_ready, > self.on_config_changed) > self.framework.observe(self.on.config_changed, > self.on_config_changed) > - self.framework.observe(self.on.update_status, self.on_config_changed) > - self.framework.observe(self.on.wordpress_initialise, > self.on_wordpress_initialise) > + self.framework.observe(self.on.leader_elected, > self.on_leader_elected) > > # Actions. > self.framework.observe(self.on.get_initial_password_action, > self._on_get_initial_password_action) > > - self.db = MySQLClient(self, 'db') > + self.db = MySQLClient(self, "db") > self.framework.observe(self.on.db_relation_created, > self.on_db_relation_created) > self.framework.observe(self.on.db_relation_broken, > self.on_db_relation_broken) > - self.framework.observe(self.db.on.database_changed, > self.on_database_changed) > + > + # Handlers for if user supplies database connection details or a > charm relation. > + self.framework.observe(self.on.config_changed, > self.on_database_config_changed) > + for db_changed_handler in [self.db.on.database_changed, > self.on.wordpress_static_database_changed]: > + self.framework.observe(db_changed_handler, > self.on_database_changed) I think since you only have two items here, just listing them out is a bit easier to read than iterating through a for loop. > > c = self.model.config > self.state.set_default( > - initialised=False, valid=False, has_db_relation=False, > - db_host=c["db_host"], db_name=c["db_name"], > db_user=c["db_user"], db_password=c["db_password"] > + blog_hostname=c["blog_hostname"] or self.app.name, Since state is persisted, this means if it's unset in config this default value wouldn't be updated. I think it's better to have a separate property for this. > + installed_successfully=False, > + install_state=set(), > + has_db_relation=False, > + has_ingress_relation=False, > + db_host=c["db_host"] or None, > + db_name=c["db_name"] or None, > + db_user=c["db_user"] or None, > + db_password=c["db_password"] or None, > ) > + > self.wordpress = Wordpress(c) > > + self.ingress = IngressRequires(self, self.ingress_config) > + > + self.framework.observe(self.on.ingress_relation_changed, > self.on_ingress_relation_changed) > + self.framework.observe(self.on.ingress_relation_created, > self.on_ingress_relation_changed) > + > + # TODO: It would be nice if there was a way to unregister an > observer at runtime. > + # Once the site is installed there is no need for > self.on_wordpress_uninitialised to continue to observe > + # config-changed hooks. > + if self.state.installed_successfully is False: > + self.framework.observe(self.on.config_changed, > self.on_wordpress_uninitialised) > + self.framework.observe(self.on.wordpress_initial_setup, > self.on_wordpress_initial_setup) > + logger.debug("all observe hooks registered...") > + > + @property > + def container_name(self): > + return self._container_name > + > + @property > + def service_ip_address(self): > + return os.environ.get("WORDPRESS_SERVICE_SERVICE_HOST") > + > + @property > + def service_port(self): > + return self._default_service_port > + > + @property > + def wordpress_workload(self): > + """Returns the WordPress pebble workload configuration.""" > + return { > + "summary": "WordPress layer", > + "description": "pebble config layer for WordPress", > + "services": { > + "wordpress-plugins": { > + "override": "replace", > + "summary": "WordPress plugin updater", > + "command": ( > + "bash -c '/srv/wordpress-helpers/plugin_handler.py > && " > + "stat /srv/wordpress-helpers/.ready && " > + "sleep infinity'" > + ), > + "after": ["apache2"], > + "environment": self._env_config, > + }, > + "wordpress-init": { > + "override": "replace", > + "summary": "WordPress initialiser", > + "command": ( > + "bash -c '" > + "/charm/bin/wordpressInit.sh >> /wordpressInit.log > 2>&1" Is this needed any more, or would it be better to just let this go to the container output that pebble now provides? > + "'" > + ), > + "environment": self._env_config, > + }, > + "apache2": { > + "override": "replace", > + "summary": "Apache2 service", > + "command": ( > + "bash -c '" > + "apache2ctl -D FOREGROUND -E /apache-error.log -e > debug >>/apache-sout.log 2>&1" Same command as above about logging. Also, do we really want debug on this? > + "'" > + ), > + "requires": ["wordpress-init"], > + "after": ["wordpress-init"], > + "environment": self._env_config, > + }, > + self.container_name: { > + "override": "replace", > + "summary": "WordPress service", > + "command": "sleep infinity", > + "requires": ["apache2", "wordpress-plugins"], > + "environment": self._env_config, > + }, > + }, > + } > + > + @property > + def ingress_config(self): > + blog_hostname = self.state.blog_hostname > + ingress_config = { > + "service-hostname": blog_hostname, > + "service-name": self.app.name, > + "service-port": self.service_port, > + } > + tls_secret_name = self.model.config["tls_secret_name"] > + if tls_secret_name: > + ingress_config["tls-secret-name"] = tls_secret_name > + return ingress_config > + > + @property > + def _db_config(self): > + """Kubernetes Pod environment variables.""" > + # TODO: make this less fragile. > + if self.unit.is_leader(): > + return { > + "WORDPRESS_DB_HOST": self.state.db_host, > + "WORDPRESS_DB_NAME": self.state.db_name, > + "WORDPRESS_DB_USER": self.state.db_user, > + "WORDPRESS_DB_PASSWORD": self.state.db_password, > + } > + else: > + return { > + "WORDPRESS_DB_HOST": self.leader_data["db_host"], > + "WORDPRESS_DB_NAME": self.leader_data["db_name"], > + "WORDPRESS_DB_USER": self.leader_data["db_user"], > + "WORDPRESS_DB_PASSWORD": self.leader_data["db_password"], > + } > + > + @property > + def _env_config(self): > + """Kubernetes Pod environment variables.""" > + config = dict(self.model.config) > + env_config = {} > + if config["container_config"].strip(): > + env_config = safe_load(config["container_config"]) > + > + env_config["WORDPRESS_BLOG_HOSTNAME"] = self.state.blog_hostname > + initial_settings = {} > + if config["initial_settings"].strip(): > + initial_settings.update(safe_load(config["initial_settings"])) > + # TODO: make these class default attributes > + env_config["WORDPRESS_ADMIN_USER"] = > initial_settings.get("user_name", "admin") > + env_config["WORDPRESS_ADMIN_EMAIL"] = > initial_settings.get("admin_email", "nobody@localhost") > + > + env_config["WORDPRESS_INSTALLED"] = self.state.installed_successfully > + env_config.update(self._wordpress_secrets) > + > + if not config["tls_secret_name"]: > + env_config["WORDPRESS_TLS_DISABLED"] = "true" > + if config.get("wp_plugin_openid_team_map"): > + env_config["WP_PLUGIN_OPENID_TEAM_MAP"] = > config["wp_plugin_openid_team_map"] > + > + # Add secrets from charm config. > + if config.get("wp_plugin_akismet_key"): > + env_config["WP_PLUGIN_AKISMET_KEY"] = > config["wp_plugin_akismet_key"] > + if config.get("wp_plugin_openstack-objectstorage_config"): > + # Actual plugin name is 'openstack-objectstorage', but we're only > + # implementing the 'swift' portion of it. > + wp_plugin_swift_config = > safe_load(config.get("wp_plugin_openstack-objectstorage_config")) > + env_config["SWIFT_AUTH_URL"] = > wp_plugin_swift_config.get("auth-url") > + env_config["SWIFT_BUCKET"] = wp_plugin_swift_config.get("bucket") > + env_config["SWIFT_PASSWORD"] = > wp_plugin_swift_config.get("password") > + env_config["SWIFT_PREFIX"] = wp_plugin_swift_config.get("prefix") > + env_config["SWIFT_REGION"] = wp_plugin_swift_config.get("region") > + env_config["SWIFT_TENANT"] = wp_plugin_swift_config.get("tenant") > + env_config["SWIFT_URL"] = wp_plugin_swift_config.get("url") > + env_config["SWIFT_USERNAME"] = > wp_plugin_swift_config.get("username") > + env_config["SWIFT_COPY_TO_SWIFT"] = > wp_plugin_swift_config.get("copy-to-swift") > + env_config["SWIFT_SERVE_FROM_SWIFT"] = > wp_plugin_swift_config.get("serve-from-swift") > + env_config["SWIFT_REMOVE_LOCAL_FILE"] = > wp_plugin_swift_config.get("remove-local-file") > + > + env_config.update(self._db_config) > + return env_config > + > + def on_wordpress_uninitialised(self, event): > + """Setup the WordPress service with default values. > + > + WordPress will expose the setup page to the user to manually > + configure with their browser. This isn't ideal from a security > + perspective so the charm will initialise the site for you and > + expose the admin password via `get_initial_password_action`. > + > + This method observes all changes to the system by registering > + to the .on.config_changed event. This avoids current state split > + brain issues because all changes to the system sink into > + `on.config_changed`. > + > + It defines the state of the install ready state as: > + - We aren't leader, so check leader_data install state for the > installed state answer. > + - We aren't ready to setup WordPress yet (missing configuration > data). > + - We're ready to do the initial setup of WordPress (all dependent > configuration data set). > + - We're currently setting up WordPress, lock out any other events > from attempting to install. > + - WordPress is operating in a production capacity, no more work to > do, no-op. > + """ > + > + if self.unit.is_leader() is False: > + # Poorly named, expect a separate flag for non leader units here. > + self.state.installed_successfully = > self.leader_data.setdefault("installed", False) > + > + if self.state.installed_successfully is True: > + logger.warning("already installed, nothing more to do...") > + return > + > + # By using sets we're able to follow a state relay pattern. Each > event handler that is > + # responsible for setting state adds their flag to the set. Once > thet set is complete > + # it will be observed here. During the install phase we use > StoredState as a mutex lock > + # to avoid race conditions with future events. By calling .emit() we > flush the current > + # state to persistent storage which ensures future events do not > observe stale state. > + first_time_ready = {"leader", "db", "ingress", "leader"} > + install_running = {"attempted", "ingress", "db", "leader"} > + > + logger.debug( > + ( > + f"DEBUG: current install ready state is > {self.state.install_state}, " > + f"required install ready state is {first_time_ready}" We shouldn't be using format strings in logging I think - you should let the logger build the string for you using the `"%s", value` approach. > + ) > + ) > + > + if self.state.install_state == install_running: > + logger.info("Install phase currently running...") > + BlockedStatus("WordPress installing...") > + > + elif self.state.install_state == first_time_ready: > + # TODO: > + # Check if WordPress is already installed. > + # Would be something like > + # if self.is_vhost_ready():[...] > + WaitingStatus("WordPress not installed yet...") > + self.state.attempted_install = True > + self.state.install_state.add("attempted") > + logger.info("Attempting WordPress install...") > + self.on.wordpress_initial_setup.emit() > + > + def on_wordpress_initial_setup(self, event): > + logger.info("Beginning WordPress setup process...") > + container = self.unit.get_container(self.container_name) > + container.add_layer(self.container_name, self.wordpress_workload, > combine=True) > + > + # Temporary workaround until the init script is baked into the > Dockerimage. > + setup_service = "wordpressInit" > + src_path = f"src/{setup_service}.sh" > + charm_bin = "/charm/bin" > + dst_path = f"{charm_bin}/{setup_service}.sh" > + with open(src_path, "r", encoding="utf-8") as f: > + container.push(dst_path, f, permissions=0o755) > + > + admin_password = "/admin_password" > + config = self._get_initial_password() > + container.push(admin_password, config, permissions=0o400) > + > + logger.info("Adding WordPress layer to container...") > + self.ingress.update_config(self.ingress_config) > + container = self.unit.get_container(self.container_name) > + pebble = container.pebble > + wait_on = pebble.start_services([self.container_name]) > + pebble.wait_change(wait_on) > + > + logger.info("first time WordPress install was successful...") > + container.remove_path(admin_password) > + self.unit.status = MaintenanceStatus("WordPress Initialised") > + > + wait_on = pebble.stop_services([s for s in > self.wordpress_workload["services"]]) > + self.leader_data["installed"] = True > + self.state.installed_successfully = True > + self.on.config_changed.emit() > + > def on_config_changed(self, event): > - """Handle the config-changed hook.""" > - self.config_changed() > - > - def on_wordpress_initialise(self, event): > - wordpress_needs_configuring = False > - pod_alive = self.model.unit.is_leader() and self.is_service_up() > - if pod_alive: > - wordpress_configured = > self.wordpress.wordpress_configured(self.get_service_ip()) > - wordpress_needs_configuring = not self.state.initialised and not > wordpress_configured > - elif self.model.unit.is_leader(): > - msg = "Wordpress workload pod is not ready" > - logger.info(msg) > - self.model.unit.status = WaitingStatus(msg) > + """Merge charm configuration transitions.""" > + logger.debug(f"Event {event} install ready state is > {self.state.install_state}") > + > + is_valid = self.is_valid_config() > + if not is_valid: > return > > - if wordpress_needs_configuring: > - msg = "Wordpress needs configuration" > - logger.info(msg) > - self.model.unit.status = MaintenanceStatus(msg) > - initial_password = self._get_initial_password() > - installed = self.wordpress.first_install(self.get_service_ip(), > initial_password) > - if not installed: > - msg = "Failed to configure wordpress" > - logger.info(msg) > - self.model.unit.status = BlockedStatus(msg) > - return > - > - self.state.initialised = True > - logger.info("Wordpress configured and initialised") > - self.model.unit.status = ActiveStatus() > + container = self.unit.get_container(self.container_name) > + services = container.get_plan().to_dict().get("services", {}) > > - else: > - logger.info("Wordpress workload pod is ready and configured") > - self.model.unit.status = ActiveStatus() > + if services != self.wordpress_workload["services"]: > + logger.info("WordPress configuration transition detected...") > + self.unit.status = MaintenanceStatus("Transitioning WordPress > configuration") > + container.add_layer(self.container_name, > self.wordpress_workload, combine=True) > + > + self.unit.status = MaintenanceStatus("Restarting WordPress") > + running_services = [s for s in > self.wordpress_workload["services"] if container.get_service(s).is_running()] > + if running_services: > + container.pebble.stop_services(running_services) > + > + # Temporary workaround until the init script is baked into the > Dockerimage. > + setup_service = "wordpressInit" > + src_path = f"src/{setup_service}.sh" > + charm_bin = "/charm/bin" > + dst_path = f"{charm_bin}/{setup_service}.sh" > + with open(src_path, "r", encoding="utf-8") as f: > + container.push(dst_path, f, permissions=0o755) > + > + container.start(self.container_name) > + > + self.unit.status = ActiveStatus("WordPress service is live!") > + self.ingress.update_config(self.ingress_config) > + > + def on_database_config_changed(self, event): > + """Handle when the user supplies database details via charm config. > + """ > + if self.state.has_db_relation is False: > + db_config = {k: v or None for (k, v) in > self.model.config.items() if k.startswith("db_")} > + if any(db_config.values()) is True: # User has supplied db > config. > + current_db_data = {self.state.db_host, self.state.db_name, > self.state.db_user, self.state.db_password} > + new_db_data = {db_config.values()} > + db_differences = current_db_data.difference(new_db_data) > + if db_differences: > + self.on.wordpress_static_database_changed.emit() > > def on_db_relation_created(self, event): > """Handle the db-relation-created hook. -- https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress-1/+merge/404135 Your team Wordpress Charmers is requested to review the proposed merge of ~tcuthbert/charm-k8s-wordpress:sidecar into charm-k8s-wordpress:master. -- Mailing list: https://launchpad.net/~wordpress-charmers Post to : [email protected] Unsubscribe : https://launchpad.net/~wordpress-charmers More help : https://help.launchpad.net/ListHelp
