Status, Details, and List rayclusters CLI Functions #258
Conversation
There was a problem hiding this comment.
Looks great, just a couple of small adjustments:
- Rather than have the default namespace be "default" for list/status/details, I think we should simply require the user to specify the namespace. We are slowly moving away from default="default" (still some things to change in the SDK), as it's not actually what most people use.
- You may want to clean up output when a cluster of a certain name is not found:
Traceback (most recent call last):
File "/opt/homebrew/bin/codeflare", line 8, in <module>
sys.exit(cli())
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 1128, in __call__
return self.main(*args, **kwargs)
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 1053, in main
rv = self.invoke(ctx)
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 1659, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 1659, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 1395, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/opt/homebrew/lib/python3.8/site-packages/click/core.py", line 754, in invoke
return __callback(*args, **kwargs)
File "/opt/homebrew/lib/python3.8/site-packages/click/decorators.py", line 26, in new_func
return f(get_current_context(), *args, **kwargs)
File "/Users/meyceoz/Documents/codeflare-sdk/src/codeflare_sdk/cli/commands/status.py", line 19, in raycluster
cluster = get_cluster(name, namespace)
File "/Users/meyceoz/Documents/codeflare-sdk/src/codeflare_sdk/cluster/cluster.py", line 433, in get_cluster
raise FileNotFoundError(
FileNotFoundError: Cluster quicktest is not found in default namespace
This is the correct error, though since this is a CLI, we may want to catch this FileNotFoundError and just tell the user the message part "Cluster {name} not found in {ns} namespace"
- With the current implementation, it seems as though we are rewriting the appwrapper to storage every time we call
statusordetails, due to recreation of theClusterobject. I don't think this is necessarily a blocking issue, but something we may want to think about, or maybe open an issue for, for tracking purposes.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Maxusmusti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@Maxusmusti @carsonmh what about using |
| @click.option("--namespace", type=str) | ||
| @click.option("--all", is_flag=True) | ||
| @click.pass_context | ||
| def rayclusters(ctx, namespace, all): |
There was a problem hiding this comment.
can this be raycluster? Its a little confusing when sometimes we want to use raycluster and othertimes we use rayclusters
There was a problem hiding this comment.
I'll create an alias so the user can do either
| def _get_all_ray_clusters() -> List[RayCluster]: | ||
| list_of_clusters = [] | ||
| try: | ||
| config_check() | ||
| api_instance = client.CustomObjectsApi(api_config_handler()) | ||
| rcs = api_instance.list_cluster_custom_object( | ||
| group="ray.io", | ||
| version="v1alpha1", | ||
| plural="rayclusters", | ||
| ) | ||
| except Exception as e: | ||
| return _kube_api_error_handling(e) | ||
| for rc in rcs["items"]: | ||
| list_of_clusters.append(_map_to_ray_cluster(rc)) | ||
| return list_of_clusters |
There was a problem hiding this comment.
Can we add this functionality into the original _get_ray_clusters() function?
the idea is that by default, _get_ray_clusters() get from all namespaces. If you provide a namespace, then it will select only from that namespace.
@Maxusmusti WDYT?
| def raycluster(name, namespace): | ||
| """ | ||
| Delete a specified RayCluster from the Kubernetes cluster | ||
| """ | ||
| cluster = get_cluster(name, namespace) | ||
| try: | ||
| cluster = get_cluster(name, namespace) |
There was a problem hiding this comment.
Its a little awkward that delete command returns a "Written to: xyz.yaml` message. Anyway we can avoid this?
Issue link
Closes #240
What changes have been made
list rayclustersdetails rayclusterandstatus rayclusterfunctions in the CLIlist_clusters_all_namespacesto list all RayClusters in all the namespaces in a Kubernetes ClusterVerification steps
After building SDK, see list of commands using
codeflarelist rayclustersfor listing, optionally specifying namespace or--allstatus rayclusterspecifying name as an argument and namespace as an optiondetails rayclusterspecifying name as an argument and namespace as an optionThere are also unit tests for these functions to be run
Checks