Skip to content

Feat/checkpoint bugfix#275

Merged
furykerry merged 1 commit intoopenkruise:masterfrom
BH4AWS:feat/checkpoint_bugfix
Apr 28, 2026
Merged

Feat/checkpoint bugfix#275
furykerry merged 1 commit intoopenkruise:masterfrom
BH4AWS:feat/checkpoint_bugfix

Conversation

@BH4AWS
Copy link
Copy Markdown
Contributor

@BH4AWS BH4AWS commented Apr 15, 2026

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 15, 2026

Codecov Report

❌ Patch coverage is 54.54545% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.01%. Comparing base (b9d8f27) to head (e754cbc).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/utils/runtime/runtime.go 0.00% 25 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   73.17%   73.01%   -0.17%     
==========================================
  Files         125      126       +1     
  Lines        9336     9382      +46     
==========================================
+ Hits         6832     6850      +18     
- Misses       2183     2210      +27     
- Partials      321      322       +1     
Flag Coverage Δ
unittests 73.01% <54.54%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch 2 times, most recently from 226181e to cb10e08 Compare April 16, 2026 15:49
@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch from cb10e08 to 8df498e Compare April 21, 2026 05:27
Copy link
Copy Markdown
Member

@AiRanthem AiRanthem left a comment

Choose a reason for hiding this comment

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

/lgtm

@AiRanthem
Copy link
Copy Markdown
Member

AiRanthem commented Apr 24, 2026

There's a small issue found by Qoder:

Issue #1: propagateAnnotationsToTemplate may unnecessarily mutate template annotations

Attribute Details
Location pkg/sandbox-manager/infra/sandboxcr/utils.go:80-86
Category Logic / Safety

Description:

propagateAnnotationsToTemplate calls sbx.GetAnnotations() which can return nil. While Go allows indexing a nil map (returns zero value, no panic), the function always calls template.SetAnnotations(tmplAnnotations) even when no necessary keys were found, replacing the existing nil with an empty map. For CreateCheckpoint, this creates a template with an empty annotations map instead of nil, which is benign but slightly wasteful.

Fix:

Consider returning early if no necessary keys are found in sandbox annotations:

if sbxAnnotations == nil {
    return
}

@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch from 8df498e to e4b0c7d Compare April 25, 2026 07:17
@kruise-bot kruise-bot removed the lgtm label Apr 25, 2026
@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch 3 times, most recently from 8b34339 to 9536bea Compare April 28, 2026 05:36
Comment thread pkg/sandbox-manager/infra/sandboxcr/clone.go Outdated
Comment thread pkg/sandbox-manager/infra/sandboxcr/utils.go Outdated
@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch from 9536bea to e754cbc Compare April 28, 2026 08:49
Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
@BH4AWS BH4AWS force-pushed the feat/checkpoint_bugfix branch from e754cbc to 856e8de Compare April 28, 2026 12:59
Copy link
Copy Markdown
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kruise-bot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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:

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

@furykerry furykerry merged commit 7fa95c4 into openkruise:master Apr 28, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants