Skip to content

Commit 09cddb2

Browse files
authored
further prep for integration tests (#145)
Signed-off-by: Alex Price <aprice@atlassian.com>
1 parent 251b401 commit 09cddb2

7 files changed

Lines changed: 329 additions & 94 deletions

File tree

cmd/manager/main.go

Lines changed: 30 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,11 @@ import (
77
"runtime"
88

99
"github.com/alecthomas/kingpin/v2"
10-
"github.com/atlassian-labs/cyclops/pkg/apis"
1110
"github.com/atlassian-labs/cyclops/pkg/cloudprovider/builder"
12-
"github.com/atlassian-labs/cyclops/pkg/controller/cyclenoderequest"
11+
cyclopsmanager "github.com/atlassian-labs/cyclops/pkg/manager"
1312
cnrTransitioner "github.com/atlassian-labs/cyclops/pkg/controller/cyclenoderequest/transitioner"
14-
"github.com/atlassian-labs/cyclops/pkg/controller/cyclenodestatus"
1513
cnsTransitioner "github.com/atlassian-labs/cyclops/pkg/controller/cyclenodestatus/transitioner"
1614
nodecontroller "github.com/atlassian-labs/cyclops/pkg/controller/node"
17-
"github.com/atlassian-labs/cyclops/pkg/metrics"
1815
"github.com/atlassian-labs/cyclops/pkg/notifications"
1916
"github.com/atlassian-labs/cyclops/pkg/notifications/notifierbuilder"
2017
"github.com/operator-framework/operator-lib/leader"
@@ -100,17 +97,6 @@ func main() {
10097
os.Exit(1)
10198
}
10299

103-
log.Info("Registering Components.")
104-
105-
// Setup Scheme for all resources
106-
if err := apis.AddToScheme(mgr.GetScheme()); err != nil {
107-
log.Error(err, "Unable to setup scheme")
108-
os.Exit(1)
109-
}
110-
111-
// Register the custom metrics
112-
metrics.Register(mgr.GetClient(), log, *namespace)
113-
114100
// Setup the cloud provider
115101
// Uses AWS SDK's built-in retry behavior
116102
cloudProvider, err := builder.BuildCloudProvider(*cloudProviderName, logger)
@@ -130,57 +116,37 @@ func main() {
130116
}
131117
}
132118

133-
// Configure the CNR transitioner options
134-
cnrOptions := cnrTransitioner.Options{
135-
DeleteCNR: *deleteCNR,
136-
DeleteCNRExpiry: *deleteCNRExpiry,
137-
DeleteCNRRequeue: *deleteCNRRequeue,
138-
HealthCheckTimeout: *healthCheckTimeout,
139-
ScaleUpWait: *cnrScaleUpWait,
140-
ScaleUpLimit: *cnrScaleUpLimit,
141-
NodeEquilibriumWaitLimit: *cnrNodeEquilibriumWaitLimit,
142-
TransitionDuration: *cnrTransitionDuration,
143-
RequeueDuration: *cnrRequeueDuration,
144-
}
145-
146-
// Configure the CNS transitioner options
147-
cnsOptions := cnsTransitioner.Options{
148-
DefaultCNScyclingExpiry: *defaultCNScyclingExpiry,
149-
UnhealthyPodTerminationThreshold: *unhealthyPodTerminationThreshold,
150-
TransitionDuration: *cnsTransitionDuration,
151-
WaitingPodsRequeue: *cnsWaitingPodsRequeue,
152-
RemovingLabelsPodsRequeue: *cnsRemovingLabelsPodsRequeue,
153-
DrainingRetryRequeue: *cnsDrainingRetryRequeue,
154-
DrainingPodsRequeue: *cnsDrainingPodsRequeue,
155-
}
156-
157-
// Configure the node controller options
158-
nodeOptions := nodecontroller.Options{
159-
ReconcileConcurrency: *nodeControllerReconcileConcurrency,
160-
RequeueAfter: *nodeControllerRequeueAfter,
161-
}
162-
163-
// Set up and register the controllers that will share resources between them
164-
_, err = cyclenoderequest.NewReconciler(mgr, cloudProvider, notifier, *namespace, cnrOptions)
165-
if err != nil {
166-
log.Error(err, "Unable to add cycleNodeRequest controller")
167-
os.Exit(1)
168-
}
169-
_, err = cyclenodestatus.NewReconciler(mgr, cloudProvider, notifier, *namespace, cnsOptions)
170-
if err != nil {
171-
log.Error(err, "Unable to add cycleNodeStatus controller")
172-
os.Exit(1)
173-
}
174-
_, err = nodecontroller.NewReconciler(mgr, *namespace, nodeOptions)
175-
if err != nil {
176-
log.Error(err, "Unable to add node controller")
177-
os.Exit(1)
178-
}
179-
180119
log.Info("Starting the Cmd.")
181120

182-
// Start the Cmd
183-
if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
121+
if err := cyclopsmanager.Run(signals.SetupSignalHandler(), mgr, cyclopsmanager.Dependencies{
122+
CloudProvider: cloudProvider,
123+
Notifier: notifier,
124+
Namespace: *namespace,
125+
CNROptions: cnrTransitioner.Options{
126+
DeleteCNR: *deleteCNR,
127+
DeleteCNRExpiry: *deleteCNRExpiry,
128+
DeleteCNRRequeue: *deleteCNRRequeue,
129+
HealthCheckTimeout: *healthCheckTimeout,
130+
ScaleUpWait: *cnrScaleUpWait,
131+
ScaleUpLimit: *cnrScaleUpLimit,
132+
NodeEquilibriumWaitLimit: *cnrNodeEquilibriumWaitLimit,
133+
TransitionDuration: *cnrTransitionDuration,
134+
RequeueDuration: *cnrRequeueDuration,
135+
},
136+
CNSOptions: cnsTransitioner.Options{
137+
DefaultCNScyclingExpiry: *defaultCNScyclingExpiry,
138+
UnhealthyPodTerminationThreshold: *unhealthyPodTerminationThreshold,
139+
TransitionDuration: *cnsTransitionDuration,
140+
WaitingPodsRequeue: *cnsWaitingPodsRequeue,
141+
RemovingLabelsPodsRequeue: *cnsRemovingLabelsPodsRequeue,
142+
DrainingRetryRequeue: *cnsDrainingRetryRequeue,
143+
DrainingPodsRequeue: *cnsDrainingPodsRequeue,
144+
},
145+
NodeOptions: nodecontroller.Options{
146+
ReconcileConcurrency: *nodeControllerReconcileConcurrency,
147+
RequeueAfter: *nodeControllerRequeueAfter,
148+
},
149+
}); err != nil {
184150
log.Error(err, "Manager exited non-zero")
185151
os.Exit(1)
186152
}

go.mod

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ require (
1313
github.com/operator-framework/operator-lib v0.17.0
1414
github.com/pkg/errors v0.9.1
1515
github.com/prometheus/client_golang v1.22.0
16+
github.com/prometheus/client_model v0.6.1
1617
github.com/slack-go/slack v0.23.1
1718
github.com/spf13/cobra v1.9.1
1819
github.com/spf13/pflag v1.0.6
@@ -64,7 +65,6 @@ require (
6465
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
6566
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
6667
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
67-
github.com/prometheus/client_model v0.6.1 // indirect
6868
github.com/prometheus/common v0.63.0 // indirect
6969
github.com/prometheus/procfs v0.16.0 // indirect
7070
github.com/x448/float16 v0.8.4 // indirect
@@ -84,6 +84,7 @@ require (
8484
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
8585
gopkg.in/inf.v0 v0.9.1 // indirect
8686
gopkg.in/yaml.v3 v3.0.1 // indirect
87+
k8s.io/apiextensions-apiserver v0.32.1 // indirect
8788
k8s.io/kube-openapi v0.0.0-20250318190949-c8a335a9a2ff // indirect
8889
k8s.io/utils v0.0.0-20250321185631-1f6e0b77f77e // indirect
8990
sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect

pkg/k8s/node.go

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -59,28 +59,24 @@ func IsCordoned(name string, client kubernetes.Interface) (bool, error) {
5959

6060
// AddLabelToNode performs a patch operation on a node to add a label to the node
6161
func AddLabelToNode(nodeName string, labelName string, labelValue string, client kubernetes.Interface) error {
62-
patches := []Patch{
63-
{
64-
Op: "add",
65-
// json patch spec maps "~1" to "/" as an escape sequence, so we do the translation here...
66-
Path: fmt.Sprintf("/metadata/labels/%s", strings.ReplaceAll(labelName, "/", "~1")),
67-
Value: labelValue,
62+
return MergePatchNode(nodeName, map[string]interface{}{
63+
"metadata": map[string]interface{}{
64+
"labels": map[string]string{labelName: labelValue},
6865
},
69-
}
70-
return PatchNode(nodeName, patches, client)
66+
}, client)
7167
}
7268

73-
// AddAnnotationToNode performs a patch operation on a node to add an annotation to the node
69+
// AddAnnotationToNode performs a merge patch on a node to add an annotation.
70+
// A merge patch is used rather than a JSON Patch "add" operation because the
71+
// latter fails when the node's annotations map is nil (which can happen in
72+
// envtest and occasionally in real clusters before the kubelet has written
73+
// its own annotations).
7474
func AddAnnotationToNode(nodeName string, annotationName string, annotationValue string, client kubernetes.Interface) error {
75-
patches := []Patch{
76-
{
77-
Op: "add",
78-
// json patch spec maps "~1" to "/" as an escape sequence, so we do the translation here...
79-
Path: fmt.Sprintf("/metadata/annotations/%s", strings.ReplaceAll(annotationName, "/", "~1")),
80-
Value: annotationValue,
75+
return MergePatchNode(nodeName, map[string]interface{}{
76+
"metadata": map[string]interface{}{
77+
"annotations": map[string]string{annotationName: annotationValue},
8178
},
82-
}
83-
return PatchNode(nodeName, patches, client)
79+
}, client)
8480
}
8581

8682
// RemoveAnnotationFromNode performs a patch operation on a node to remove an annotation from the node
@@ -134,17 +130,6 @@ func RemoveScaleDownDisabledAnnotationsFromNode(nodeName string, client kubernet
134130
)
135131
}
136132

137-
// RemoveLabelFromNode performs a patch operation on a node to remove a label from the node
138-
func RemoveLabelFromNode(nodeName string, labelName string, client kubernetes.Interface) error {
139-
patches := []Patch{
140-
{
141-
Op: "remove",
142-
Path: fmt.Sprintf("/metadata/labels/%s", strings.ReplaceAll(labelName, "/", "~1")),
143-
},
144-
}
145-
return PatchNode(nodeName, patches, client)
146-
}
147-
148133
// AddFinalizerToNode updates a node to add a finalizer to it
149134
func AddFinalizerToNode(node *v1.Node, finalizerName string, client kubernetes.Interface) error {
150135
controllerutil.AddFinalizer(node, finalizerName)

pkg/k8s/node_test.go

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
package k8s
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
corev1 "k8s.io/api/core/v1"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"k8s.io/client-go/kubernetes/fake"
12+
)
13+
14+
// newNodeForPatch creates a minimal node and registers it with a fresh fake
15+
// clientset, returning both. Using fake.NewSimpleClientset means patch calls
16+
// go through the object tracker and work out of the box.
17+
func newNodeForPatch(name string, annotations, labels map[string]string) (*corev1.Node, *fake.Clientset) {
18+
node := &corev1.Node{
19+
ObjectMeta: metav1.ObjectMeta{
20+
Name: name,
21+
Annotations: annotations,
22+
Labels: labels,
23+
},
24+
}
25+
client := fake.NewSimpleClientset(node)
26+
return node, client
27+
}
28+
29+
// TestAddAnnotationToNode_WithExistingAnnotations verifies that
30+
// AddAnnotationToNode merges into an existing annotations map without
31+
// clobbering other entries.
32+
func TestAddAnnotationToNode_WithExistingAnnotations(t *testing.T) {
33+
node, client := newNodeForPatch("test-node",
34+
map[string]string{"existing-key": "existing-value"},
35+
nil,
36+
)
37+
38+
err := AddAnnotationToNode(node.Name, "new-key", "new-value", client)
39+
require.NoError(t, err)
40+
41+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
42+
require.NoError(t, err)
43+
assert.Equal(t, "existing-value", got.Annotations["existing-key"], "existing annotation should be preserved")
44+
assert.Equal(t, "new-value", got.Annotations["new-key"], "new annotation should be set")
45+
}
46+
47+
// TestAddAnnotationToNode_WithNilAnnotations verifies that AddAnnotationToNode
48+
// succeeds even when the node's annotations map is nil. This was the bug with
49+
// the old JSON Patch "add" approach: adding a key to a nil map via JSON Patch
50+
// fails at the apiserver level.
51+
func TestAddAnnotationToNode_WithNilAnnotations(t *testing.T) {
52+
node, client := newNodeForPatch("test-node", nil, nil)
53+
54+
err := AddAnnotationToNode(node.Name, "some-key", "some-value", client)
55+
require.NoError(t, err)
56+
57+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
58+
require.NoError(t, err)
59+
assert.Equal(t, "some-value", got.Annotations["some-key"])
60+
}
61+
62+
// TestAddAnnotationToNode_OverwritesExistingValue verifies that patching an
63+
// annotation that already exists updates its value.
64+
func TestAddAnnotationToNode_OverwritesExistingValue(t *testing.T) {
65+
node, client := newNodeForPatch("test-node",
66+
map[string]string{"my-key": "old-value"},
67+
nil,
68+
)
69+
70+
err := AddAnnotationToNode(node.Name, "my-key", "new-value", client)
71+
require.NoError(t, err)
72+
73+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
74+
require.NoError(t, err)
75+
assert.Equal(t, "new-value", got.Annotations["my-key"])
76+
}
77+
78+
// TestAddLabelToNode_WithExistingLabels verifies that AddLabelToNode merges
79+
// into an existing labels map without clobbering other entries.
80+
func TestAddLabelToNode_WithExistingLabels(t *testing.T) {
81+
node, client := newNodeForPatch("test-node",
82+
nil,
83+
map[string]string{"existing-label": "existing-value"},
84+
)
85+
86+
err := AddLabelToNode(node.Name, "new-label", "new-value", client)
87+
require.NoError(t, err)
88+
89+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
90+
require.NoError(t, err)
91+
assert.Equal(t, "existing-value", got.Labels["existing-label"], "existing label should be preserved")
92+
assert.Equal(t, "new-value", got.Labels["new-label"], "new label should be set")
93+
}
94+
95+
// TestAddLabelToNode_WithNilLabels verifies that AddLabelToNode succeeds even
96+
// when the node has no labels. Mirrors the nil-annotations case.
97+
func TestAddLabelToNode_WithNilLabels(t *testing.T) {
98+
node, client := newNodeForPatch("test-node", nil, nil)
99+
100+
err := AddLabelToNode(node.Name, "role", "worker", client)
101+
require.NoError(t, err)
102+
103+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
104+
require.NoError(t, err)
105+
assert.Equal(t, "worker", got.Labels["role"])
106+
}
107+
108+
// TestAddAnnotationAndLabelToNode_Independent verifies that patching
109+
// annotations and labels independently doesn't interfere.
110+
func TestAddAnnotationAndLabelToNode_Independent(t *testing.T) {
111+
node, client := newNodeForPatch("test-node", nil, nil)
112+
113+
require.NoError(t, AddAnnotationToNode(node.Name, "ann", "ann-val", client))
114+
require.NoError(t, AddLabelToNode(node.Name, "lbl", "lbl-val", client))
115+
116+
got, err := client.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
117+
require.NoError(t, err)
118+
assert.Equal(t, "ann-val", got.Annotations["ann"])
119+
assert.Equal(t, "lbl-val", got.Labels["lbl"])
120+
}
121+
122+
// TestAddAnnotationToNode_NodeNotFound verifies that an error is returned when
123+
// the target node doesn't exist.
124+
func TestAddAnnotationToNode_NodeNotFound(t *testing.T) {
125+
client := fake.NewSimpleClientset() // empty — no nodes registered
126+
err := AddAnnotationToNode("does-not-exist", "k", "v", client)
127+
assert.Error(t, err)
128+
}
129+
130+
// TestAddLabelToNode_NodeNotFound verifies that an error is returned when the
131+
// target node doesn't exist.
132+
func TestAddLabelToNode_NodeNotFound(t *testing.T) {
133+
client := fake.NewSimpleClientset()
134+
err := AddLabelToNode("does-not-exist", "k", "v", client)
135+
assert.Error(t, err)
136+
}

pkg/k8s/patch.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ func PatchPod(name, namespace string, patches []Patch, client kubernetes.Interfa
2626
return err
2727
}
2828

29-
// PatchNode patches a node
29+
// PatchNode patches a node using a JSON Patch (RFC 6902).
3030
func PatchNode(name string, patches []Patch, client kubernetes.Interface) error {
3131
data, err := json.Marshal(patches)
3232
if err != nil {
@@ -35,3 +35,17 @@ func PatchNode(name string, patches []Patch, client kubernetes.Interface) error
3535
_, err = client.CoreV1().Nodes().Patch(context.TODO(), name, types.JSONPatchType, data, v1.PatchOptions{})
3636
return err
3737
}
38+
39+
// MergePatchNode patches a node using a strategic merge patch. The patch
40+
// argument is any JSON-serialisable value describing the fields to update.
41+
// Unlike JSON Patch, merge patches work correctly even when the target map
42+
// field (e.g. metadata.annotations) is nil on the object — the apiserver
43+
// creates the map automatically.
44+
func MergePatchNode(name string, patch interface{}, client kubernetes.Interface) error {
45+
data, err := json.Marshal(patch)
46+
if err != nil {
47+
return err
48+
}
49+
_, err = client.CoreV1().Nodes().Patch(context.TODO(), name, types.StrategicMergePatchType, data, v1.PatchOptions{})
50+
return err
51+
}

0 commit comments

Comments
 (0)