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

syscontainers: Fall back to destination #1114

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@ashcrow
Member

ashcrow commented Oct 11, 2017

Description

If /sysroot/ostree/deploy/rhel-atomic-host/var does not show the same
content destination then fall back destination.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1500961

Related Bugzillas

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Oct 11, 2017

Show outdated Hide outdated Atomic/syscontainers.py
Show outdated Hide outdated Atomic/syscontainers.py
Show outdated Hide outdated Atomic/syscontainers.py
Show outdated Hide outdated Atomic/syscontainers.py
@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 11, 2017

Member

@giuseppe updated per notes.

Member

ashcrow commented Oct 11, 2017

@giuseppe updated per notes.

@ashcrow ashcrow changed the title from WIP: syscontainers: Fall back to /var to WIP: syscontainers: Fall back to destination Oct 11, 2017

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 11, 2017

Member

thinking more of it. Would something like:

a = os.stat(dirname(destination))
b = os.stat(dirname(ostree_destination))
can_use_ostree_destination = a.st_dev == b.st_dev && a.st_ino == b.st_ino

be enough? I've not checked on AH, but I believe it should be enough

Member

giuseppe commented Oct 11, 2017

thinking more of it. Would something like:

a = os.stat(dirname(destination))
b = os.stat(dirname(ostree_destination))
can_use_ostree_destination = a.st_dev == b.st_dev && a.st_ino == b.st_ino

be enough? I've not checked on AH, but I believe it should be enough

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 11, 2017

Member

@giuseppe actually, yeah, it should be. I'll update.

Member

ashcrow commented Oct 11, 2017

@giuseppe actually, yeah, it should be. I'll update.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 11, 2017

Member

It assumes that destination is passed in with a trailing / but I think that's acceptable.

Member

ashcrow commented Oct 11, 2017

It assumes that destination is passed in with a trailing / but I think that's acceptable.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe updated

Member

ashcrow commented Oct 12, 2017

@giuseppe updated

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe
Member

giuseppe commented Oct 12, 2017

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Oct 12, 2017

📋 Looks like this PR is still in progress, ignoring approval

rh-atomic-bot commented Oct 12, 2017

📋 Looks like this PR is still in progress, ignoring approval

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Oct 12, 2017

Member

We always want hardlinks right? Otherwise the ostree bit is mostly pointless. So I think we should move the location to e.g. /usr/lib/atomic/containers.

Member

cgwalters commented Oct 12, 2017

We always want hardlinks right? Otherwise the ostree bit is mostly pointless. So I think we should move the location to e.g. /usr/lib/atomic/containers.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

/usr/lib/atomic/containers sounds fine to me. @cgwalters /usr/lib/atomic/containers for all cases or the case specific to Atomic Host?

Member

ashcrow commented Oct 12, 2017

/usr/lib/atomic/containers sounds fine to me. @cgwalters /usr/lib/atomic/containers for all cases or the case specific to Atomic Host?

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Oct 12, 2017

Member

I'd vote for both cases; system containers need to be tightly integrated with the host, and having them share storage with / hence makes sense to me. I not uncommonly blow away everything in /var/lib/docker, but that's only safe because nothing there is required by system bootstrap.

Member

cgwalters commented Oct 12, 2017

I'd vote for both cases; system containers need to be tightly integrated with the host, and having them share storage with / hence makes sense to me. I not uncommonly blow away everything in /var/lib/docker, but that's only safe because nothing there is required by system bootstrap.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

There is already some code for system containers preinstalled under /usr/lib/atomic/containers but they are treated as read only, so no modifications are allowed there. Will /usr/lib/atomic/containers be writeable?

What about moving the ostree repository for system containers to /var/lib/containers/atomic (with a parent repo of /ostree/repo on AH)?

Member

giuseppe commented Oct 12, 2017

There is already some code for system containers preinstalled under /usr/lib/atomic/containers but they are treated as read only, so no modifications are allowed there. Will /usr/lib/atomic/containers be writeable?

What about moving the ostree repository for system containers to /var/lib/containers/atomic (with a parent repo of /ostree/repo on AH)?

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe I remember you noting there was a requirement for /var/ to show the same result. Could we (easily) move to /usr/lib/atomic/containers/ as a single location for both AH and non-AH systems?

Member

ashcrow commented Oct 12, 2017

@giuseppe I remember you noting there was a requirement for /var/ to show the same result. Could we (easily) move to /usr/lib/atomic/containers/ as a single location for both AH and non-AH systems?

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Oct 12, 2017

Member

Yeah...a different approach would be to detect that /var is a mount point, and put the ostree repo there too. (Though this gives up sharing storage with the host...though we need to fix the selinux issue to really make that work). As far as having /usr be writable, that's no problem, right now /sysroot is writable, and that's the same method that rpm-ostree livefs uses. I'd like to make this a more formal API...see ostreedev/ostree#1265 but for now one can rely on it.

Member

cgwalters commented Oct 12, 2017

Yeah...a different approach would be to detect that /var is a mount point, and put the ostree repo there too. (Though this gives up sharing storage with the host...though we need to fix the selinux issue to really make that work). As far as having /usr be writable, that's no problem, right now /sysroot is writable, and that's the same method that rpm-ostree livefs uses. I'd like to make this a more formal API...see ostreedev/ostree#1265 but for now one can rely on it.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

I still feel like we should support both configurations:

  1. a system container is completely integrated into the system so it is deployed on /usr/lib and the systemd service file goes as well in /usr/lib/systemd/system (this is currently supported and atomic refuses to upgrade/manage them) and
  2. system containers that are installed on top of the immutable system and are deployed under /var.

If we install the system containers under /usr/lib wouldn't we need to reinstall them all on an upgrade?

Member

giuseppe commented Oct 12, 2017

I still feel like we should support both configurations:

  1. a system container is completely integrated into the system so it is deployed on /usr/lib and the systemd service file goes as well in /usr/lib/systemd/system (this is currently supported and atomic refuses to upgrade/manage them) and
  2. system containers that are installed on top of the immutable system and are deployed under /var.

If we install the system containers under /usr/lib wouldn't we need to reinstall them all on an upgrade?

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Oct 12, 2017

Member

Yeah duh...for AH/ostree based systems we'd indeed need to integrate atomic and rpm-ostree to teach the latter how to basically move the dir across updates. (Well and really this opens up the can of worms about being able to do both simultaneously).

So I think my short term vote is - detect if /var is a mount, and if so, make the syscontainers ostree repo live in /var too.

Member

cgwalters commented Oct 12, 2017

Yeah duh...for AH/ostree based systems we'd indeed need to integrate atomic and rpm-ostree to teach the latter how to basically move the dir across updates. (Well and really this opens up the can of worms about being able to do both simultaneously).

So I think my short term vote is - detect if /var is a mount, and if so, make the syscontainers ostree repo live in /var too.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

we can re-use this closed PR to achieve the migration of the repository:

#1105

The difference will be in 14e273c#diff-47fc29e44fac9e9de875734500ac6759R974

Instead of checking for the existence of "/ostree/repo" we will have to check if /var is on a different file system (the stat comparison already done in this PR)

Member

giuseppe commented Oct 12, 2017

we can re-use this closed PR to achieve the migration of the repository:

#1105

The difference will be in 14e273c#diff-47fc29e44fac9e9de875734500ac6759R974

Instead of checking for the existence of "/ostree/repo" we will have to check if /var is on a different file system (the stat comparison already done in this PR)

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe should we close this PR and move to+reopen #1105?

Member

ashcrow commented Oct 12, 2017

@giuseppe should we close this PR and move to+reopen #1105?

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

I think it is a good starting point, we do still need what is done here. We can cherry-pick the patch here on top of yours.

What I'd do:

  1. Simplify the check we are doing to just ATOMIC_VAR and /ostree/repo.
  2. Reuse it instead of 14e273c#diff-47fc29e44fac9e9de875734500ac6759R974
Member

giuseppe commented Oct 12, 2017

I think it is a good starting point, we do still need what is done here. We can cherry-pick the patch here on top of yours.

What I'd do:

  1. Simplify the check we are doing to just ATOMIC_VAR and /ostree/repo.
  2. Reuse it instead of 14e273c#diff-47fc29e44fac9e9de875734500ac6759R974
@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

@ashcrow could you cherry-pick the two commits from https://github.com/giuseppe/atomic/tree/1500961 ?

(and rebase on top of master so tests pass)

Member

giuseppe commented Oct 12, 2017

@ashcrow could you cherry-pick the two commits from https://github.com/giuseppe/atomic/tree/1500961 ?

(and rebase on top of master so tests pass)

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Oct 12, 2017

@giuseppe done

@ashcrow ashcrow changed the title from WIP: syscontainers: Fall back to destination to syscontainers: Fall back to destination Oct 12, 2017

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

I've tested it on RHELAH 7.4.2 and it fixes the issue for me.

@ashcrow if it is fine for you too, please feel free to r+ it.

@rh-atomic-bot delegate=ashcrow

Member

giuseppe commented Oct 12, 2017

I've tested it on RHELAH 7.4.2 and it fixes the issue for me.

@ashcrow if it is fine for you too, please feel free to r+ it.

@rh-atomic-bot delegate=ashcrow

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Oct 12, 2017

✌️ @ashcrow can now approve this pull request

rh-atomic-bot commented Oct 12, 2017

✌️ @ashcrow can now approve this pull request

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe Works on AH, not on non AH ...

$ sudo python atomic install --system --system-package=no -n rrr --storage ostree repo:8888/openshift3/cri-o:latest
FATA[0000] Invalid destination name ostree:repo:8888/openshift3/cri-o:latest@/var/lib/containers/atomic/.storage/ostree: invalid tag format 
Member

ashcrow commented Oct 12, 2017

@giuseppe Works on AH, not on non AH ...

$ sudo python atomic install --system --system-package=no -n rrr --storage ostree repo:8888/openshift3/cri-o:latest
FATA[0000] Invalid destination name ostree:repo:8888/openshift3/cri-o:latest@/var/lib/containers/atomic/.storage/ostree: invalid tag format 
@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

The error is coming from skopeo:

$ skopeo --version
skopeo version 0.1.23 commit: 24510500d48f15b52ddfed6972a2a56452ef16b6
Member

ashcrow commented Oct 12, 2017

The error is coming from skopeo:

$ skopeo --version
skopeo version 0.1.23 commit: 24510500d48f15b52ddfed6972a2a56452ef16b6
@edsantiago

This comment has been minimized.

Show comment
Hide comment
@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe it is, however I'm noticing with the fixed skopeo version atomic install --system --system-package=no -n NAME repo:8888/path/image:tag causes a failure. I just realized it's a different error that's causing the failure ...

sudo python atomic --debug install --system --system-package=no -n rrr --storage ostree repo:8888/openshift3/container-engine:latest

FATA[0000] Error initializing image from source docker://repo:8888/openshift3/container-engine:latest: pinging docker registry returned: Get https://repo:8888/v2/: http: server gave HTTP response to HTTPS client        

Adding http: in front causes an atomic failure. I believe this is an atomic issue that is caused from the new version of skopeo.

Member

ashcrow commented Oct 12, 2017

@giuseppe it is, however I'm noticing with the fixed skopeo version atomic install --system --system-package=no -n NAME repo:8888/path/image:tag causes a failure. I just realized it's a different error that's causing the failure ...

sudo python atomic --debug install --system --system-package=no -n rrr --storage ostree repo:8888/openshift3/container-engine:latest

FATA[0000] Error initializing image from source docker://repo:8888/openshift3/container-engine:latest: pinging docker registry returned: Get https://repo:8888/v2/: http: server gave HTTP response to HTTPS client        

Adding http: in front causes an atomic failure. I believe this is an atomic issue that is caused from the new version of skopeo.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

I'm checking out util.Decompose as it seems like by the time install the registry doesn't get set.

Member

ashcrow commented Oct 12, 2017

I'm checking out util.Decompose as it seems like by the time install the registry doesn't get set.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

I have a hacky patch I'm trying to make a little less hacky, but it works 😄

Member

ashcrow commented Oct 12, 2017

I have a hacky patch I'm trying to make a little less hacky, but it works 😄

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

@ashcrow it looks like: #1109

Does make any difference when you add the http: prefix?

Member

giuseppe commented Oct 12, 2017

@ashcrow it looks like: #1109

Does make any difference when you add the http: prefix?

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe yeah it would fail. I just added another patch which strips the http: or https: from the image name when installing via a new function in util. This ended up working for me. Without the patch adding http: would pass skopeo but fail in the actual installation. With the patch http: is required for non TLS registries but we will strip it out when it is used by other tools/calls that don't want it.

PTAL

Member

ashcrow commented Oct 12, 2017

@giuseppe yeah it would fail. I just added another patch which strips the http: or https: from the image name when installing via a new function in util. This ended up working for me. Without the patch adding http: would pass skopeo but fail in the actual installation. With the patch http: is required for non TLS registries but we will strip it out when it is used by other tools/calls that don't want it.

PTAL

giuseppe and others added some commits Sep 26, 2017

syscontainers: use repo under /var if on a different fs than /ostree/…
…repo

If /ostree/repo is on a different file system than the checkout path,
use /var/lib/containers/atomic/.storage/ostree as the location for the
ostree repository.
Having the repository and the checkouts directory on the same file
system permits checkouts to be done through hard links instead of copied
files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
syscontainers: Fall back to to destination
If /sysroot/ostree/deploy/rhel-atomic-host/var does not show the same
content as destination then fall back to the original destination.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1500961
syscontainers: refactor code in a function
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
syscontainers: Remove skopeo prefixes
http: is now required by skopeo when working with insecure registries.
However, having the prefix causes other calls/tools problems. This
change strips off http:/https: from the image uri outside of skopeo
usage.

Signed-off-by: Steve Milner <smilner@redhat.com>
@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Oct 12, 2017

Member

the http: prefix is handled by atomic, but anyway it is just a detail in the comment.

I added a fixup patch (a3e2277) to my branch, could you please cherry-pick it? I've seen this issue on a machine without a /ostree/repo directory already present.

Member

giuseppe commented Oct 12, 2017

the http: prefix is handled by atomic, but anyway it is just a detail in the comment.

I added a fixup patch (a3e2277) to my branch, could you please cherry-pick it? I've seen this issue on a machine without a /ostree/repo directory already present.

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Oct 12, 2017

@giuseppe done

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 12, 2017

Member

@giuseppe I did notice it was handled on pull, but on install it would pass through a different code path that wouldn't handle it properly. I looked at how it was being handled in syscontainers.py and made a simplified util version which is accessible outside of the syscontainers module.

Member

ashcrow commented Oct 12, 2017

@giuseppe I did notice it was handled on pull, but on install it would pass through a different code path that wouldn't handle it properly. I looked at how it was being handled in syscontainers.py and made a simplified util version which is accessible outside of the syscontainers module.

Show outdated Hide outdated Atomic/util.py
Show outdated Hide outdated Atomic/util.py

ashcrow added some commits Oct 13, 2017

syscontainers: Use util.remove_skopeo_prefixes
_get_skopeo_args now offloads removing of http:, https:, and oci: to
util.remove_skopeo_prefixes.
@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Oct 13, 2017

Member

@giuseppe updated

Member

ashcrow commented Oct 13, 2017

@giuseppe updated

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe
Member

giuseppe commented Oct 13, 2017

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Oct 13, 2017

⚡️ Test exempted: merge already tested.

rh-atomic-bot commented Oct 13, 2017

⚡️ Test exempted: merge already tested.

rh-atomic-bot added a commit that referenced this pull request Oct 13, 2017

syscontainers: refactor code in a function
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #1114
Approved by: giuseppe

rh-atomic-bot added a commit that referenced this pull request Oct 13, 2017

syscontainers: use repo under /var if on a different fs than /ostree/…
…repo

If /ostree/repo is on a different file system than the checkout path,
use /var/lib/containers/atomic/.storage/ostree as the location for the
ostree repository.
Having the repository and the checkouts directory on the same file
system permits checkouts to be done through hard links instead of copied
files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #1114
Approved by: giuseppe

rh-atomic-bot added a commit that referenced this pull request Oct 13, 2017

syscontainers: Remove skopeo prefixes
http: is now required by skopeo when working with insecure registries.
However, having the prefix causes other calls/tools problems. This
change strips off http:/https: from the image uri outside of skopeo
usage.

Signed-off-by: Steve Milner <smilner@redhat.com>

Closes: #1114
Approved by: giuseppe

rh-atomic-bot added a commit that referenced this pull request Oct 13, 2017

syscontainers: Use util.remove_skopeo_prefixes
_get_skopeo_args now offloads removing of http:, https:, and oci: to
util.remove_skopeo_prefixes.

Closes: #1114
Approved by: giuseppe

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

syscontainers: Fall back to to destination
If /sysroot/ostree/deploy/rhel-atomic-host/var does not show the same
content as destination then fall back to the original destination.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1500961

Closes: #1114
Approved by: giuseppe

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

syscontainers: refactor code in a function
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #1114
Approved by: giuseppe

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

syscontainers: use repo under /var if on a different fs than /ostree/…
…repo

If /ostree/repo is on a different file system than the checkout path,
use /var/lib/containers/atomic/.storage/ostree as the location for the
ostree repository.
Having the repository and the checkouts directory on the same file
system permits checkouts to be done through hard links instead of copied
files.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: #1114
Approved by: giuseppe

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

syscontainers: Remove skopeo prefixes
http: is now required by skopeo when working with insecure registries.
However, having the prefix causes other calls/tools problems. This
change strips off http:/https: from the image uri outside of skopeo
usage.

Signed-off-by: Steve Milner <smilner@redhat.com>

Closes: #1114
Approved by: giuseppe

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

syscontainers: Use util.remove_skopeo_prefixes
_get_skopeo_args now offloads removing of http:, https:, and oci: to
util.remove_skopeo_prefixes.

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