Skip to content

Commit 04cf9ab

Browse files
enxebreopenshift-merge-robot
enxebre
authored and
openshift-merge-robot
committed
UPSTREAM: <carry>: openshift: Validate machineSet before reconciliation
1 parent 0857cd2 commit 04cf9ab

File tree

2 files changed

+138
-64
lines changed

2 files changed

+138
-64
lines changed

‎pkg/controller/machineset/controller.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,12 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
166166

167167
func (r *ReconcileMachineSet) reconcile(ctx context.Context, machineSet *machinev1beta1.MachineSet) (reconcile.Result, error) {
168168
klog.V(4).Infof("Reconcile machineset %v", machineSet.Name)
169+
if errList := machineSet.Validate(); len(errList) > 0 {
170+
err := fmt.Errorf("%q machineset validation failed: %v", machineSet.Name, errList.ToAggregate().Error())
171+
klog.Error(err)
172+
return reconcile.Result{}, err
173+
}
174+
169175
allMachines := &machinev1beta1.MachineList{}
170176

171177
if err := r.Client.List(context.Background(), client.InNamespace(machineSet.Namespace), allMachines); err != nil {

‎pkg/controller/machineset/machineset_controller_test.go

Lines changed: 132 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import (
2323
"golang.org/x/net/context"
2424
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2525
"k8s.io/apimachinery/pkg/types"
26-
clusterv1beta1 "sigs.k8s.io/cluster-api/pkg/apis/machine/v1beta1"
26+
machinev1beta1 "sigs.k8s.io/cluster-api/pkg/apis/machine/v1beta1"
2727
"sigs.k8s.io/controller-runtime/pkg/client"
2828
"sigs.k8s.io/controller-runtime/pkg/manager"
2929
"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -36,19 +36,6 @@ var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Nam
3636
const timeout = time.Second * 5
3737

3838
func TestReconcile(t *testing.T) {
39-
replicas := int32(2)
40-
instance := &clusterv1beta1.MachineSet{
41-
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
42-
Spec: clusterv1beta1.MachineSetSpec{
43-
Replicas: &replicas,
44-
Template: clusterv1beta1.MachineTemplateSpec{
45-
Spec: clusterv1beta1.MachineSpec{
46-
Versions: clusterv1beta1.MachineVersionInfo{Kubelet: "1.10.3"},
47-
},
48-
},
49-
},
50-
}
51-
5239
// Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a
5340
// channel when it is finished.
5441
mgr, err := manager.New(cfg, manager.Options{})
@@ -64,61 +51,142 @@ func TestReconcile(t *testing.T) {
6451
}
6552
defer close(StartTestManager(mgr, t))
6653

67-
// Create the MachineSet object and expect Reconcile to be called and the Machines to be created.
68-
if err := c.Create(context.TODO(), instance); err != nil {
69-
t.Errorf("error creating instance: %v", err)
70-
}
71-
defer c.Delete(context.TODO(), instance)
72-
select {
73-
case recv := <-requests:
74-
if recv != expectedRequest {
75-
t.Error("received request does not match expected request")
76-
}
77-
case <-time.After(timeout):
78-
t.Error("timed out waiting for request")
54+
replicas := int32(2)
55+
labels := map[string]string{"foo": "bar"}
56+
57+
testCases := []struct {
58+
name string
59+
instance *machinev1beta1.MachineSet
60+
expectedRequest reconcile.Request
61+
verifyFnc func()
62+
}{
63+
{
64+
name: "Refuse invalid machineset (with invalid matching labels)",
65+
instance: &machinev1beta1.MachineSet{
66+
ObjectMeta: metav1.ObjectMeta{Name: "invalidfoo", Namespace: "default"},
67+
Spec: machinev1beta1.MachineSetSpec{
68+
Replicas: &replicas,
69+
Selector: metav1.LabelSelector{
70+
MatchLabels: map[string]string{"foo": "bar"},
71+
},
72+
Template: machinev1beta1.MachineTemplateSpec{
73+
ObjectMeta: metav1.ObjectMeta{
74+
Labels: map[string]string{"foo": "bar2"},
75+
},
76+
Spec: machinev1beta1.MachineSpec{
77+
Versions: machinev1beta1.MachineVersionInfo{Kubelet: "1.10.3"},
78+
},
79+
},
80+
},
81+
},
82+
expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"}},
83+
verifyFnc: func() {
84+
// expecting machineset validation error
85+
if _, err := r.Reconcile(reconcile.Request{
86+
NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"},
87+
}); err == nil {
88+
t.Errorf("expected validation error did not occur")
89+
}
90+
},
91+
},
92+
{
93+
name: "Create the MachineSet object and expect Reconcile to be called and the Machines to be created",
94+
instance: &machinev1beta1.MachineSet{
95+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
96+
Spec: machinev1beta1.MachineSetSpec{
97+
Replicas: &replicas,
98+
Selector: metav1.LabelSelector{
99+
MatchLabels: labels,
100+
},
101+
Template: machinev1beta1.MachineTemplateSpec{
102+
ObjectMeta: metav1.ObjectMeta{
103+
Labels: labels,
104+
},
105+
Spec: machinev1beta1.MachineSpec{
106+
Versions: machinev1beta1.MachineVersionInfo{Kubelet: "1.10.3"},
107+
},
108+
},
109+
},
110+
},
111+
expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "foo", Namespace: "default"}},
112+
// Verify machines are created and recreated after deletion
113+
verifyFnc: func() {
114+
machines := &machinev1beta1.MachineList{}
115+
116+
// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
117+
// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
118+
expectInt(t, int(replicas), func(ctx context.Context) int {
119+
if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
120+
return -1
121+
}
122+
return len(machines.Items)
123+
})
124+
125+
// Verify that each machine has the desired kubelet version.
126+
for _, m := range machines.Items {
127+
if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
128+
t.Errorf("kubelet was %q not '1.10.3'", k)
129+
}
130+
}
131+
132+
// Delete a Machine and expect Reconcile to be called to replace it.
133+
m := machines.Items[0]
134+
if err := c.Delete(context.TODO(), &m); err != nil {
135+
t.Errorf("error deleting machine: %v", err)
136+
}
137+
select {
138+
case recv := <-requests:
139+
if recv != expectedRequest {
140+
t.Error("received request does not match expected request")
141+
}
142+
case <-time.After(timeout):
143+
t.Error("timed out waiting for request")
144+
}
145+
146+
// TODO (robertbailey): Figure out why the control loop isn't working as expected.
147+
/*
148+
g.Eventually(func() int {
149+
if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
150+
return -1
151+
}
152+
return len(machines.Items)
153+
}, timeout).Should(gomega.BeEquivalentTo(replicas))
154+
*/
155+
},
156+
},
79157
}
80158

81-
machines := &clusterv1beta1.MachineList{}
82-
83-
// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
84-
// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
85-
expectInt(t, int(replicas), func(ctx context.Context) int {
86-
if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
87-
return -1
88-
}
89-
return len(machines.Items)
90-
})
159+
for _, tc := range testCases {
160+
t.Logf("Running %q testcase", tc.name)
161+
func() {
162+
if err := c.Create(context.TODO(), tc.instance); err != nil {
163+
t.Errorf("error creating instance: %v", err)
164+
}
91165

92-
// Verify that each machine has the desired kubelet version.
93-
for _, m := range machines.Items {
94-
if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
95-
t.Errorf("kubelet was %q not '1.10.3'", k)
96-
}
97-
}
166+
defer func() {
167+
c.Delete(context.TODO(), tc.instance)
168+
select {
169+
case recv := <-requests:
170+
if recv != tc.expectedRequest {
171+
t.Error("received request does not match expected request")
172+
}
173+
case <-time.After(timeout):
174+
t.Error("timed out waiting for request")
175+
}
176+
}()
177+
178+
select {
179+
case recv := <-requests:
180+
if recv != tc.expectedRequest {
181+
t.Error("received request does not match expected request")
182+
}
183+
case <-time.After(timeout):
184+
t.Error("timed out waiting for request")
185+
}
98186

99-
// Delete a Machine and expect Reconcile to be called to replace it.
100-
m := machines.Items[0]
101-
if err := c.Delete(context.TODO(), &m); err != nil {
102-
t.Errorf("error deleting machine: %v", err)
187+
tc.verifyFnc()
188+
}()
103189
}
104-
select {
105-
case recv := <-requests:
106-
if recv != expectedRequest {
107-
t.Error("received request does not match expected request")
108-
}
109-
case <-time.After(timeout):
110-
t.Error("timed out waiting for request")
111-
}
112-
113-
// TODO (robertbailey): Figure out why the control loop isn't working as expected.
114-
/*
115-
g.Eventually(func() int {
116-
if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
117-
return -1
118-
}
119-
return len(machines.Items)
120-
}, timeout).Should(gomega.BeEquivalentTo(replicas))
121-
*/
122190
}
123191

124192
func expectInt(t *testing.T, expect int, fn func(context.Context) int) {

0 commit comments

Comments
 (0)