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

ostree: fix mount when the `http:` prefix is used in the image name #1127

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@giuseppe
Member

giuseppe commented Nov 9, 2017

Fix "atomic mount" for system containers when http: is added to the image name

/cc @peterbaouoft

giuseppe added some commits Nov 8, 2017

mount: fail if image cannot be mounted from OSTree
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
mount: drop prefixes from the ostree image
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@peterbaouoft

I tend to be paying a lot more attention to details/minors at the beginning of journey to 'atomic'. I feel this helps me understand the codebase better. So hopefully you don't mind it :p.

And below and nits and questions. =)

# If ostree storage was explicitely requested, then we have to
# error out if the container/image could not be mounted.
if res == False:
raise ValueError("Could not mount {}".format(self.image))

This comment has been minimized.

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Hey, a minor question here. I noticed that in _try_ostree_mount(), it seems like the only case that FALSE is returned is that no images or containers is found via syscontainers function calls. (has_container and has_image both FALSE). It is this line:
https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR865. and
https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR166

Other cases seems all raising an error already upon mounting failure.
Case 1: https://github.com/projectatomic/atomic/blob/master/Atomic/mount.py#L289
Case 2: https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR857

If that is the case ( I hope I was right but ignore this line if I was wrong :p), do we need to change the description of the error to "Image not found" or something similar. But that.. would share a similarity with _no_such_image() below.. so thoughts?

==========================Another question for GLib.Error==============
I can't seem to find the place where the GLib.Error is raised in the code section

 except GLib.Error: # pylint: disable=catching-non-exception
                self._no_such_image()

Since I am relative new to the code base, can you give me a reminder for where is it being used? =)

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Hey, a minor question here. I noticed that in _try_ostree_mount(), it seems like the only case that FALSE is returned is that no images or containers is found via syscontainers function calls. (has_container and has_image both FALSE). It is this line:
https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR865. and
https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR166

Other cases seems all raising an error already upon mounting failure.
Case 1: https://github.com/projectatomic/atomic/blob/master/Atomic/mount.py#L289
Case 2: https://github.com/projectatomic/atomic/pull/1127/files#diff-ce5473cd96a720a53d0834698c9b2eaaR857

If that is the case ( I hope I was right but ignore this line if I was wrong :p), do we need to change the description of the error to "Image not found" or something similar. But that.. would share a similarity with _no_such_image() below.. so thoughts?

==========================Another question for GLib.Error==============
I can't seem to find the place where the GLib.Error is raised in the code section

 except GLib.Error: # pylint: disable=catching-non-exception
                self._no_such_image()

Since I am relative new to the code base, can you give me a reminder for where is it being used? =)

This comment has been minimized.

@giuseppe

giuseppe Nov 10, 2017

Member

that is the exception type that can be raised from the OSTree bindings. In this case OSTreeMount, both as a fallback and in the case the user != root, tries a checkout from OSTree and it can fail with that exception.

We cannot really use "Image not found" as the same mount is use to mount a container (that might be using an already deleted image from the repository), so I preferred the more generic "Cannot mount X", where X can be both an image or a container.

@giuseppe

giuseppe Nov 10, 2017

Member

that is the exception type that can be raised from the OSTree bindings. In this case OSTreeMount, both as a fallback and in the case the user != root, tries a checkout from OSTree and it can fail with that exception.

We cannot really use "Image not found" as the same mount is use to mount a container (that might be using an already deleted image from the repository), so I preferred the more generic "Cannot mount X", where X can be both an image or a container.

This comment has been minimized.

@peterbaouoft

peterbaouoft Nov 10, 2017

Contributor

that is the exception type that can be raised from the OSTree bindings

ah I see, I think it makes sense to me now =).

We cannot really use "Image not found" as the same mount is use to mount a container

Sorry for not being clear for the first time =(. I was trying to say that .. the source of this failure( FALSE ) was because the identifier(name) was neither found in containers or in image backend right? The Cannot mount X failure seems to me that the cause of failure was due to a mounting error? But it is really a minor nit =), so I think it is ok to keep the description.

@peterbaouoft

peterbaouoft Nov 10, 2017

Contributor

that is the exception type that can be raised from the OSTree bindings

ah I see, I think it makes sense to me now =).

We cannot really use "Image not found" as the same mount is use to mount a container

Sorry for not being clear for the first time =(. I was trying to say that .. the source of this failure( FALSE ) was because the identifier(name) was neither found in containers or in image backend right? The Cannot mount X failure seems to me that the cause of failure was due to a mounting error? But it is really a minor nit =), so I think it is ok to keep the description.

@@ -824,6 +827,8 @@ def mount(self, identifier, options=None): # pylint: disable=arguments-differ
if not OSTREE_PRESENT:
return False
identifier = util.remove_skopeo_prefixes(identifier)

This comment has been minimized.

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Hmm, since we add this removal for ostree prefix, I have a question: Do we also need to worry about something like
atomic pull --storage =docker http:${NAME} case? In the docker handlation part, do we also want to strip the prefix(http: or oci: or https:) off?

I understand this PR is mainly for ostree case, but unsure where to put a related question like this, so this is the wrong place, please correct me =).

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Hmm, since we add this removal for ostree prefix, I have a question: Do we also need to worry about something like
atomic pull --storage =docker http:${NAME} case? In the docker handlation part, do we also want to strip the prefix(http: or oci: or https:) off?

I understand this PR is mainly for ostree case, but unsure where to put a related question like this, so this is the wrong place, please correct me =).

This comment has been minimized.

@giuseppe

giuseppe Nov 10, 2017

Member

the Docker backend doesn't use the "http:" prefix notation since insecure registries are configured in the configuration for the Docker daemon (i.e. --insecure-registries)

@giuseppe

giuseppe Nov 10, 2017

Member

the Docker backend doesn't use the "http:" prefix notation since insecure registries are configured in the configuration for the Docker daemon (i.e. --insecure-registries)

This comment has been minimized.

@peterbaouoft

peterbaouoft Nov 10, 2017

Contributor

the Docker backend doesn't use the "http:" prefix notation

I think I get it now =). So we will be assuming when using docker as storage, atomic pull --storage=docker $image, we do not need to worry about images starting with ostree prefix (http: or oci: or https:) right?

@peterbaouoft

peterbaouoft Nov 10, 2017

Contributor

the Docker backend doesn't use the "http:" prefix notation

I think I get it now =). So we will be assuming when using docker as storage, atomic pull --storage=docker $image, we do not need to worry about images starting with ostree prefix (http: or oci: or https:) right?

This comment has been minimized.

@giuseppe

giuseppe Nov 10, 2017

Member

yes correct, there won't be such prefixes for Docker images

@giuseppe

giuseppe Nov 10, 2017

Member

yes correct, there won't be such prefixes for Docker images

# 4. Check that --shared works
${ATOMIC} mount --shared ${NAME} ${WORK_DIR}/mount
# 4. Check that --shared works and that 'http:' is dropped
${ATOMIC} mount --shared http:${NAME} ${WORK_DIR}/mount

This comment has been minimized.

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Is --shared option handled is ostree Mount in mount.py? I did not find the usage of it.. But I do find --shared used for DockerMount.

The reason why I ask is that.. this line will default to OstreeMount first right? Shown below in mount.py:

 if not self.storage:
            if len(self.beu.available_backends) > 1:
                if self.is_duplicate_image(self.image):
                    raise ValueError("Found more than one Image with name {}; "
                                     "please specify with --storage.".format(self.image))
            try:
                if self._try_ostree_mount(best_mountpoint_for_storage):
                    return

But ostree backend seems not handling the --shared option?

@peterbaouoft

peterbaouoft Nov 9, 2017

Contributor

Is --shared option handled is ostree Mount in mount.py? I did not find the usage of it.. But I do find --shared used for DockerMount.

The reason why I ask is that.. this line will default to OstreeMount first right? Shown below in mount.py:

 if not self.storage:
            if len(self.beu.available_backends) > 1:
                if self.is_duplicate_image(self.image):
                    raise ValueError("Found more than one Image with name {}; "
                                     "please specify with --storage.".format(self.image))
            try:
                if self._try_ostree_mount(best_mountpoint_for_storage):
                    return

But ostree backend seems not handling the --shared option?

This comment has been minimized.

@giuseppe

giuseppe Nov 10, 2017

Member

the OSTree storage doesn't use a different selinux mount label since system containers do not require a new file system when used. We do not create any new mount point, we only checkout the files to the file system (/var/lib/containers/atomic/$ID.0) so they all have the same mount label. Since it is shared in any case, the --shared option is simply ignored.

@giuseppe

giuseppe Nov 10, 2017

Member

the OSTree storage doesn't use a different selinux mount label since system containers do not require a new file system when used. We do not create any new mount point, we only checkout the files to the file system (/var/lib/containers/atomic/$ID.0) so they all have the same mount label. Since it is shared in any case, the --shared option is simply ignored.

@peterbaouoft

This comment has been minimized.

Show comment
Hide comment
@peterbaouoft

peterbaouoft Nov 14, 2017

Contributor

LGTM, tho, since I am new to the codebase, I feel it might be better if other reviewers can take a look at it as well =)

Contributor

peterbaouoft commented Nov 14, 2017

LGTM, tho, since I am new to the codebase, I feel it might be better if other reviewers can take a look at it as well =)

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Nov 14, 2017

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Nov 14, 2017

⌛️ Testing commit ea9b498 with merge 0999b88...

rh-atomic-bot commented Nov 14, 2017

⌛️ Testing commit ea9b498 with merge 0999b88...

rh-atomic-bot added a commit that referenced this pull request Nov 14, 2017

mount: drop prefixes from the ostree image
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #1127
Approved by: ashcrow
@TomSweeneyRedHat

This comment has been minimized.

Show comment
Hide comment
@TomSweeneyRedHat

TomSweeneyRedHat Nov 14, 2017

Member

LGTM, assuming happy tests.

Member

TomSweeneyRedHat commented Nov 14, 2017

LGTM, assuming happy tests.

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Nov 14, 2017

☀️ Test successful - status-papr
Approved by: ashcrow
Pushing 0999b88 to master...

rh-atomic-bot commented Nov 14, 2017

☀️ Test successful - status-papr
Approved by: ashcrow
Pushing 0999b88 to master...

eyusupov added a commit to eyusupov/atomic that referenced this pull request Mar 10, 2018

mount: fail if image cannot be mounted from OSTree
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: projectatomic#1127
Approved by: ashcrow

eyusupov added a commit to eyusupov/atomic that referenced this pull request Mar 10, 2018

mount: drop prefixes from the ostree image
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: projectatomic#1127
Approved by: ashcrow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment