fix(scheduling): conditionally inject matchLabelKeys #2471
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmdeal 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 |
k8s-ci-robot
added
approved
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
| }) | ||
| } | ||
|
|
||
| type KubernetesVersionProvider interface { |
There was a problem hiding this comment.
Should this go in the version package? Seems a bit odd that this would land in the operator package to me
| nodehydration.NewController(kubeClient, cloudProvider), | ||
| } | ||
|
|
||
| if p, ok := kubeVersionProvider.(*version.Provider); ok { |
There was a problem hiding this comment.
Is there a better way to handle this by registering the controller on the operator.go side. It feels sort of odd to pass this through and then do static type checking like this to enable the controller mechanism
| nodeClaimStateController = informer.NewNodeClaimController(env.Client, cloudProvider, cluster) | ||
| recorder = test.NewEventRecorder() | ||
| prov = provisioning.NewProvisioner(env.Client, recorder, cloudProvider, cluster, fakeClock) | ||
| prov = provisioning.NewProvisioner(env.Client, env.KubernetesVersionProvider(), recorder, cloudProvider, cluster, fakeClock) |
There was a problem hiding this comment.
I could go either way, but why make this a method and not have this just be an interface that can be accessed directly
| return resource.MustParse(fmt.Sprintf("%dm", cpu[r.Intn(len(cpu))])) | ||
| } | ||
|
|
||
| type staticVersionProvider struct { |
There was a problem hiding this comment.
Should static version provider just be something that we put in the version package? We could share this across packages, including in the test environment
| } | ||
| } | ||
| marshaled := string(lo.Must(json.Marshal(tsc.LabelSelector.MatchExpressions))) | ||
| fmt.Printf("%s\n", marshaled) |
There was a problem hiding this comment.
I think you need to drop this print statement and the marshaling
|
Discussed offline, and agreed that we should figure out why the double-injected requirements are having the impact they are. In theory, it shouldn't matter if we've injected the same match expression twice into the selector term set. We've reverted 1.34 testing for the time being to unblock other PRs, and will reintroduce it once this PR is merged. |
|
Well this is interesting - it looks like we're probably running into mitchellh/hashstructure#36. I've been able to verify that the issue is caused by us getting the same hash for a |
|
I'm going to close this out in favor of #2479 - the two PRs are significantly different in approach. |
3 participants
Fixes #N/A
Description
Starting in k8s 1.34,
matchLabelKeysvalues are injected into match expressions by the API server. This matches the implementation for pod affinity. This PR adds a default version provider (overridable by the provider) which will allow us to conditionally disable our own injection on k8s versions 1.34 and greater.How was this change tested?
make testBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.