wordpress-charmers team mailing list archive
-
wordpress-charmers team
-
Mailing list archive
-
Message #00749
Re: [Merge] ~tcuthbert/charm-k8s-wordpress:sidecar into charm-k8s-wordpress:master
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.
References
