Skip to content

Conversation

@haircommander
Copy link
Member

- What I did
with a new version of cri-o, we are going to cleanup containers when a node reboots (which we detect when a tmpfs file is removed), and cleanup images when a node upgraded (which we detect how we used to: persistent version file out of date)
This PR changes where cri-o looks for those files. These match the upstream defaults, so when we move to drop in files, we won't need to specify these

- How to verify it
containers are cleared on node reboot, images are cleared when we bump a cri-o minor version (1.17->1.18)
- Description for the changelog

with a new version of cri-o, we are going to cleanup containers when a node reboots (which we detect when a tmpfs file is removed), and cleanup images when a node upgraded (which we detect how we used to: persistent version file out of date)
This PR changes where cri-o looks for those files. These match the upstream defaults, so when we move to drop in files, we won't need to specify these

Signed-off-by: Peter Hunt <[email protected]>
@openshift-ci-robot
Copy link
Contributor

@haircommander: This pull request references Bugzilla bug 1826896, which is invalid:

  • expected dependent Bugzilla bug 1826895 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is NEW instead

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

Details

In response to this:

Bug 1826896: [release-4.4] crio: set version-file{,-persist} correctly

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-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 23, 2020
@haircommander
Copy link
Member Author

/hold

needs cri-o/cri-o#3635 integrated into 4.4 first

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020
@kikisdeliveryservice kikisdeliveryservice self-requested a review April 23, 2020 21:28
@kikisdeliveryservice
Copy link
Contributor

@mrunalp PTAL

@mrunalp
Copy link
Member

mrunalp commented Apr 23, 2020

LGTM

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

approving sthis but will need lgtm and passing ci via a new rpm (which is all in progress)

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2020
@ashcrow
Copy link
Member

ashcrow commented Apr 25, 2020

Updated cri-o will be pinned in the boot image via openshift/installer#3508

@mrunalp
Copy link
Member

mrunalp commented Apr 25, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kikisdeliveryservice, mrunalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [kikisdeliveryservice]

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

@bparees bparees added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Apr 25, 2020
@kikisdeliveryservice
Copy link
Contributor

Hold to be lifted by @ashcrow & @mrunalp in the morning after we rekick ci and verify with promoted image.

@ashcrow
Copy link
Member

ashcrow commented Apr 25, 2020

/retest

@ashcrow
Copy link
Member

ashcrow commented Apr 25, 2020

/retest

2 similar comments
@ashcrow
Copy link
Member

ashcrow commented Apr 25, 2020

/retest

@ashcrow
Copy link
Member

ashcrow commented Apr 25, 2020

/retest

@cgwalters
Copy link
Member

containers are cleared on node reboot, images are cleared when we bump a cri-o minor version (1.17->1.18)

Clearing containers on reboot makes total sense for cri-o. But...clearing images conflicts with the messaging in openshift/machine-api-operator#565 to some degree. The story still holds for minor updates of course. Ideally at some point we have rigorous enough testing for the containers/image layer across upgrades that we don't need to do this (or, we only do it for actually breaking changes to the image store).

@mrunalp mrunalp removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2020
@mrunalp
Copy link
Member

mrunalp commented Apr 25, 2020

Ideally at some point we have rigorous enough testing for the containers/image layer across upgrades that we don't need to do this (or, we only do it for actually breaking changes to the image store).

Yes, agree with this 👍 With rigorous enough testing we can rely on the kubelet to gc older images.

@mrunalp mrunalp added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2020
@kikisdeliveryservice
Copy link
Contributor

used old images

/retest

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. and removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 25, 2020
@openshift-ci-robot
Copy link
Contributor

@haircommander: This pull request references Bugzilla bug 1826896, which is invalid:

  • expected dependent Bugzilla bug 1826895 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

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

Details

In response to this:

Bug 1826896: [release-4.4] crio: set version-file{,-persist} correctly

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-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 25, 2020
@mrunalp mrunalp removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Apr 25, 2020
@wking wking added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 25, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 25, 2020
@openshift-ci-robot
Copy link
Contributor

@haircommander: This pull request references Bugzilla bug 1826896, which is invalid:

  • expected dependent Bugzilla bug 1826895 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

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

Details

In response to this:

Bug 1826896: [release-4.4] crio: set version-file{,-persist} correctly

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.

@mrunalp mrunalp added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Apr 25, 2020
@kikisdeliveryservice
Copy link
Contributor

/test e2e-gcp-upgrade

@kikisdeliveryservice
Copy link
Contributor

level=fatal msg="failed to fetch Cluster: failed to fetch dependency of \"Cluster\": failed to generate asset \"Platform Permissions Check\": validate AWS credentials: checking install permissions: error simulating policy: Throttling: Rate exceeded\n\tstatus code: 400, request id: 240145ea-661f-46ef-8699-cdb968f7d7f4"

/test e2e-aws-scaleup-rhel7

@kikisdeliveryservice
Copy link
Contributor

waiting on e2e-gcp-op & e2e-gcp-upgrade to finish..

rhel7 job (not reqd) seems to be going through something today. ¯_(ツ)_/¯

@kikisdeliveryservice
Copy link
Contributor

/skip

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 26, 2020

@haircommander: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 c280fa6 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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

@kikisdeliveryservice
Copy link
Contributor

/retest

@mrunalp mrunalp removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2020
@openshift-merge-robot openshift-merge-robot merged commit c83f295 into openshift:release-4.4 Apr 26, 2020
@openshift-ci-robot
Copy link
Contributor

@haircommander: All pull requests linked via external trackers have merged: cri-o/cri-o#3635, openshift/installer#3508, openshift/machine-config-operator#1679. Bugzilla bug 1826896 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1826896: [release-4.4] crio: set version-file{,-persist} correctly

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants