Skip to content

Commit 758c25d

Browse files
andrewsykimandrewsykim
committed
service controller: store feature gate in local fields for better testability
Signed-off-by: Andrew Sy Kim <[email protected]>
1 parent b3419e0 commit 758c25d

File tree

5 files changed

+67
-30
lines changed

5 files changed

+67
-30
lines changed

‎cmd/cloud-controller-manager/app/core.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ func startServiceController(ctx *cloudcontrollerconfig.CompletedConfig, cloud cl
8989
ctx.SharedInformers.Core().V1().Services(),
9090
ctx.SharedInformers.Core().V1().Nodes(),
9191
ctx.ComponentConfig.KubeCloudShared.ClusterName,
92+
utilfeature.DefaultFeatureGate,
9293
)
9394
if err != nil {
9495
// This error shouldn't fail. It lives like this as a legacy.

‎cmd/kube-controller-manager/app/core.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ func startServiceController(ctx ControllerContext) (http.Handler, bool, error) {
8383
ctx.InformerFactory.Core().V1().Services(),
8484
ctx.InformerFactory.Core().V1().Nodes(),
8585
ctx.ComponentConfig.KubeCloudShared.ClusterName,
86+
utilfeature.DefaultFeatureGate,
8687
)
8788
if err != nil {
8889
// This error shouldn't fail. It lives like this as a legacy.

‎pkg/controller/service/BUILD‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ go_library(
1515
"//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
1616
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
1717
"//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
18-
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
1918
"//staging/src/k8s.io/client-go/informers/core/v1:go_default_library",
2019
"//staging/src/k8s.io/client-go/kubernetes:go_default_library",
2120
"//staging/src/k8s.io/client-go/kubernetes/scheme:go_default_library",
@@ -26,6 +25,7 @@ go_library(
2625
"//staging/src/k8s.io/client-go/util/workqueue:go_default_library",
2726
"//staging/src/k8s.io/cloud-provider:go_default_library",
2827
"//staging/src/k8s.io/cloud-provider/service/helpers:go_default_library",
28+
"//staging/src/k8s.io/component-base/featuregate:go_default_library",
2929
"//staging/src/k8s.io/component-base/metrics/prometheus/ratelimiter:go_default_library",
3030
"//vendor/k8s.io/klog:go_default_library",
3131
],
@@ -43,14 +43,16 @@ go_test(
4343
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
4444
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
4545
"//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
46-
"//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
4746
"//staging/src/k8s.io/client-go/informers:go_default_library",
4847
"//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
48+
"//staging/src/k8s.io/client-go/kubernetes/scheme:go_default_library",
49+
"//staging/src/k8s.io/client-go/kubernetes/typed/core/v1:go_default_library",
4950
"//staging/src/k8s.io/client-go/testing:go_default_library",
5051
"//staging/src/k8s.io/client-go/tools/record:go_default_library",
52+
"//staging/src/k8s.io/client-go/util/workqueue:go_default_library",
5153
"//staging/src/k8s.io/cloud-provider/fake:go_default_library",
5254
"//staging/src/k8s.io/cloud-provider/service/helpers:go_default_library",
53-
"//staging/src/k8s.io/component-base/featuregate/testing:go_default_library",
55+
"//vendor/k8s.io/klog:go_default_library",
5456
],
5557
)
5658

‎pkg/controller/service/controller.go‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"k8s.io/apimachinery/pkg/util/runtime"
3030
"k8s.io/apimachinery/pkg/util/sets"
3131
"k8s.io/apimachinery/pkg/util/wait"
32-
utilfeature "k8s.io/apiserver/pkg/util/feature"
3332
coreinformers "k8s.io/client-go/informers/core/v1"
3433
clientset "k8s.io/client-go/kubernetes"
3534
"k8s.io/client-go/kubernetes/scheme"
@@ -40,6 +39,7 @@ import (
4039
"k8s.io/client-go/util/workqueue"
4140
cloudprovider "k8s.io/cloud-provider"
4241
servicehelper "k8s.io/cloud-provider/service/helpers"
42+
"k8s.io/component-base/featuregate"
4343
"k8s.io/component-base/metrics/prometheus/ratelimiter"
4444
"k8s.io/klog"
4545
)
@@ -110,6 +110,9 @@ type Controller struct {
110110
nodeListerSynced cache.InformerSynced
111111
// services that need to be synced
112112
queue workqueue.RateLimitingInterface
113+
// feature gates stored in local field for better testability
114+
legacyNodeRoleFeatureEnabled bool
115+
serviceNodeExclusionFeatureEnabled bool
113116
}
114117

115118
// New returns a new service controller to keep cloud provider service resources
@@ -120,6 +123,7 @@ func New(
120123
serviceInformer coreinformers.ServiceInformer,
121124
nodeInformer coreinformers.NodeInformer,
122125
clusterName string,
126+
featureGate featuregate.FeatureGate,
123127
) (*Controller, error) {
124128
broadcaster := record.NewBroadcaster()
125129
broadcaster.StartLogging(klog.Infof)
@@ -133,16 +137,18 @@ func New(
133137
}
134138

135139
s := &Controller{
136-
cloud: cloud,
137-
knownHosts: []*v1.Node{},
138-
kubeClient: kubeClient,
139-
clusterName: clusterName,
140-
cache: &serviceCache{serviceMap: make(map[string]*cachedService)},
141-
eventBroadcaster: broadcaster,
142-
eventRecorder: recorder,
143-
nodeLister: nodeInformer.Lister(),
144-
nodeListerSynced: nodeInformer.Informer().HasSynced,
145-
queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "service"),
140+
cloud: cloud,
141+
knownHosts: []*v1.Node{},
142+
kubeClient: kubeClient,
143+
clusterName: clusterName,
144+
cache: &serviceCache{serviceMap: make(map[string]*cachedService)},
145+
eventBroadcaster: broadcaster,
146+
eventRecorder: recorder,
147+
nodeLister: nodeInformer.Lister(),
148+
nodeListerSynced: nodeInformer.Informer().HasSynced,
149+
queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "service"),
150+
legacyNodeRoleFeatureEnabled: featureGate.Enabled(legacyNodeRoleBehaviorFeature),
151+
serviceNodeExclusionFeatureEnabled: featureGate.Enabled(serviceNodeExclusionFeature),
146152
}
147153

148154
serviceInformer.Informer().AddEventHandlerWithResyncPeriod(
@@ -188,6 +194,7 @@ func New(
188194
if err := s.init(); err != nil {
189195
return nil, err
190196
}
197+
191198
return s, nil
192199
}
193200

@@ -383,7 +390,7 @@ func (s *Controller) syncLoadBalancerIfNeeded(service *v1.Service, key string) (
383390
}
384391

385392
func (s *Controller) ensureLoadBalancer(service *v1.Service) (*v1.LoadBalancerStatus, error) {
386-
nodes, err := listWithPredicate(s.nodeLister, getNodeConditionPredicate())
393+
nodes, err := listWithPredicate(s.nodeLister, s.getNodeConditionPredicate())
387394
if err != nil {
388395
return nil, err
389396
}
@@ -612,22 +619,22 @@ func nodeSlicesEqualForLB(x, y []*v1.Node) bool {
612619
return nodeNames(x).Equal(nodeNames(y))
613620
}
614621

615-
func getNodeConditionPredicate() NodeConditionPredicate {
622+
func (s *Controller) getNodeConditionPredicate() NodeConditionPredicate {
616623
return func(node *v1.Node) bool {
617624
// We add the master to the node list, but its unschedulable. So we use this to filter
618625
// the master.
619626
if node.Spec.Unschedulable {
620627
return false
621628
}
622629

623-
if utilfeature.DefaultFeatureGate.Enabled(legacyNodeRoleBehaviorFeature) {
630+
if s.legacyNodeRoleFeatureEnabled {
624631
// As of 1.6, we will taint the master, but not necessarily mark it unschedulable.
625632
// Recognize nodes labeled as master, and filter them also, as we were doing previously.
626633
if _, hasMasterRoleLabel := node.Labels[labelNodeRoleMaster]; hasMasterRoleLabel {
627634
return false
628635
}
629636
}
630-
if utilfeature.DefaultFeatureGate.Enabled(serviceNodeExclusionFeature) {
637+
if s.serviceNodeExclusionFeatureEnabled {
631638
if _, hasExcludeBalancerLabel := node.Labels[labelNodeRoleExcludeBalancer]; hasExcludeBalancerLabel {
632639
return false
633640
}
@@ -654,7 +661,7 @@ func getNodeConditionPredicate() NodeConditionPredicate {
654661
func (s *Controller) nodeSyncLoop() {
655662
s.knownHostsLock.Lock()
656663
defer s.knownHostsLock.Unlock()
657-
newHosts, err := listWithPredicate(s.nodeLister, getNodeConditionPredicate())
664+
newHosts, err := listWithPredicate(s.nodeLister, s.getNodeConditionPredicate())
658665
if err != nil {
659666
runtime.HandleError(fmt.Errorf("Failed to retrieve current set of nodes from node lister: %v", err))
660667
return

‎pkg/controller/service/controller_test.go‎

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,16 @@ import (
3333
"k8s.io/apimachinery/pkg/runtime/schema"
3434
"k8s.io/apimachinery/pkg/types"
3535
"k8s.io/apimachinery/pkg/util/intstr"
36-
utilfeature "k8s.io/apiserver/pkg/util/feature"
3736
"k8s.io/client-go/informers"
3837
"k8s.io/client-go/kubernetes/fake"
38+
"k8s.io/client-go/kubernetes/scheme"
39+
v1core "k8s.io/client-go/kubernetes/typed/core/v1"
3940
core "k8s.io/client-go/testing"
4041
"k8s.io/client-go/tools/record"
42+
"k8s.io/client-go/util/workqueue"
4143
fakecloud "k8s.io/cloud-provider/fake"
4244
servicehelper "k8s.io/cloud-provider/service/helpers"
43-
featuregatetesting "k8s.io/component-base/featuregate/testing"
45+
"k8s.io/klog"
4446
)
4547

4648
const region = "us-central"
@@ -70,21 +72,43 @@ func newController() (*Controller, *fakecloud.Cloud, *fake.Clientset) {
7072
cloud := &fakecloud.Cloud{}
7173
cloud.Region = region
7274

73-
client := fake.NewSimpleClientset()
75+
kubeClient := fake.NewSimpleClientset()
7476

75-
informerFactory := informers.NewSharedInformerFactory(client, 0)
77+
informerFactory := informers.NewSharedInformerFactory(kubeClient, 0)
7678
serviceInformer := informerFactory.Core().V1().Services()
7779
nodeInformer := informerFactory.Core().V1().Nodes()
7880

79-
controller, _ := New(cloud, client, serviceInformer, nodeInformer, "test-cluster")
81+
broadcaster := record.NewBroadcaster()
82+
broadcaster.StartLogging(klog.Infof)
83+
broadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
84+
recorder := broadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "service-controller"})
85+
86+
controller := &Controller{
87+
cloud: cloud,
88+
knownHosts: []*v1.Node{},
89+
kubeClient: kubeClient,
90+
clusterName: "test-cluster",
91+
cache: &serviceCache{serviceMap: make(map[string]*cachedService)},
92+
eventBroadcaster: broadcaster,
93+
eventRecorder: recorder,
94+
nodeLister: nodeInformer.Lister(),
95+
nodeListerSynced: nodeInformer.Informer().HasSynced,
96+
queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "service"),
97+
}
98+
99+
balancer, _ := cloud.LoadBalancer()
100+
controller.balancer = balancer
101+
102+
controller.serviceLister = serviceInformer.Lister()
103+
80104
controller.nodeListerSynced = alwaysReady
81105
controller.serviceListerSynced = alwaysReady
82106
controller.eventRecorder = record.NewFakeRecorder(100)
83107

84-
cloud.Calls = nil // ignore any cloud calls made in init()
85-
client.ClearActions() // ignore any client calls made in init()
108+
cloud.Calls = nil // ignore any cloud calls made in init()
109+
kubeClient.ClearActions() // ignore any client calls made in init()
86110

87-
return controller, cloud, client
111+
return controller, cloud, kubeClient
88112
}
89113

90114
// TODO(@MrHohn): Verify the end state when below issue is resolved:
@@ -1367,10 +1391,12 @@ func Test_getNodeConditionPredicate(t *testing.T) {
13671391
}
13681392
for _, tt := range tests {
13691393
t.Run(tt.name, func(t *testing.T) {
1370-
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, serviceNodeExclusionFeature, tt.enableExclusion)()
1371-
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, legacyNodeRoleBehaviorFeature, tt.enableLegacy)()
1394+
c := &Controller{
1395+
legacyNodeRoleFeatureEnabled: tt.enableLegacy,
1396+
serviceNodeExclusionFeatureEnabled: tt.enableExclusion,
1397+
}
13721398

1373-
if result := getNodeConditionPredicate()(tt.input); result != tt.want {
1399+
if result := c.getNodeConditionPredicate()(tt.input); result != tt.want {
13741400
t.Errorf("getNodeConditionPredicate() = %v, want %v", result, tt.want)
13751401
}
13761402
})

0 commit comments

Comments
 (0)