feat: add auto create namespace #9328

amandavialva01 · 2024-05-07T17:03:58Z

feat: add auto create namespace

Ticket

DET-10224

Description

Add auto-create namespace.
Rebased onto #9329

Test Plan

Start up a multiRM cluster using two kubernetes resource managers with names rm1 and rm2 respectively.
Run det w create <workspace-name> --cluster-name rm1 --auto-create-namespace
Run kubectl get namespaces and verify that the namespace name returned from the above CLI command is listed

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-05-07T17:04:15Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`f35e221`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/663bf552fc99aa0008e0b890
😎 Deploy Preview	https://deploy-preview-9328--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jgongd · 2024-05-08T17:11:44Z

proto/src/determined/api/v1/workspace.proto

@@ -165,7 +171,7 @@ message PostWorkspaceResponse {
 };

 // The workspace created.
- determined.workspace.v1.Workspace workspace = 1;
+ determined.workspace.v1.Workspace workspace = 1; 


Nit: Could you remove the additional spaces.

jgongd · 2024-05-08T17:44:31Z

master/internal/api_workspace.go

 func validateWorkspaceName(name string) error {
 switch {
 case len(name) < 1:
 return status.Errorf(codes.InvalidArgument, "name '%s' must be at least 1 character long", name)
- case len(name) > 80:
- return status.Errorf(codes.InvalidArgument, "name '%s' must be at most 80 character long", name)
+ case len(name) > 53:


Could you help em understand why we limit workspace name to <= 52 characters? It's a bit unclear to me.

I initially changed this to 53 to aid in auto-generated namespace name character length validation. I changed this back to 80 char max and added a comment explaining the need to limit the character length of the corresponding auto-generated namespace name in #9226.

jgongd · 2024-05-08T18:27:16Z

master/internal/rm/kubernetesrm/pods.go

+ },
+ }
+
+ namespaceToCreate.Name = namespaceName


We may not need namespaceToCreate.Name = namespaceName, since it's assigned in line 1336.

namespaceToCreate.Name: Name is promoted to be accessible as if it's a direct field of the parent struct k8sV1.Namespace, since ObjectMeta is an anonymous field in struct k8sV1.Namespace and Name is a field in struct ObjectMeta.

Good catch! Changed in #9226

jgongd · 2024-05-10T14:09:09Z

harness/determined/cli/workspace.py

@@ -422,6 +464,27 @@ def yaml_file_arg(val: str) -> Any:
 cli.Arg("--json", action="store_true", help="print as JSON"),
 ],
 ),
+ cli.Cmd(
+ "bindings",


A workspace can create project(s) and a resource pool may bind to workspace(s).

To create a project: det project create workspace_name name

To bind a resource pool: det resource-pool binding add pool_name workspace_names

I'm trying to figure out the best way to bind a namespace to workspace.

Do we want to stick to the convention above, like:

det cluster binding add cluster_name workspace_name --auto-create-namespace

det cluster binding add cluster_name workspace_name --namespace_name name

Or the existing commands work better:

det workspace bindings add workspace_name cluster_name --auto-create-namespace

det workspace bindings add workspace_name cluster_name --namespace_name name

Let me get back to you on this.

@salonig23 and I talked a good amount about the best convention to follow for creating commands that perform workspace-namespace bindings. I think we settled on det workspace bindings add workspace_name cluster_name --auto-create-namespace and det workspace bindings add workspace_name cluster_name --namespace_name name because unlike resource pools, namespaces aren't determined concepts. Therefore, Kubernetes should be the source of truth for all metadata pertaining to a given namespace. All that we want to store on Determined's side is the workspace-namespace bindings, which is why we settled on adding the command to the set of workspace-related commands.
Starting the command with cluster instead of workspace would also imply that we would want to hit a cluster related API endpoint, which kind of goes against the organizational flow of adding metadata to our workspaces.
WDYT @jgongd @salonig23

…ace deletion

jgongd · 2024-05-15T21:07:13Z

harness/determined/cli/workspace.py

@@ -473,6 +477,8 @@ def yaml_file_arg(val: str) -> Any:
 cli.Arg("workspace_name", type=str, help="name of the workspace"),
 cli.Arg("cluster_name", type=str, help="cluster within which we create \


What keeps us from using resource_manager_name which is consistent with the master config?

I'm hoping to minimize the need of alias. So the users don't have to understand the cluster_name here should match the resource_manger_name in the master config.

In #9226, I added clusterName to the resource manager section of the master config to make the naming more intuitive.
Personally, I am not a huge fan of changing the CLI argument to resource-manager because it implies (IMO) that the namespace's existence is tied to the Determined resource manager when it is actually tied to the cluster.
I am open to more discussion about this though, because even cluster-name might get confusing since users might think this alludes to the name of the whole Determined deployment/cluster, rather than the clusterName in the resource manager's metadata in the master config

Personally, I am not a huge fan of changing the CLI argument to resource-manager because it implies (IMO) that the namespace's existence is tied to the Determined resource manager when it is actually tied to the cluster.

I thought that the resource manager is equivalent to the cluster(k8s cluster) in this project?

They are equivalent as far as the way we handle them in Determined, but let's say that someone starts up a Kubernetes cluster with Minikube and names it minikube_cluster_1, and then names the resource manager rm_1. Then, they create a namespace in minikube_cluster_1. After this, they take down their master, start a new Minikube cluster named minikube_cluster_2, leaving the resource manager name the same (rm_1). Now, this new cluster does not have the namespace created in minikube_cluster_1, but now in the db, we would have a workspace-namespace binding where resource_manager_name == rm_1 and namespace name is the name of the namespace created in minikube_cluster_1, but this namespace no longer exists in minikube_cluster_2.
Alternatively, if we use clusterName in the master config, the user would likely intuitively change clusterName to the name of the Minikube cluster, rather than leaving the resource manager name the same across different clusters.
All this to say, I think clusterName makes more sense as far as the config goes, because it creates less opportunity for a disconnect on our side between resources associated with a given cluster and how we handle them in the db.

Update: we have decided to change Name to ClusterName in the master config, so leaving the argument as cluster-name will be intuitive

carolinaecalderon

LGTM

carolinaecalderon · 2024-05-29T19:17:19Z

harness/determined/cli/workspace.py

@@ -194,12 +195,12 @@ def create_workspace(args: argparse.Namespace) -> None:
 agent_user_group = _parse_agent_user_group_args(args)
 checkpoint_storage = _parse_checkpoint_storage_args(args)

- if args.cluster_name and not args.namespace:
+ if args.cluster_name and (not args.namespace and not args.auto_create_namespace):


is there a cleaner way to represent this logic instead of A & (!B & !C)? maybe just remove the parentheses?

amandavialva01 requested review from a team as code owners May 7, 2024 17:03

amandavialva01 requested review from carolinaecalderon and jgongd May 7, 2024 17:03

cla-bot bot added the cla-signed label May 7, 2024

amandavialva01 mentioned this pull request May 7, 2024

chore: cleanup det-created namespaces during workspace deletion #9329

Open

4 tasks

feat: implement CLI and API for workspace-namespace bindings

7ab3190

jgongd reviewed May 8, 2024

View reviewed changes

amandavialva01 force-pushed the amanda/addAutoCreateNamespace branch from 9523e69 to f35e221 Compare May 8, 2024 21:57

jgongd reviewed May 10, 2024

View reviewed changes

amandavialva01 mentioned this pull request May 10, 2024

feat: implement API and CLI for set resource quota #9327

Open

4 tasks

amandavialva01 changed the base branch from main to amanda/cleanupNamespaces May 13, 2024 18:12

amandavialva01 force-pushed the amanda/cleanupNamespaces branch from 536fb59 to 1023ffd Compare May 13, 2024 19:05

amandavialva01 force-pushed the amanda/addAutoCreateNamespace branch from f35e221 to d073e49 Compare May 14, 2024 14:38

amandavialva01 added 2 commits May 14, 2024 11:08

progress

4e98118

chore: cleanup determined-created namespaces and quotas during worksp…

42325e3

…ace deletion

amandavialva01 force-pushed the amanda/cleanupNamespaces branch from 1023ffd to 42325e3 Compare May 14, 2024 15:11

feat: implement CLI and API for auto-creating namespaces in k8s

77c9422

amandavialva01 force-pushed the amanda/addAutoCreateNamespace branch from d073e49 to 77c9422 Compare May 14, 2024 15:13

jgongd reviewed May 15, 2024

View reviewed changes

amandavialva01 mentioned this pull request May 16, 2024

feat: implement commands and API endpoints for creating workspace-namespace bindings #9226

Open

4 tasks

amandavialva01 force-pushed the amanda/cleanupNamespaces branch 4 times, most recently from 89914b0 to 149e49c Compare May 23, 2024 20:57

carolinaecalderon approved these changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add auto create namespace #9328

feat: add auto create namespace #9328

amandavialva01 commented May 7, 2024 •

edited

netlify bot commented May 7, 2024 •

edited

jgongd May 8, 2024

jgongd May 8, 2024

amandavialva01 May 15, 2024

jgongd May 8, 2024 •

edited

amandavialva01 May 15, 2024

jgongd May 10, 2024 •

edited

amandavialva01 May 10, 2024 •

edited

jgongd May 15, 2024

amandavialva01 May 15, 2024

jgongd May 16, 2024

amandavialva01 May 16, 2024

amandavialva01 May 16, 2024 •

edited

carolinaecalderon left a comment

carolinaecalderon May 29, 2024

		@@ -473,6 +477,8 @@ def yaml_file_arg(val: str) -> Any:
		cli.Arg("workspace_name", type=str, help="name of the workspace"),
		cli.Arg("cluster_name", type=str, help="cluster within which we create \

feat: add auto create namespace #9328

Are you sure you want to change the base?

feat: add auto create namespace #9328

Conversation

amandavialva01 commented May 7, 2024 • edited

Ticket

Description

Test Plan

Checklist

netlify bot commented May 7, 2024 • edited

✅ Deploy Preview for determined-ui ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgongd May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgongd May 10, 2024 • edited

Choose a reason for hiding this comment

amandavialva01 May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amandavialva01 May 16, 2024 • edited

Choose a reason for hiding this comment

carolinaecalderon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amandavialva01 commented May 7, 2024 •

edited

netlify bot commented May 7, 2024 •

edited

jgongd May 8, 2024 •

edited

jgongd May 10, 2024 •

edited

amandavialva01 May 10, 2024 •

edited

amandavialva01 May 16, 2024 •

edited