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

overlay: Extend documentation comment for layout #1933

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cgwalters
Copy link
Contributor

@cgwalters cgwalters commented May 30, 2024

Let's expand the existing doc comment, as it's really
useful to understand the implementation in order to
debug or improve things.

We also clearly say first that these are implementation details.

Signed-off-by: Colin Walters [email protected]

@cgwalters
Copy link
Contributor Author

BTW, feel free to (please do) just force push updates to this PR to add/fix things.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Just some quick comments because I glanced at the PR list…

One vague thought: that there are two very different audiences who might read this: support investigators who need to understand some specific broken filesystem state, and developers who need to understand all possible combinations and rules and invariants.

docs/containers-storage-architecture.md Outdated Show resolved Hide resolved
## DESCRIPTION
The *containers/storage* library manages the following object types:

- layer: A filesystem tree, along with metadata
Copy link
Collaborator

Choose a reason for hiding this comment

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

… organized in, and optimized for, a tree of parent/child links, where a single “layer” may either serve as a complete filesystem tree, or a difference from the parent.

Comment on lines 15 to 16
- image: A reference to a layer, along with metadata
- container: A layer whose parent is an image
Copy link
Collaborator

Choose a reason for hiding this comment

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

These feel a bit too reductionist to me.

The important thing about images is that, unlike, layers, users frequently with deal with them directly; that they have human-usable names; and that they are nevertheless deduplicated.

The important thing about containers is… ? That the top layer is read-write? That a lot of the metadata is actually not in c/storage but in Podman’s / CRI-O’s data? Also, technically the parent of the container’s layer is still a layer.

- image: A reference to a layer, along with metadata
- container: A layer whose parent is an image

## Layer storage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Discusssion of graph drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would make sense to add in theory but I personally don't care much since I think we're in a place where there is only the overlay driver that matters, right?

docs/containers-storage-architecture.md Outdated Show resolved Hide resolved
docs/containers-storage-architecture.md Outdated Show resolved Hide resolved
docs/containers-storage-architecture.md Outdated Show resolved Hide resolved
docs/containers-storage-architecture.md Outdated Show resolved Hide resolved
Let's expand the existing doc comment, as it's really
useful to understand the implementation in order to
debug or improve things.

We also clearly say first that these are implementation details.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters changed the title docs: Add containers-storage-architecture.md overlay: Extend documentation comment for layout May 31, 2024
@rhatdan
Copy link
Member

rhatdan commented Jun 5, 2024

@giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 5, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Jun 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, rhatdan

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

@openshift-ci openshift-ci bot added the approved label Jun 5, 2024
@rhatdan
Copy link
Member

rhatdan commented Jun 5, 2024

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 20, 2024

@giuseppe @saschagrunert @mtrmac PTAL

@cgwalters
Copy link
Contributor Author

This one isn't urgent, I think we can just leave it as draft and I may circle back to it later. No need for others to dive in for review now, we have more important things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants