Skip to content

Conversation

@aleksandrmelnikov
Copy link
Contributor

@aleksandrmelnikov aleksandrmelnikov commented May 28, 2020

What type of PR is this?

/kind enhancement

What this PR does:
This PR adds CVAT as a workspace template to the template builder.

  • DB entries and k8s operations are carried out.
    This also includes fixes to the JupyterLab migraiton

Which issue(s) this PR fixes:

closes #317

Special notes for your reviewer:
Make sure you carry out the steps here first:

If you want to test on your cluster directly

  • Pull down this branch
  • Run make docker to push the image to docker
  • Update your cluster deployment for onepanel-core to use the commit-hash image
  • Wait for changes to take affect
  • Exec into your onepanel-core pod
  • Run ./goose status to verify that cvat migration is executed
    Note: If you're seeing crash loops, check the onepanel-core logs. The jupyter and cvat migrations throw a fatal log if an error is encountered while executing.

Assuming the initial migration up ran without errors:

  • Check CVAT is available as a template in workspaces
  • Create 1-2 workspaces with CVAT as base
  • Wait for them to start up or to appear in the UI (database, k8s entries)
  • Run goose down inside the pod.
  • You should see no errors
  • The workspaces should now be flagged as "terminating" in the UI
  • Database entries for workspace_templates will show archived, along with the related workflow_template

If you are developing locally, off of code

  • You need to make sure your kubeconfig context is pointing to a k8s cluster.
  • Pull down core-ui; run npm install, ng serve
  • Wait for web to start
  • Pull down this branch for core
  • Compile main.go, have it running so it's taking requests

Assuming the initial migration up ran without errors:

  • Check CVAT is available as a template in workspaces
  • Create 1-2 workspaces with CVAT as base
  • Wait for them to start up or to appear in the UI (database, k8s entries)
  • Compile cmd/goose.go, run ./goose down
  • You should see no errors
  • The workspaces should now be flagged as "terminating" in the UI
  • Database entries for workspace_templates will show archived, along with the related workflow_template
  • Test again by running ./goose up, creating more cvat, then ./goose down

Aleksandr Melnikov and others added 3 commits May 30, 2020 18:05
- Per testing, a migration error does not happen with an Azure deployment.
@rushtehrani rushtehrani force-pushed the feat/add.cvat.migration.template branch from cd6cec2 to 56856ca Compare May 31, 2020 01:10
}

for _, namespace := range namespaces {
if err := client.DeleteWorkspace(namespace.Name, cvatTemplateName); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aleksandrmelnikov @Vafilor is this supposed to be client.ArchiveWorkspaceTemplate? If so, the JupyterLab one needs to be fixed too.

Also note the changes I made in the this PR: 7b570f8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two thoughts:

  • If we want to support running the "down" migration after they make DB / K8S changes (or they've done some work), then yes. I think we need to make this change.
  • If they do this immediately on creation, then this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed up a possible change.

@rushtehrani rushtehrani changed the title Feat/add.cvat.migration.template feat: add.cvat.migration.template Jun 1, 2020
Aleksandr Melnikov added 2 commits June 1, 2020 10:59
- This will add the goose command with the ".go" migration files.
This is needed to run goose down commands.
goose.go is needed to generate an executable.
@rushtehrani rushtehrani changed the base branch from develop to dev June 2, 2020 17:41
@rushtehrani rushtehrani added the kind/enhancement New feature or request label Jun 2, 2020
@rushtehrani rushtehrani added this to the v0.10.0 milestone Jun 2, 2020
Aleksandr Melnikov added 5 commits June 2, 2020 17:05
- The passed in name was not converted to the UID format, so the code
could never find the relevant data.
- This is if there are no workspaces in the database.
- This code was re-arranged to focus on archiving the k8s components
first, then the database.
- Also, workspace components are archived first,
because they rely on the workflow templates being present.
If workflow template is archived first, then the clean-up
can't resume to run again if some error is thrown.
This arrangement will ensure that if an error occurs during clean-up,
it can be re-run without throwing an error of missing prerequisite data.
- Removed an early return statement, which prevented the rest
of the clean-up
- Added extra code to not return an error if no workflow templates
are found in the database. This is to ensure k8s cleanup can continue.
- Fixed various error message responses
- Added code to grab the WorkspaceTemplate versions
Then, code will run through these versions and grab workspaces that use
that version.
Then, code will try to Archive those workspaces (delete k8s resources,
archive database entries).
Finally, code cleans up the related WorkflowTemplate.
- Updated method docs
- Migrations should not go on if there is an error
@aleksandrmelnikov aleksandrmelnikov self-assigned this Jun 3, 2020
@rushtehrani
Copy link
Contributor

@aleksandrmelnikov I keep getting this error when I try this in an existing cluster. I've used multiple names to make sure there is no conflict.

2020/06/03 13:47:01 error rpc error: code = Unknown desc = Error with insert into workspace_templates_versions.  already exists.
Exiting.

@rushtehrani rushtehrani changed the base branch from dev to master June 3, 2020 22:01
@rushtehrani rushtehrani changed the base branch from master to dev June 3, 2020 22:01
@rushtehrani rushtehrani changed the title feat: add.cvat.migration.template feat: add cvat migration template Jun 4, 2020
@rushtehrani rushtehrani merged commit a47bbe9 into dev Jun 4, 2020
@rushtehrani rushtehrani deleted the feat/add.cvat.migration.template branch June 4, 2020 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants