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

feat: add data connectors #478

Merged
merged 18 commits into from
Oct 30, 2024

Conversation

olevski
Copy link
Member

@olevski olevski commented Oct 23, 2024

Enables the data connectors to work with the new amalthea.

NOTE: The target branch is not main.

/deploy renku=feat-jupyter-free-sessions amalthea-sessions=main renku-ui=andrea/jupyter-free-build extra-values=amalthea-sessions.deployCrd=false

@olevski olevski requested a review from a team as a code owner October 23, 2024 09:40
@olevski olevski marked this pull request as draft October 23, 2024 09:40
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ds-478.dev.renku.ch

@olevski olevski temporarily deployed to renku-ci-ds-478 October 23, 2024 13:08 — with GitHub Actions Inactive
@olevski olevski temporarily deployed to renku-ci-ds-478 October 23, 2024 13:24 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from 3085cbb to ed2325a Compare October 23, 2024 23:17
@olevski olevski temporarily deployed to renku-ci-ds-478 October 23, 2024 23:30 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from df31b67 to ccb09b7 Compare October 24, 2024 00:23
@olevski olevski temporarily deployed to renku-ci-ds-478 October 24, 2024 00:24 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from ccb09b7 to 3062e07 Compare October 24, 2024 00:49
@olevski olevski temporarily deployed to renku-ci-ds-478 October 24, 2024 00:49 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from 3062e07 to 34b33aa Compare October 24, 2024 01:01
@olevski olevski temporarily deployed to renku-ci-ds-478 October 24, 2024 01:02 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from 34b33aa to ae494a1 Compare October 24, 2024 01:47
@olevski olevski temporarily deployed to renku-ci-ds-478 October 24, 2024 01:47 — with GitHub Actions Inactive
@olevski
Copy link
Member Author

olevski commented Oct 28, 2024

NOTE: Saved secrets do not work until this is merged and released: SwissDataScienceCenter/csi-rclone#37

But if you do not save the storage secret and then type it in at session start then things will work as expected.

@olevski olevski temporarily deployed to renku-ci-ds-478 October 28, 2024 20:41 — with GitHub Actions Inactive
@olevski olevski temporarily deployed to renku-ci-ds-478 October 29, 2024 18:07 — with GitHub Actions Inactive
@olevski olevski force-pushed the feat-add-data-connectors branch from ffb3570 to 5d3333b Compare October 29, 2024 18:25
@olevski olevski temporarily deployed to renku-ci-ds-478 October 29, 2024 18:25 — with GitHub Actions Inactive
@olevski olevski requested a review from leafty October 29, 2024 19:07
@olevski olevski temporarily deployed to renku-ci-ds-478 October 29, 2024 19:08 — with GitHub Actions Inactive
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Code looks good.

Question: Is this deployment fully functional? It would be nice to test the sessions with different combinations of data connectors (with saved or unsaved secrets, etc.).

Comment on lines 332 to 337
# TODO: The UI always does this check if it is acceptable/safe
# if csr_id in dcs_secrets and csr.configuration is not None:
# raise errors.ValidationError(
# message=f"Overriding the storage configuration for storage with ID {csr_id} "
# "is not allowed because the storage has an associated saved secret.",
# )
Copy link
Member

Choose a reason for hiding this comment

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

Note: I am not sure about the validity of this comment. What if both access_key_id and secret_access_key are required, but I only saved access_key_id?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I meant to remove this. I will remove it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its gone now.

return f"{prefix[:12]}-renku-2-{server_hash[:21]}"
# !NOTE: For now we limit the server name to a max of 25 characters.
# NOTE: This is 12 + 1 + 12 = 25 characters
return f"{prefix[:12]}-{server_hash[:12]}"
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have access to all the data-services information, it would be nice to have a human-readable session name:

{prefix}-{ns_slug}-{project_slug}-{launcher_slug}-{hash}

Also, it would be nicer to try to use Slug.from_name for the prefix part to avoid percent-encoded characters in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I have been meaning to do this. And now I am using Slug as you suggested. We actually have Slug.from_user which fits this pretty well. I just still have to do some extra cleaning because k8s is picky about the names.

@olevski olevski temporarily deployed to renku-ci-ds-478 October 30, 2024 13:09 — with GitHub Actions Inactive
@olevski
Copy link
Member Author

olevski commented Oct 30, 2024

Question: Is this deployment fully functional? It would be nice to test the sessions with different combinations of data connectors (with saved or unsaved secrets, etc.).

The only thing that does not work here is decrypting and using saved secrets for data connectors. It should work though once the fix I submitted for the csi rclone is rolled out on dev (the PR for this is merged and it will be released with renku 0.60.0). Rolling out a parallel version of rclone just for a CI deployment does not really work because both rclones step over one another.

@olevski olevski requested a review from leafty October 30, 2024 13:13
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Trying to use the deployment from the UI, this is what I get from trying to launch a session:

{
    "error": {
        "code": 1422,
        "message": "There are errors in the following fields, name: String should match pattern '^[a-z]([-a-z0-9]*[a-z0-9])?$'"
    }
}

@olevski
Copy link
Member Author

olevski commented Oct 30, 2024

Trying to use the deployment from the UI, this is what I get from trying to launch a session

Yes I just saw that. I am fixing it rn.

@olevski olevski merged commit 91b8e88 into release-amaltheas-migration Oct 30, 2024
14 checks passed
@olevski olevski deleted the feat-add-data-connectors branch October 30, 2024 14:50
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

olevski added a commit that referenced this pull request Oct 30, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Oct 31, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 1, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 4, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 6, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 8, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 11, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
olevski added a commit that referenced this pull request Nov 11, 2024
Co-authored-by: Samuel Gaist <[email protected]>
Co-authored-by: Ralf Grubenmann <[email protected]>
Co-authored-by: Flora Thiebaut <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants