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.

Sign up for GitHub

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

Jump to bottom

Image pull implementation #26

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Image pull implementation #26

wants to merge 9 commits into from

Conversation

CertainLach
Copy link

@CertainLach CertainLach commented Sep 8, 2020 •
edited

I like the idea of Rust CRI implementation, and want to help in it :D

Is there any implementation plan/architecture description?

If no - i suggest using sled for internal storage, this is fast embedded kv database, which is pretty stable

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind dependency-change
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #16

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@CertainLach
Implement image storage db
03caafa 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@CertainLach
Implement list images request
e69f952 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 8, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CertainLach
To complete the pull request process, please assign harche
You can assign the PR to them by writing /assign @harche in a comment when ready.

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

@CertainLach
Image list test
4b0b935 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 8, 2020
haircommander
haircommander reviewed Sep 8, 2020
View reviewed changes
src/runtime.rs Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Member

I like the idea of Rust CRI implementation, and want to help in it :D

Nice, that's wonderful to hear! ❤️

Is there any implementation plan/architecture description?

Not yet, but are you interested in contributing to some? :) We could start with a fresh documentation page inside the repository. WDYT?

saschagrunert
saschagrunert reviewed Sep 8, 2020
View reviewed changes
src/runtime.rs Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 8, 2020
@CertainLach
Generalize database field name
9ec0fa8 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@CertainLach
Simplify database corruption error message
6a8e27c 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 8, 2020
@CertainLach
Merge commit '416f7863bdc20ef030c0b5e04bae4f4549b1863f' into image-se…
635ae13 
…rvice

Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot added release-note-none and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 8, 2020
@CertainLach
Implement image pull
f6677cf 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@CertainLach CertainLach mentioned this pull request Sep 8, 2020
Closed
@CertainLach
Layer locks, remove temp layers on failure
8be0d6d 
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2020
@CertainLach
Merge commit '14c2660951ecf74fee1d00ba83a9bf97f9579f7a' into image-se…
30a8986 
…rvice

Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2020
@CertainLach CertainLach changed the title Image service implementation Image pull implementation Sep 10, 2020
@CertainLach CertainLach marked this pull request as ready for review September 10, 2020 20:59
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2020
@CertainLach
Copy link
Author

I reduced scope of this PR, because currently i stuck at some bits of multiple transports support, feel free to merge this as partially finished work, i will commit more transports soon :D

@saschagrunert
Copy link
Member

Can you please rebase and resolve the conflict? 😇 Fixing the CI would be necessary now too.

@openshift-ci-robot
Copy link

@CertainLach: PR needs rebase.

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2020
harche
harche reviewed Sep 13, 2020
View reviewed changes
Copy link
Collaborator

@harche harche left a comment

Choose a reason for hiding this comment

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

Awesome work @CertainLach :) Thanks for your PR. I just had some minor comments, but they are just nits.

src/image_service.rs
Ok(MyImage {
database: sled::open(db_path)?,
layers,
registries: vec![DockerRegistryClientV2::new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead of hard coding the registries here maybe in future we can read from the config file, just like how we do it currently in CRI-O.

src/image_service.rs
pub async fn pull_image(&self, spec: &ImageSpec) -> Result<Option<Manifest>> {
// TODO: handle registry name in image name (I.e docker.io/redis:latest)
let parts = spec.image.split(':').collect::<Vec<&str>>();
assert_eq!(parts.len(), 2, "bad name format");
Copy link
Collaborator

@harche harche Sep 13, 2020 •
edited

Choose a reason for hiding this comment

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

In case image tag is absent, should we just default to latest instead ?

src/image_service.rs
};
let mut found_manifest = None;
for manifest in manifests.manifests {
// TODO: check platform with passed annotations
Copy link
Collaborator

@harche harche Sep 13, 2020 •
edited

Choose a reason for hiding this comment

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

I guess you meant in case of index manifest, we should check against architecture and os fields to see if they match.

@saschagrunert
Copy link
Member

@CertainLach do you mind giving this a rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert left review comments

@haircommander haircommander haircommander left review comments

@harche harche harche left review comments

@rhatdan rhatdan Awaiting requested review from rhatdan

@umohnani8 umohnani8 Awaiting requested review from umohnani8

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add image pull capability
5 participants
@CertainLach @openshift-ci-robot @saschagrunert @haircommander @harche