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

server: allow Bidirectional mounts that contain storage root #7408

Merged
merged 1 commit into from
Oct 31, 2023
Merged

server: allow Bidirectional mounts that contain storage root #7408

merged 1 commit into from
Oct 31, 2023

Conversation

haircommander
Copy link
Member

@haircommander haircommander commented Oct 19, 2023 •
edited

previously we would never allow a container to get anything other than HostToContainer for something that contains the storage root. This breaks situations where the container wishes to update mount points within the container and see them propegated to the host.

Instead, we should only override this if the mount is Private.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

fixes #7308

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix a bug where CRI-O would override a Bidirectional mount in favor of a HostToContainer if the mount contained the host's container storage

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2023
@haircommander haircommander mentioned this pull request Oct 19, 2023
Closed
@haircommander
Copy link
Member Author

cc @gnufied

@haircommander
Copy link
Member Author

cc @harche

@haircommander haircommander marked this pull request as ready for review October 20, 2023 13:49
@harche
Copy link
Contributor

harche commented Oct 20, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2023
@gnufied
Copy link

gnufied commented Oct 23, 2023

Since this was a regression. Can we get some unit or e2e/integration tests in crio itself to ensure we don't break again?

@haircommander haircommander mentioned this pull request Oct 24, 2023
Merged
@haircommander
server: allow Bidirectional mounts that contain storage root
28c3955 
previously we would never allow a container to get anything other than HostToContainer
for something that contains the storage root. This breaks situations where the container wishes to update
mount points within the container and see them propegated to the host.

Instead, we should only override this if the mount is Private.

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander haircommander force-pushed the storage-subdir-allow-bidirectional branch from 7c0ad17 to 28c3955 Compare October 24, 2023 19:54
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023
@haircommander
Copy link
Member Author

Since this was a regression. Can we get some unit or e2e/integration tests in crio itself to ensure we don't break again?

good idea, added one for both cases this covers

@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited

Codecov Report

Merging #7408 (28c3955) into main (31febae) will increase coverage by 0.13%.
Report is 50 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7408      +/-   ##
==========================================
+ Coverage   49.17%   49.31%   +0.13%     
==========================================
  Files         138      138              
  Lines       15707    15774      +67     
==========================================
+ Hits         7724     7779      +55     
- Misses       7055     7058       +3     
- Partials      928      937       +9

@haircommander
Copy link
Member Author

/retest

saschagrunert
saschagrunert approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm
/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023
@saschagrunert
Copy link
Member

Is that still WIP? Looks like the bot did not remove the do-not-merge/work-in-progress label.

@saschagrunert
Copy link
Member

/cherry-pick release-1.28

@openshift-cherrypick-robot
Copy link

@saschagrunert: once the present PR merges, I will cherry-pick it on top of release-1.28 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.28

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.

@saschagrunert
Copy link
Member

/retest

1 similar comment
@saschagrunert
Copy link
Member

/retest

@saschagrunert
Copy link
Member

Hm:

time="2023-10-25T10:50:00Z" level=fatal msg="Error pushing manifest list/index to registry: sha256:3e51591659d87627b650a38bbdde53412762eac0500ee7e68b53879f99f1e3c8: failed commit on ref \"index-image-registry.openshift-image-registry.svc:5000/ci-op-0t0zypqb/pipeline:src@sha256:3e51591659d87627b650a38bbdde53412762eac0500ee7e68b53879f99f1e3c8\": unexpected status: 500 Internal Server Error"

@haircommander haircommander removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2023
@kwilczynski
Copy link
Member

/retest-required

@kwilczynski
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, saschagrunert

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:
  • OWNERS [haircommander,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kwilczynski
Copy link
Member

/retest-required

@openshift-ci openshift-ci bot merged commit 95c42bd into cri-o:main Oct 31, 2023
55 of 57 checks passed
@openshift-cherrypick-robot
Copy link

@saschagrunert: new pull request created: #7456

In response to this:

/cherry-pick release-1.28

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-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 31, 2023
Merged
@kwilczynski
Copy link
Member

/cherry-pick release-1.27

@openshift-cherrypick-robot
Copy link

@kwilczynski: #7408 failed to apply on top of branch "release-1.27":

Applying: server: allow Bidirectional mounts that contain storage root
Using index info to reconstruct a base tree...
M	server/container_create_linux.go
M	test/ctr.bats
Falling back to patching base and 3-way merge...
Auto-merging test/ctr.bats
Auto-merging server/container_create_linux.go
CONFLICT (content): Merge conflict in server/container_create_linux.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 server: allow Bidirectional mounts that contain storage root
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.27

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.

@kwilczynski
Copy link
Member

OK. Will backport manually.

@kwilczynski kwilczynski mentioned this pull request Oct 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

Assignees

@saschagrunert saschagrunert

@harche harche

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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UnmountVolume failed
6 participants
@haircommander @harche @gnufied @saschagrunert @openshift-cherrypick-robot @kwilczynski