-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: dynamic sidebar content #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising, but I have some concerns.
src/charm.py
Outdated
except CheckFailed as check_failed: | ||
self.model.unit.status = check_failed.status | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 restart
This 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sidebar
relation 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.json
and reloads the configmap. Currently supporting (juju application names)Sidebar element is removed after the relation is broken.
Prerequisites:
Setup:
katib
andtensorboards
juju deploy ./*.charm --resource oci-image=$(yq eval '.resources.oci-image.upstream-source' metadata.yaml)
juju relate tensorboards-web-app:sidepanel kubeflow-dashboard:sidepanel
juju relate katib-ui:sidepanel kubeflow-dashboard:sidepanel