Add context-based warning suppression to SetEmulationVersion #133207

PersistentJZH · 2025-07-25T06:41:19Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

implement a context logger to optimize the warning logic

Which issue(s) this PR is related to:

Fix: #133056

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla · 2025-07-25T06:41:23Z

The committers listed above are authorized under a signed CLA.

✅ login: PersistentJZH (463f95f)

k8s-ci-robot · 2025-07-25T06:41:28Z

Welcome @PersistentJZH!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-07-25T06:41:28Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2025-07-25T06:41:29Z

Hi @PersistentJZH. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2025-07-25T06:41:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PersistentJZH
Once this PR has been reviewed and has the lgtm label, please assign msau42 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:

staging/src/k8s.io/component-base/featuregate/OWNERS

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

k8s-triage-robot · 2025-07-25T07:15:50Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

BenTheElder · 2025-07-29T20:51:33Z

/cc @Jefftree

Jefftree · 2025-07-29T21:13:16Z

/ok-to-test
/cc @michaelasp

michaelasp · 2025-07-29T21:16:51Z

staging/src/k8s.io/component-base/featuregate/feature_gate.go

@@ -165,6 +165,8 @@ type MutableVersionedFeatureGate interface {
 	// Otherwise, the emulationVersion will be the same as the binary version.
 	// If set, the feature defaults and availability will be as if the binary is at the emulated version.
 	SetEmulationVersion(emulationVersion *version.Version) error
+	// SetEmulationVersionWithContext is like SetEmulationVersion but accepts a context for controlling behavior.


Should still describe the behavior in the function description, other functions may get changed or removed.

ok, will describe the behavior.

michaelasp · 2025-07-29T21:29:55Z

staging/src/k8s.io/component-base/featuregate/feature_gate.go

+type testLoggerKey struct{}
+
+// WithWarningSuppressionContext returns a context with warning suppression flag
+func WithWarningSuppressionContext(ctx context.Context) context.Context {


Imo I think we should have a different method of suppressing the warning than passing the value in the context. We shouldn't really have a scenario outside of testing where this isn't logged. @pohly's suggestion of passing a per test logger for this makes sense.

Possibly even we should error on this if we don't pass some allow override since we are setting an already set flag which is unusual during normal operation.

I think we should likely go with passing in a logger for now and follow up on what to do if the emulated version is set multiple times for a feature.

thanks for @michaelasp's advice. I will use a context logger to optimize the logic and report an error if don't pass some allow override.

michaelasp · 2025-07-29T21:32:44Z

staging/src/k8s.io/component-base/featuregate/testing/feature_gate.go

-		if err := gate.(featuregate.MutableVersionedFeatureGate).SetEmulationVersion(originalEmuVer); err != nil {
+		// Use context to suppress warnings during test cleanup
+		ctx := featuregate.WithWarningSuppressionContext(context.Background())
+		if err := gate.(featuregate.MutableVersionedFeatureGate).SetEmulationVersionWithContext(ctx, originalEmuVer); err != nil {


This is probably where we can pass some sort of logger that isn't spammy, we expect the values to be changed here so we could probably log to somewhere outside of the base klog.

…minate test noise Signed-off-by: zhihao jian <[email protected]> optimize SetEmulationVersionWithContext logic

PersistentJZH · 2025-07-30T11:16:45Z

@michaelasp thanks for code review. I have resolved all comments above, pls help to review again, thank you !

liggitt · 2025-07-30T13:28:55Z

staging/src/k8s.io/component-base/featuregate/feature_gate.go

+	// warnings will be redirected to the test output instead of the global klog.
+	// If features would change and no test logger is provided, an error will be returned to prevent
+	// unexpected behavior in production environments.
+	SetEmulationVersionWithContext(ctx context.Context, emulationVersion *version.Version) error


I think adding a WithContext version here is really confusing ... can we do something more scoped to the single integration test call site that will use this that doesn't expand the interface for all callers?

yup, add SetEmulationVersionWithContext func here is confusing, but there doesn't seem to be a more elegant way to do this.
Is it possible to use klog.FromContext(ctx) to implement different loggers for different envs? However, even if we use klog.FromContext(ctx), we still need an additional parameter or additional method to pass context info.

eg: change SetEmulationVersion(emulationVersion *version.Version) error to SetEmulationVersion(ctx context.Context, emulationVersion *version.Version) error

The changes here are very large and all callers need to be changed, but it is not impossible to do so. Do you have any idea about this? @liggitt

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 25, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 25, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 25, 2025

k8s-ci-robot requested review from derekwaynecarr and liggitt July 25, 2025 06:41

k8s-ci-robot requested a review from Jefftree July 29, 2025 20:51

k8s-ci-robot requested a review from michaelasp July 29, 2025 21:13

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 29, 2025

michaelasp reviewed Jul 29, 2025

View reviewed changes

added context-based warning suppression to SetEmulationVersion to eli…

463f95f

…minate test noise Signed-off-by: zhihao jian <[email protected]> optimize SetEmulationVersionWithContext logic

PersistentJZH force-pushed the feat/enhance-warning-in-set-emulation-version branch from adbd958 to 463f95f Compare July 30, 2025 10:51

liggitt reviewed Jul 30, 2025

View reviewed changes

Add context-based warning suppression to SetEmulationVersion #133207

Are you sure you want to change the base?

Add context-based warning suppression to SetEmulationVersion #133207

Uh oh!

Conversation

PersistentJZH commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

linux-foundation-easycla bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

k8s-triage-robot commented Jul 25, 2025

Uh oh!

BenTheElder commented Jul 29, 2025

Uh oh!

Jefftree commented Jul 29, 2025

Uh oh!

michaelasp Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

PersistentJZH Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

michaelasp Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

PersistentJZH Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

michaelasp Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

PersistentJZH commented Jul 30, 2025

Uh oh!

liggitt Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

PersistentJZH Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PersistentJZH commented Jul 25, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jul 25, 2025 •

edited

Loading

PersistentJZH Jul 30, 2025 •

edited

Loading