Skip to content

Conversation

@estroz
Copy link

@estroz estroz commented Oct 24, 2025

Fixes #702

@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 24, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jgehrcke
Copy link
Collaborator

Instantiating the feature gate singleton only if there's actually a consumer: appears to be a tidy approach. @estroz you tested that this actually fixes #702 for you, right?

We have identified an invariant here that want to maintain. Hence, we may want to add a pragmatic test to make sure that we don't regress from here. We can do that in another PR.

@klueska
Copy link
Collaborator

klueska commented Oct 25, 2025

There was a desire in the initial design to not have to call a function, but rather just access a global variable from the featuregate package (similar to as is done in k8s proper). I'm not 100% opposed to this approach, but I would want to then remove the global FeatureGates var and then call the function just FeatureGates() instead of GetFeatureGates().

@estroz
Copy link
Author

estroz commented Oct 27, 2025

@jgehrcke yes I have confirmed this fixes the panic for type imports.

@klueska renamed the function and accepted the suggestion, thanks.

@klueska
Copy link
Collaborator

klueska commented Oct 28, 2025

I think we can still get away with having FeatureGates be a global variable instead of a function. We just need to implement its methods on its pointer type and init it with the once iff the incoming pointer is nil and equals the address of the global FeatureGates variable.

@estroz
Copy link
Author

estroz commented Nov 17, 2025

@klueska that won't work because packages importing and using FeatureGate directly can invoke the component-base's featuregate.MutableVersionedFeatureGate methods that are not implemented in this module, which will panic.

@klueska klueska added this to the v25.12.0 milestone Nov 24, 2025
@klueska klueska self-assigned this Nov 24, 2025
@klueska
Copy link
Collaborator

klueska commented Nov 26, 2025

Sorry for the delay on this. Between vacation and conferences we've let this PR sit open much longer than it should have.

Given the following, I'd say let's go ahead and merge this as is -- if we think of a way around this in the future we can implement it, but it's not worth blocking this PR any further:

@klueska that won't work because packages importing and using FeatureGate directly can invoke the component-base's featuregate.MutableVersionedFeatureGate methods that are not implemented in this module, which will panic.

@klueska
Copy link
Collaborator

klueska commented Nov 26, 2025

/ok to test 45967ec

@klueska klueska merged commit 49019ab into NVIDIA:main Nov 26, 2025
20 of 21 checks passed
@jgehrcke
Copy link
Collaborator

/cherry-pick release-25.8

@github-actions
Copy link

🤖 Backport PR created for release-25.8: #749

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

Projects

Development

Successfully merging this pull request may close these issues.

Panic in pkg/featureflag when importing API types

4 participants