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

📖 Document how storage keys are computed for workspaces #1905

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

p0lyn0mial
Copy link
Contributor

@p0lyn0mial p0lyn0mial commented Sep 7, 2022

Summary

Initially, I wanted to document the mapping of a URL to a storage prefix. However, there seems to be an open PR addressing that.

Instead, I've decided to describe how a storage key is computed and why it matters.

It could complement #1086, we don't have to merge it if it doesn't add any value.

Related issue(s)

Fixes #

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ncdc for approval by writing /assign @ncdc in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@p0lyn0mial
Copy link
Contributor Author

/assign @stevekuznetsov @sttts

@stevekuznetsov
Copy link
Contributor

I think as far as user-facing docs, #1086 is likely going to answer more questions than this one, so at the least we should get both in.

Everything else, like a workspace name, is added dynamically.
For example, a request with a URL of `/clusters/acme/core/secrets` finds a storage responsible
for `core/secrets` resources and adds `acme` to the `ResourcePrefix`.
The new string becomes a key that is passed to etcd to find only resources for `acme` cluster.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is anything, in particular, we would like to see documented, let me know, I could expand this doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about documenting PartialObjectMetadata requests and their relation to the generic storage.

@@ -0,0 +1,74 @@
### workspaces

kcp promises to support 1 mln of workspaces.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kcp promises to support 1 mln of workspaces.
kcp's goal is to support 1 million of workspaces.

### etcd

etcd is the primary datastore used by kcp.
It stores data in a key-value store.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It stores data in a key-value store.
It stores data as key-value.

One "store" less.

The store’s logical view is a flat binary key space.
The key space has a lexically sorted index on byte string keys.

In order to create a logical hierarchy, keys are usually mixed with `/` e.g. `/company/branch/location`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In order to create a logical hierarchy, keys are usually mixed with `/` e.g. `/company/branch/location`
In order to create a logical hierarchy, keys are usually structured with `/` e.g. `/company/branch/location`

foo
```

Note that those queries are based on byte comparisons.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that those queries are based on byte comparisons.
Note, that those queries are based on byte comparisons.

For example, `secrets` resources in the `core` API group gets their own storage.

From the perspective of this document, we can assume that the most important feature of generic storage
is to compute the key that is passed to the database to find the data the user wants.
Copy link
Member

Choose a reason for hiding this comment

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

I like this view a lot!


![](registry.png)

When the server starts it precomputes the `ResourcePrefix` with a group and a resource name.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When the server starts it precomputes the `ResourcePrefix` with a group and a resource name.
When the server starts, it precomputes the `ResourcePrefix` with a group and a resource name.

@ncdc ncdc changed the title documents computing storage keys for workspaces. 📖 documents computing storage keys for workspaces. Sep 9, 2022
@ncdc ncdc changed the title 📖 documents computing storage keys for workspaces. 📖 Document how storage keys are computed for workspaces Sep 12, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2022

@p0lyn0mial: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-sharded 76d5bb5 link true /test e2e-sharded

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ncdc
Copy link
Member

ncdc commented Mar 6, 2023

@p0lyn0mial I've moved what was in 1086 to #2867. Would you be interested in adding to that doc? Or do you want to close this?

@p0lyn0mial
Copy link
Contributor Author

@p0lyn0mial I've moved what was in 1086 to #2867. Would you be interested in adding to that doc? Or do you want to close this?

sure, I would like to move it to https://github.com/kcp-dev/kcp/tree/main/docs/content/developers. Also please let me know when you you think it doesn't bring any value.

@mjudeikis
Copy link
Contributor

@kcp-dev/kcp-contributors lets pick this up?

@sttts
Copy link
Member

sttts commented Mar 25, 2024

/lgtm
/approve

We can iterate. Just docs.

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2024
@kcp-ci-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 20a00fe8326fb70474cf9d74bee29afa098baf7e

@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2024
@embik
Copy link
Member

embik commented Mar 26, 2024

/retest

@embik embik closed this Mar 26, 2024
@embik embik reopened this Mar 26, 2024
@kcp-ci-bot kcp-ci-bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Mar 26, 2024
@kcp-ci-bot kcp-ci-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 26, 2024
@sttts sttts merged commit e601352 into kcp-dev:main Mar 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants