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

Add registry volume during provision #703

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Add registry volume during provision #703

merged 1 commit into from
Nov 17, 2021

Conversation

fgimenez
Copy link
Contributor

@fgimenez fgimenez commented Nov 11, 2021 •
edited
Loading

Will prevent errors like https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/logs/periodic-kubevirt-e2e-k8s-1.21-sig-network/1458811162673025024#1:build-log.txt%3A789, using podman as the container runtime makes the registry volume being mounted as readonly, this is the result of an inspect on the container created with the current version of gocli:

"Mounts": [
            {
                "Type": "volume",
                "Name": "k8s-1.20-registry",
                "Source": "/var/lib/containers/storage/volumes/k8s-1.20-registry/_data",
                "Destination": "/var/lib/registry",
                "Driver": "local",
                "Mode": "",
                "Options": [
                    "nosuid",
                    "nodev",
                    "rbind"
                ],
                "RW": false,
                "Propagation": "rprivate"
            }
        ],

Using a bind mount instead gives:

"Mounts": [
            {
                "Type": "bind",
                "Source": "/tmp/kubevirt_registry",
                "Destination": "/var/lib/registry",
                "Driver": "",
                "Mode": "",
                "Options": [
                    "rbind"
                ],
                "RW": true,
                "Propagation": "rprivate"
            }
        ],

/cc @rmohr @dhiller

Signed-off-by: Federico Gimenez fgimenez@redhat.com

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Nov 11, 2021
@fgimenez
Copy link
Contributor Author

/retest

@rmohr
Copy link
Member

rmohr commented Nov 16, 2021

@fgimenez I know we talked about this already but I am still confused. I think one key point is that I only want this to be a volume to not write into the container overlay. Why do I now have to care about the specific location? maybe it just needs the rw mode extra passed?

@fgimenez
Copy link
Contributor Author

@rmohr In fact we have a ReadOnly bool field in the mount struct https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/api/types/mount/mount.go#L30, which should be set to false by default. I've tried to explicitly set it to false without any luck.

I agree that having to specify the path of the bind mount is a pain, will try to create the volume during provision (same as we do with /var/run/disk)

@fgimenez fgimenez force-pushed the fix-registry-readonly branch from 2d49141 to f6528c6 Compare November 17, 2021 09:24
@fgimenez fgimenez changed the title Use bind mount for registry data Add registry volume during provision Nov 17, 2021
@fgimenez
Copy link
Contributor Author

@rmohr changes pushed to create the registry volume during provision as we do with /var/run/disk, tested with quay.io/kubevirtci/gocli:podman-test19 and quay.io/kubevirtci/k8s-1.20:podman-test19 and works fine PTAL

@rmohr
Copy link
Member

rmohr commented Nov 17, 2021

@rmohr changes pushed to create the registry volume during provision as we do with /var/run/disk, tested with quay.io/kubevirtci/gocli:podman-test19 and quay.io/kubevirtci/k8s-1.20:podman-test19 and works fine PTAL

Awesome. I guess all this confusion comes from the fact that we add the volumes and then snapshot the container during provision.

rmohr
rmohr approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@rmohr rmohr left a comment

Choose a reason for hiding this comment

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

@rmohr changes pushed to create the registry volume during provision as we do with /var/run/disk, tested with quay.io/kubevirtci/gocli:podman-test19 and quay.io/kubevirtci/k8s-1.20:podman-test19 and works fine PTAL

/lgtm
/approve

/hold

Could you do one final check with docker if the volumes are also there created as expected? If that looks fine to, feel free to unhold.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2021
@fgimenez
Copy link
Contributor Author

/unhold
/retest

thanks @rmohr tested successfully with docker, log here https://gist.github.com/fgimenez/87dfa36106d7a1b4b841bcae27188dc0

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021
@kubevirt-bot kubevirt-bot merged commit 986fdd4 into kubevirt:main Nov 17, 2021
@kubevirt-bot kubevirt-bot mentioned this pull request Nov 18, 2021
Merged
kubevirt-bot added a commit to kubevirt-bot/kubevirt that referenced this pull request Nov 18, 2021
@kubevirt-bot
Bump kubevirtci
4d5880a 
[986fdd4 Add registry volume during provision](kubevirt/kubevirtci#703)
[827961b network, multus: remove multus delegate config](kubevirt/kubevirtci#704)
[0528698 Fix deploy-manifests (exclude ip-reconciler pod)](kubevirt/kubevirtci#706)

```release-note
NONE
```

Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>
@fgimenez fgimenez deleted the fix-registry-readonly branch November 18, 2021 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr approved these changes

@dhiller dhiller Awaiting requested review from dhiller

Assignees

@rmohr rmohr

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 DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fgimenez @rmohr @kubevirt-bot