-
Couldn't load subscription status.
- Fork 1.1k
OCPNODE-3062: Add container stop signal feature (KEP-4960) #9086
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
Conversation
9a99fe7 to
78010ec
Compare
78010ec to
52fbc90
Compare
52fbc90 to
675bf8a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9086 +/- ##
==========================================
- Coverage 47.03% 47.02% -0.01%
==========================================
Files 160 161 +1
Lines 23608 23617 +9
==========================================
+ Hits 11104 11106 +2
- Misses 11407 11413 +6
- Partials 1097 1098 +1 🚀 New features to boost your workflow:
|
Signed-off-by: Ayato Tokubi <[email protected]>
26a7643 to
2a57e60
Compare
Signed-off-by: Ayato Tokubi <[email protected]>
2a57e60 to
6a4a3ee
Compare
|
@bitoku: This pull request references OCPNODE-3062 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
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. |
|
@cri-o/cri-o-maintainers Can you PTAL? |
|
@cri-o/cri-o-maintainers PTAL ? thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, just a question about the follow-up.
| // TODO: implement this function | ||
| // https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/1287-in-place-update-pod-resources/README.md#cri-changes | ||
| return nil, errors.New("not implemented yet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to follow-up on that or how should we handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can follow up.
Since the KEP is going to be beta in 1.33, we should implement this anyway, right?
I'll create a JIRA ticket about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, 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:
Approvers can indicate their approval by writing |
|
/override ci/prow/ci-e2e-evented-pleg |
|
@bitoku: bitoku unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:. In response to this:
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. |
|
@cri-o/cri-o-admins Can you override ci/prow/ci-e2e-evented-pleg ? |
|
/override ci/prow/ci-e2e-evented-pleg |
|
@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-e2e-evented-pleg In response to this:
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. |
| - name: install kata | ||
| include_tasks: "build/kata.yml" | ||
| when: "build_kata | default(False) | bool" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we've upated the cri-tools in the VM image we should undo this change so we continue to use the (new) cached version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious why do we want to remove the building cri-tools step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do it as part of setup which runs as a periodic job that we then cache the images for. basically: we update the images daily instead of doing it each ci run. The upstream doesn't change enough to warrant rebuilding each PR and spending all that download/build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haircommander
aha, I should've waited for a while. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no this is fine to force a rebuild temporarily, no worries 🙂
What type of PR is this?
/kind api-change
What this PR does / why we need it:
This PR implements feature introduced in kubernetes/kubernetes#130556
Which issue(s) this PR fixes:
Special notes for your reviewer:
By updating cri-api, it needs implementing UpdatePodSandboxResources.
I wrote just a placeholder for it which always return error. Should we implement this first?
Does this PR introduce a user-facing change?