Skip to content
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

fix: allow workspaces to be attempted to be deleted when experiments had existed historically [CM-412] #10050

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
43 changes: 37 additions & 6 deletions master/internal/api_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,17 @@ func (a *apiServer) workspaceHasModels(ctx context.Context, workspaceID int32) (
return exists, nil
}

func (a *apiServer) workspaceHasExperiments(ctx context.Context, workspaceID int32) (bool, error) {
exists, err := db.Bun().NewSelect().Table("experiments").
Join("INNER JOIN projects ON experiments.project_id = projects.id").
Where("projects.workspace_id=?", workspaceID).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Where("projects.workspace_id=?", workspaceID).
Where("projects.workspace_id = ?", workspaceID).

Exists(ctx)
if err != nil {
return false, fmt.Errorf("checking workspace for experiments: %w", err)
}
return exists, nil
}

func (a *apiServer) GetWorkspace(
ctx context.Context, req *apiv1.GetWorkspaceRequest,
) (*apiv1.GetWorkspaceResponse, error) {
Expand Down Expand Up @@ -1293,15 +1304,35 @@ func (a *apiServer) DeleteWorkspace(
return nil, err
}

modelsExist, err := a.workspaceHasModels(ctx, req.Id)
maxrussell marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
checks := []struct {
checkFunc func(context.Context, int32) (bool, error)
errorStr string
}{
{a.workspaceHasModels, "workspace (%d) contains models; move or delete models first"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxrussell it looks like you took this over. can you unroll this loop?

{a.workspaceHasExperiments, "workspace (%d) contains experiments; move or delete experiments first"},
}
if modelsExist {
return nil, status.Errorf(codes.FailedPrecondition, "workspace (%d) contains models; move or delete models first",
req.Id)

for _, check := range checks {
exists, err := check.checkFunc(ctx, req.Id)
if err != nil {
return nil, err
}
if exists {
return nil, status.Errorf(codes.FailedPrecondition, check.errorStr, req.Id)
}
}

// modelsExist, err := a.workspaceHasModels(ctx, req.Id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Thanks!

// if err != nil {
// return nil, err
// }
// if modelsExist {
// return nil, status.Errorf(codes.FailedPrecondition, "workspace (%d) contains models; move or delete models first",
// req.Id)
// }

// experimentsExist, err :=

holder := &workspacev1.Workspace{}
// TODO(kristine): DET-10138 update workspace state in transaction with template delete
err = a.m.db.QueryProto("deletable_workspace", holder, req.Id)
Expand Down
41 changes: 40 additions & 1 deletion master/internal/api_workspace_intg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"strconv"
"testing"
"time"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -939,7 +940,7 @@ func TestDeleteWorkspace(t *testing.T) {
mockRM := MockRM()
noName := ""
// set up api server
api, _, ctx := setupAPITest(t, nil, mockRM)
api, curUser, ctx := setupAPITest(t, nil, mockRM)
api.m.allRms = map[string]rm.ResourceManager{noName: mockRM}
// set up command service - required for successful DeleteWorkspaceRequest calls
cs, err := command.NewService(api.m.db, api.m.rm)
Expand Down Expand Up @@ -1002,6 +1003,44 @@ func TestDeleteWorkspace(t *testing.T) {
Id: resp.Workspace.Id,
})
require.Error(t, err)

// create another workspace, and add a experiment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, put the new test in a separate test.

w, p := createProjectAndWorkspace(ctx, t, api)
err = db.Bun().RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
autoGeneratedNamespaceName, err := getAutoGeneratedNamespaceName(ctx, w, &tx)
require.NoError(t, err)
autoCreatedNamespace = autoGeneratedNamespaceName
return nil
})
require.NoError(t, err)
e := createTestExpWithProjectID(t, api, curUser, p)
_, err = db.Bun().NewUpdate().Table("experiments").
Set("state = ?", model.CompletedState).
Where("id = ?", e.ID).Exec(ctx)
require.NoError(t, err)
_, err = api.DeleteWorkspace(ctx, &apiv1.DeleteWorkspaceRequest{
Id: int32(w),
})

require.Error(t, err)
// delete experiment, so that workspace can be deleted
_, err = api.DeleteExperiment(ctx, &apiv1.DeleteExperimentRequest{ExperimentId: int32(e.ID)})
require.NoError(t, err)

// since delete experiment is async, we need to wait for the experiment to be deleted
for i := 0; i < 10; i++ {
_, err = api.GetExperiment(ctx, &apiv1.GetExperimentRequest{ExperimentId: int32(e.ID)})
if err != nil {
break
}
time.Sleep(1 * time.Second)
}
// delete the workspace successfully
mockRM.On("DeleteNamespace", *autoCreatedNamespace).Return(nil).Once()
_, err = api.DeleteWorkspace(ctx, &apiv1.DeleteWorkspaceRequest{
Id: int32(w),
})
require.NoError(t, err)
}

func TestSetWorkspaceNamespaceBindings(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export const useWorkspaceActionMenu: (props: WorkspaceMenuPropsIn) => WorkspaceM
} else if (!workspace.archived) {
menuItems.push({ key: MenuKey.Edit, label: 'View Config' });
}
if (canDeleteWorkspace({ workspace }) && workspace.numExperiments === 0) {
if (canDeleteWorkspace({ workspace })) {
menuItems.push({ type: 'divider' });
menuItems.push({ danger: true, key: MenuKey.Delete, label: 'Delete...' });
}
Expand Down
Loading