Skip to content

[release-1.26] OCPBUGS-45908: Only restore container if all bind mounts are defined #8803

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
Closed
wants to merge 3 commits into from
Closed

[release-1.26] OCPBUGS-45908: Only restore container if all bind mounts are defined #8803

wants to merge 3 commits into from

Conversation

@kwilczynski
Copy link
Contributor

@kwilczynski kwilczynski commented Dec 2, 2024 •
edited
Loading

This is a manual cherry-pick of commit 429ef7c

commit 429ef7c366406280cfa3daa15812d14b22953a7a
Author: Adrian Reber <[email protected]>
Date:   Mon Sep 30 23:37:13 2024

    Only restore container if all bind mounts are defined
    
    To avoid the situation where a container that is restored via a registry
    mounts unexpected host paths into the container, this changes the
    restore behaviour of CRI-O.
    
    Previously all bind mounted paths in the original container which were
    defined for example like this:
    
        volumeMounts:
        - mountPath: /data
          name: data-volume
      volumes:
      - name: data-volume
        hostPath:
          path: /srv/container/data
    
    Were automatically mounted in the restored container without and
    definition necessary. This lead to the situation that the user does not
    know which path will be mounted if starting a restored container.
    
    Now CRI-O will refuse to restore a container if not all bind mounts are
    defined via the CRI CreateContainer RPC in the CreateContainerRequest
    message.
    
    CRI-O will now return an error that will look something likes this:
    
    Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)
    
    Now the user has to explicitly add those bind mounts in the same way as
    it was done during initial container creation.
    
    Signed-off-by: Adrian Reber <[email protected]>

/assign kwilczynski

Note

This cherry-pickl brings the following Pull Requests as a dependency:

- Only restore container if all bind mounts are defined.
- Ensure that the "image" attribute has been provided.

Krzysztof Wilczyński and others added 3 commits December 2, 2024 16:13
Check for nil values when importing container definition
4b5a25d 
Also, ensure that the "image" attribute has been provided.

Signed-off-by: Krzysztof Wilczyński <[email protected]>
@adrianreber
Only restore container if all bind mounts are defined
82b3d4b 
To avoid the situation where a container that is restored via a registry
mounts unexpected host paths into the container, this changes the
restore behaviour of CRI-O.

Previously all bind mounted paths in the original container which were
defined for example like this:

    volumeMounts:
    - mountPath: /data
      name: data-volume
  volumes:
  - name: data-volume
    hostPath:
      path: /srv/container/data

Were automatically mounted in the restored container without and
definition necessary. This lead to the situation that the user does not
know which path will be mounted if starting a restored container.

Now CRI-O will refuse to restore a container if not all bind mounts are
defined via the CRI CreateContainer RPC in the CreateContainerRequest
message.

CRI-O will now return an error that will look something likes this:

Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)

Now the user has to explicitly add those bind mounts in the same way as
it was done during initial container creation.

Signed-off-by: Adrian Reber <[email protected]>
@saschagrunert
Fix container restore lint report
12230e1 
Signed-off-by: Sascha Grunert <[email protected]>
@kwilczynski kwilczynski requested a review from mrunalp as a code owner December 2, 2024 07:53
@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 2, 2024
@openshift-ci openshift-ci bot requested review from QiWang19 and klihub December 2, 2024 07:53
@kwilczynski kwilczynski changed the title Only restore container if all bind mounts are defined [release-1.26] Only restore container if all bind mounts are defined Dec 2, 2024
@kwilczynski kwilczynski changed the title [release-1.26] Only restore container if all bind mounts are defined [release-1.26] OCPBUGS-45908: Only restore container if all bind mounts are defined Dec 10, 2024
@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Dec 10, 2024
@openshift-ci-robot
Copy link

@kwilczynski: This pull request references Jira Issue OCPBUGS-45908, which is invalid:

  • release note text must be set and not match the template OR release note type must be set to "Release Note Not Required". For more information you can reference the OpenShift Bug Process.
  • expected Jira Issue OCPBUGS-45908 to depend on a bug in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This is a manual cherry-pick of commit 429ef7c

commit 429ef7c366406280cfa3daa15812d14b22953a7a
Author: Adrian Reber <[email protected]>
Date:   Mon Sep 30 23:37:13 2024

   Only restore container if all bind mounts are defined
   
   To avoid the situation where a container that is restored via a registry
   mounts unexpected host paths into the container, this changes the
   restore behaviour of CRI-O.
   
   Previously all bind mounted paths in the original container which were
   defined for example like this:
   
       volumeMounts:
       - mountPath: /data
         name: data-volume
     volumes:
     - name: data-volume
       hostPath:
         path: /srv/container/data
   
   Were automatically mounted in the restored container without and
   definition necessary. This lead to the situation that the user does not
   know which path will be mounted if starting a restored container.
   
   Now CRI-O will refuse to restore a container if not all bind mounts are
   defined via the CRI CreateContainer RPC in the CreateContainerRequest
   message.
   
   CRI-O will now return an error that will look something likes this:
   
   Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)
   
   Now the user has to explicitly add those bind mounts in the same way as
   it was done during initial container creation.
   
   Signed-off-by: Adrian Reber <[email protected]>

/assign kwilczynski

[!NOTE]
This cherry-pickl brings the following Pull Requests as a dependency:

- Only restore container if all bind mounts are defined.
- Ensure that the "image" attribute has been provided.

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 openshift-eng/jira-lifecycle-plugin repository.

@kwilczynski
Copy link
Contributor Author

/cc @cri-o/cri-o-maintainers

@openshift-ci openshift-ci bot requested a review from a team December 12, 2024 10:59
@kwilczynski
Copy link
Contributor Author

/retest

1 similar comment
@kwilczynski
Copy link
Contributor Author

/retest

@kwilczynski
Copy link
Contributor Author

/close

@openshift-ci openshift-ci bot closed this Jan 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

@kwilczynski: Closed this PR.

In response to this:

/close

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-sigs/prow repository.

@openshift-ci-robot
Copy link

@kwilczynski: This pull request references Jira Issue OCPBUGS-45908. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

This is a manual cherry-pick of commit 429ef7c

commit 429ef7c366406280cfa3daa15812d14b22953a7a
Author: Adrian Reber <[email protected]>
Date:   Mon Sep 30 23:37:13 2024

   Only restore container if all bind mounts are defined
   
   To avoid the situation where a container that is restored via a registry
   mounts unexpected host paths into the container, this changes the
   restore behaviour of CRI-O.
   
   Previously all bind mounted paths in the original container which were
   defined for example like this:
   
       volumeMounts:
       - mountPath: /data
         name: data-volume
     volumes:
     - name: data-volume
       hostPath:
         path: /srv/container/data
   
   Were automatically mounted in the restored container without and
   definition necessary. This lead to the situation that the user does not
   know which path will be mounted if starting a restored container.
   
   Now CRI-O will refuse to restore a container if not all bind mounts are
   defined via the CRI CreateContainer RPC in the CreateContainerRequest
   message.
   
   CRI-O will now return an error that will look something likes this:
   
   Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)
   
   Now the user has to explicitly add those bind mounts in the same way as
   it was done during initial container creation.
   
   Signed-off-by: Adrian Reber <[email protected]>

/assign kwilczynski

[!NOTE]
This cherry-pickl brings the following Pull Requests as a dependency:

- Only restore container if all bind mounts are defined.
- Ensure that the "image" attribute has been provided.

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 openshift-eng/jira-lifecycle-plugin repository.

@kwilczynski
Copy link
Contributor Author

/reopen

@openshift-ci openshift-ci bot reopened this Jan 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

@kwilczynski: Reopened this PR.

In response to this:

/reopen

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-sigs/prow repository.

@openshift-ci-robot
Copy link

@kwilczynski: This pull request references Jira Issue OCPBUGS-45908, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is Closed (Not a Bug) instead
  • release note text must be set and not match the template OR release note type must be set to "Release Note Not Required". For more information you can reference the OpenShift Bug Process.
  • expected Jira Issue OCPBUGS-45908 to depend on a bug in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This is a manual cherry-pick of commit 429ef7c

commit 429ef7c366406280cfa3daa15812d14b22953a7a
Author: Adrian Reber <[email protected]>
Date:   Mon Sep 30 23:37:13 2024

   Only restore container if all bind mounts are defined
   
   To avoid the situation where a container that is restored via a registry
   mounts unexpected host paths into the container, this changes the
   restore behaviour of CRI-O.
   
   Previously all bind mounted paths in the original container which were
   defined for example like this:
   
       volumeMounts:
       - mountPath: /data
         name: data-volume
     volumes:
     - name: data-volume
       hostPath:
         path: /srv/container/data
   
   Were automatically mounted in the restored container without and
   definition necessary. This lead to the situation that the user does not
   know which path will be mounted if starting a restored container.
   
   Now CRI-O will refuse to restore a container if not all bind mounts are
   defined via the CRI CreateContainer RPC in the CreateContainerRequest
   message.
   
   CRI-O will now return an error that will look something likes this:
   
   Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)
   
   Now the user has to explicitly add those bind mounts in the same way as
   it was done during initial container creation.
   
   Signed-off-by: Adrian Reber <[email protected]>

/assign kwilczynski

[!NOTE]
This cherry-pickl brings the following Pull Requests as a dependency:

- Only restore container if all bind mounts are defined.
- Ensure that the "image" attribute has been provided.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kwilczynski
Once this PR has been reviewed and has the lgtm label, please assign fidencio for approval. For more information see the Code Review Process.

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2025

@kwilczynski: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp 12230e1 link true /test e2e-gcp
ci/prow/e2e-agnostic 12230e1 link true /test e2e-agnostic
ci/prow/images 12230e1 link true /test images

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@kwilczynski
Copy link
Contributor Author

/cc @cri-o/cri-o-maintainers
/cc @haircommander
/cc @sohankunkerkar

@haircommander
Copy link
Member

personally I think this can be closed. It's out of upstream support and not used downstream

@kwilczynski
Copy link
Contributor Author

personally I think this can be closed. It's out of upstream support and not used downstream

@haircommander makes sense! Thank you!

@kwilczynski
Copy link
Contributor Author

Closing. This backport is no longer needed.

@kwilczynski
Copy link
Contributor Author

/close

@openshift-ci openshift-ci bot closed this Jan 15, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 15, 2025

@kwilczynski: Closed this PR.

In response to this:

/close

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-sigs/prow repository.

@openshift-ci-robot
Copy link

@kwilczynski: This pull request references Jira Issue OCPBUGS-45908. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

This is a manual cherry-pick of commit 429ef7c

commit 429ef7c366406280cfa3daa15812d14b22953a7a
Author: Adrian Reber <[email protected]>
Date:   Mon Sep 30 23:37:13 2024

   Only restore container if all bind mounts are defined
   
   To avoid the situation where a container that is restored via a registry
   mounts unexpected host paths into the container, this changes the
   restore behaviour of CRI-O.
   
   Previously all bind mounted paths in the original container which were
   defined for example like this:
   
       volumeMounts:
       - mountPath: /data
         name: data-volume
     volumes:
     - name: data-volume
       hostPath:
         path: /srv/container/data
   
   Were automatically mounted in the restored container without and
   definition necessary. This lead to the situation that the user does not
   know which path will be mounted if starting a restored container.
   
   Now CRI-O will refuse to restore a container if not all bind mounts are
   defined via the CRI CreateContainer RPC in the CreateContainerRequest
   message.
   
   CRI-O will now return an error that will look something likes this:
   
   Error: the container to restore (7f...be) expects following bind mounts defined (/data,/data2)
   
   Now the user has to explicitly add those bind mounts in the same way as
   it was done during initial container creation.
   
   Signed-off-by: Adrian Reber <[email protected]>

/assign kwilczynski

[!NOTE]
This cherry-pickl brings the following Pull Requests as a dependency:

- Only restore container if all bind mounts are defined.
- Ensure that the "image" attribute has been provided.

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 openshift-eng/jira-lifecycle-plugin repository.

@kwilczynski kwilczynski deleted the feature/backport-commit-429ef7c36-to-release-1.26 branch January 15, 2025 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@klihub klihub Awaiting requested review from klihub

@QiWang19 QiWang19 Awaiting requested review from QiWang19

@haircommander haircommander Awaiting requested review from haircommander

@sohankunkerkar sohankunkerkar Awaiting requested review from sohankunkerkar

Assignees

@kwilczynski kwilczynski

Labels

dco-signoff: yes Indicates the PR's author has DCO signed all their commits. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. 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.

5 participants

@kwilczynski @openshift-ci-robot @haircommander @adrianreber @saschagrunert