feat: dynamic sidebar content #33
Conversation
src/charm.py
Outdated
| except CheckFailed as check_failed: | ||
| self.model.unit.status = check_failed.status | ||
| return |
There was a problem hiding this comment.
I don't think we want to change the unit status, we only skip it if its not the leader as it can't modify the relation
src/charm.py
Outdated
| self.log.info(f"ADDING {event.app.name} to side panel") | ||
| side_bar_tabs_dict = json.loads(self._stored.side_bar_tabs) | ||
| if ( | ||
| SIDEBAR_EXTRA_OPTIONS[event.app.name]["menuLink"] |
There was a problem hiding this comment.
The app name is configurable by the user, you can deploy katib with whatever name, how will this be reflected here?
There was a problem hiding this comment.
I think this relates to a broader conversation here. In general, dashboard shouldn't know anything ahead of time about the applications placed into the sidebar
src/charm.py
Outdated
| except CheckFailed as check_failed: | ||
| self.model.unit.status = check_failed.status | ||
| return |
src/charm.py
Outdated
| @@ -67,6 +84,7 @@ def main(self, event): | |||
|
|
|||
| model = self.model.name | |||
| config = self.model.config | |||
| configmap_hash = self._generate_config_hash() | |||
There was a problem hiding this comment.
Is this to force a pod restart?
src/charm.py
Outdated
| self._stored.set_default(hash_salt=_gen_pass()) | ||
| self._stored.set_default(side_bar_tabs=Path("src/config.json").read_text()) |
There was a problem hiding this comment.
While storedstate seems like a reasonable way of doing things, there are some key downsides with it in charms. It is unit specific and it will be reset if the unit restarts. How is this going to affect this feature? Can we avoid using stored state?
There was a problem hiding this comment.
I think stored-state for kubernetes applications is by default stored on the controller, which I think makes it resistant to pod restarts. I haven't tried it though
src/charm.py
Outdated
| def __init__(self, *args): | ||
| super().__init__(*args) | ||
|
|
||
| self.log = logging.getLogger(__name__) | ||
| self.image = OCIImageResource(self, "oci-image") | ||
| self._stored.set_default(hash_salt=_gen_pass()) |
There was a problem hiding this comment.
I'm guessing this was pulled from one of our other charms that happens to include a password in the configmap, and thus we'd salt the hash to make it less obvious. In this case where we're just storing details of a sidebar, do we need a salt at all? Or at least do we need a randomly generated one?
src/charm.py
Outdated
| if event.app.name in SIDEBAR_EXTRA_OPTIONS: | ||
| self.log.info(f"REMOVING {event.app.name} to side panel") | ||
| side_bar_tabs_dict = json.loads(self._stored.side_bar_tabs) | ||
| side_bar_tabs_dict["menuLinks"] = [ | ||
| link | ||
| for link in side_bar_tabs_dict["menuLinks"] | ||
| if link != SIDEBAR_EXTRA_OPTIONS[event.app.name]["menuLink"] | ||
| ] | ||
| self.log.info(f"NEW SIDE BAR {side_bar_tabs_dict}") | ||
| self._stored.side_bar_tabs = json.dumps(side_bar_tabs_dict) |
There was a problem hiding this comment.
This is event-based (if relation leaves, remove relation from sidebar), which then means we need to pass the state of the sidebar between _on_sidepanel_relation_broken and main. What about in main just doing something like
def main():
sidebar = compute_sidebar_from_relations()
if sidebar != existing_configmap:
update_configmap(sidebar)
# then trigger pod restartThis removes the need for stored state and simplifies the event handling logic
src/extra_config.json
Outdated
| { | ||
| "katib-ui": { | ||
| "menuLink": { | ||
| "type": "item", | ||
| "link": "/katib/", | ||
| "text": "Experiments (AutoML)", | ||
| "icon": "kubeflow:katib" | ||
| } | ||
| }, | ||
| "tensorboards-web-app": { | ||
| "menuLink": { | ||
| "type": "item", | ||
| "link": "/tensorboards/", | ||
| "text": "Tensorboards", | ||
| "icon": "assessment" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We can make this more dynamic and remove the need for this extra_config.json entirely. Each menuLink item needs type, link, text, icon. This can be passed through the sidepanel relation. So the katib-ui application could relate to the sidepanel relation with data:
{
type: "item",
link: "/katib/",
text: "Experiments (AutoML)",
icon: "kubeflow:katib", <-- I dont know how this works - maybe this takes extra effort
}This doesn't really add much over what you have here, but it also means some unknown application can also use the relation:
{
type: "item",
link: "/wherever-i-exist/",
text: "Some custom application someone else made",
icon: "my-fancy-icon",
}This way whenever anyone wants to add a menu item, it is driven dynamically through the relation and requires no additional code change here.
In general, we can make Dashboard into a provider of a sidebar and ignorant to the actual content.
Related to this jira task and #8
There is a
sidebarrelation with thesrc/extra_config.json. In order to add element to the sidebar proper relation needs to be established. After the relation is set, charms fills the config for given app-name fromsrc/extra_config.jsonand reloads the configmap. Currently supporting (juju application names)Sidebar element is removed after the relation is broken.
Prerequisites:
Setup:
katibandtensorboardsjuju deploy ./*.charm --resource oci-image=$(yq eval '.resources.oci-image.upstream-source' metadata.yaml)juju relate tensorboards-web-app:sidepanel kubeflow-dashboard:sidepaneljuju relate katib-ui:sidepanel kubeflow-dashboard:sidepanel