-
Notifications
You must be signed in to change notification settings - Fork 772
[tmpnet] Source tmpnet defaults from a configmap #4057
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
base: master
Are you sure you want to change the base?
Conversation
Previously the scheduling label and value required to enable exclusive scheduling were defined as flag defaults. To enable the cluster to define these defaults, the defaults are now sourced from a configmap in the target namespace.
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.
Pull Request Overview
This PR centralizes tmpnet scheduling defaults in a Kubernetes ConfigMap rather than hardcoded flag defaults.
- Updated
EnsureDefaultConfig
to accept acontext.Context
and delegate to Kube-specific defaulting - Added
KubeRuntimeConfig.ensureDefaults
to fetch scheduling labels from atmpnet
ConfigMap - Removed static flag defaults and manual validation for scheduling labels
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/fixture/tmpnet/network_test.go | Updated test to pass ctx into EnsureDefaultConfig |
tests/fixture/tmpnet/network.go | Changed EnsureDefaultConfig signature and call sites to include ctx |
tests/fixture/tmpnet/kube_runtime.go | Added ensureDefaults method to source scheduling defaults from ConfigMap |
tests/fixture/tmpnet/flags/kube_runtime.go | Removed hardcoded flag defaults and validation for scheduling labels |
Comments suppressed due to low confidence (4)
tests/fixture/tmpnet/kube_runtime.go:58
- [nitpick] This error message references flag names, but it can also be triggered by missing keys in the ConfigMap. Consider clarifying that missing config-map entries will produce this error.
var errMissingSchedulingLabels = errors.New("--kube-scheduling-label-key and --kube-scheduling-label-value are required when exclusive scheduling is enabled")
tests/fixture/tmpnet/flags/kube_runtime.go:79
- [nitpick] Since defaults now come from a ConfigMap, update the flag usage text to mention that no in-code default is set and that users should configure the
tmpnet
ConfigMap.
"",
tests/fixture/tmpnet/kube_runtime.go:82
- Add unit tests for
ensureDefaults
to verify behavior when the ConfigMap contains valid defaults and when keys are missing.
func (c *KubeRuntimeConfig) ensureDefaults(ctx context.Context, log logging.Logger) error {
tests/fixture/tmpnet/network_test.go:29
- It looks like
ctx
isn’t defined in this test. Consider addingctx := context.Background()
and importing thecontext
package at the top of the file.
require.NoError(network.EnsureDefaultConfig(ctx, logging.NoLog{}))
24434e4
to
a16b090
Compare
if len(c.SchedulingLabelKey) == 0 && len(schedulingLabelKey) > 0 { | ||
log.Info("setting default value for SchedulingLabelKey", | ||
zap.String("schedulingLabelKey", schedulingLabelKey), | ||
) | ||
c.SchedulingLabelKey = schedulingLabelKey | ||
} | ||
if len(c.SchedulingLabelValue) == 0 && len(schedulingLabelValue) > 0 { | ||
log.Info("setting default value for SchedulingLabelValue", | ||
zap.String("schedulingLabelValue", schedulingLabelValue), | ||
) | ||
c.SchedulingLabelValue = schedulingLabelValue | ||
} |
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.
Is there a benefit to mix-and-matching between flag values and ConfigMap values here? I would think that the KubeRuntimeConfig
would go with exclusively with flag values or exclusively with ConfigMap values.
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.
Configmap values are defaults and only used when a flag has an empty value (as per the logging communicating their usage). The intention is to ensure that a cluster can provide appropriate defaults rather than to prevent a user from supplying values via flags.
Why this should be merged
Previously the scheduling label and value required to enable exclusive scheduling were defined as flag defaults. To enable the cluster to define these defaults, the defaults are now sourced from a configmap in the target namespace.
How this was tested
CI, manually
Need to be documented in RELEASES.md?
N/A