Skip to content

Conversation

@bitoku
Copy link
Contributor

@bitoku bitoku commented Mar 29, 2025

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?

Added container stop signal feature (KEP-4960).

@bitoku bitoku requested a review from mrunalp as a code owner March 29, 2025 19:02
@openshift-ci openshift-ci bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 29, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2025
@openshift-ci openshift-ci bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Mar 29, 2025
@openshift-ci openshift-ci bot requested review from QiWang19 and littlejawa March 29, 2025 19:02
@bitoku bitoku force-pushed the stop-container-signal branch from 9a99fe7 to 78010ec Compare March 29, 2025 19:03
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Mar 29, 2025
@bitoku bitoku force-pushed the stop-container-signal branch from 78010ec to 52fbc90 Compare March 29, 2025 19:07
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2025
@bitoku bitoku force-pushed the stop-container-signal branch from 52fbc90 to 675bf8a Compare March 29, 2025 19:10
@codecov
Copy link

codecov bot commented Mar 29, 2025

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Project coverage is 47.02%. Comparing base (3e6b6cf) to head (6a4a3ee).
Report is 7 commits behind head on main.

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bitoku bitoku force-pushed the stop-container-signal branch 2 times, most recently from 26a7643 to 2a57e60 Compare March 29, 2025 22:21
Signed-off-by: Ayato Tokubi <[email protected]>
@bitoku bitoku force-pushed the stop-container-signal branch from 2a57e60 to 6a4a3ee Compare March 29, 2025 23:10
@bitoku bitoku changed the title Add container stop signal feature (KEP-4960) OCPNODE-3062: Add container stop signal feature (KEP-4960) Mar 31, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 31, 2025
@openshift-ci-robot
Copy link

@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:

What type of PR is this?

/kind api-change

What this PR does / why we need it:

This PR implements feature introduced in containerd/containerd#11617

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?

Added container stop signal feature (KEP-4960).

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.

@bitoku
Copy link
Contributor Author

bitoku commented Mar 31, 2025

@cri-o/cri-o-maintainers Can you PTAL?

@bitoku
Copy link
Contributor Author

bitoku commented Apr 7, 2025

@cri-o/cri-o-maintainers PTAL ? thanks

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.

Changes LGTM, just a question about the follow-up.

Comment on lines +12 to +14
// 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")
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saschagrunert
Copy link
Member

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2025

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2025
@bitoku
Copy link
Contributor Author

bitoku commented Apr 7, 2025

/override ci/prow/ci-e2e-evented-pleg

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2025

@bitoku: bitoku unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:.

In response to this:

/override ci/prow/ci-e2e-evented-pleg

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.

@bitoku
Copy link
Contributor Author

bitoku commented Apr 8, 2025

@cri-o/cri-o-admins Can you override ci/prow/ci-e2e-evented-pleg ?

@saschagrunert
Copy link
Member

/override ci/prow/ci-e2e-evented-pleg

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 8, 2025

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-e2e-evented-pleg

In response to this:

/override ci/prow/ci-e2e-evented-pleg

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-merge-bot openshift-merge-bot bot merged commit 07a1e0e into cri-o:main Apr 8, 2025
74 of 78 checks passed
- name: install kata
include_tasks: "build/kata.yml"
when: "build_kata | default(False) | bool"

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Member

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 🙂

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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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

Development

Successfully merging this pull request may close these issues.

5 participants