Skip named nodes in the CNR that don't exist #83

vincentportella · 2024-07-19T05:28:54Z

Updated to skip named nodes in the CNR if they don't exist through a validationOptions.skipNamedNodes option. As a result if none of the named nodes exist the CNR will succeed immediately when it transitions to the Initialized phase.
ValidationOptions option added to the CNR and Nodegroup objects.
Very minor refactor of the Pending phase and supporting functions.

mwhittington21

You've significantly altered the behaviour of the software, but haven't added tests for the new behaviour. I would expect to see some tests added that have nodes added to the initial request that are then terminated, and the behaviour validated on that. I'm a little uneasy releasing this functionality without this test.

Additionally, there may be other people running Cyclops who are relying on the existing behaviour. I can see two options here:

Cut a new major version of Cyclops (i.e. backwards incompatible changes)
Add a configurable setting for this behaviour and default it to the old behaviour

mwhittington21 · 2024-07-22T04:53:37Z

pkg/controller/cyclenoderequest/transitioner/node.go

-				foundNode = true
+// Skips any named node that does not exist in the node group for this CycleNodeRequest.
+func (t *CycleNodeRequestTransitioner) addNamedNodesToTerminate(kubeNodes map[string]corev1.Node, nodeGroupInstances map[string]cloudprovider.Instance) {
+	kubeNodesMap := make(map[string]corev1.Node)


nit: name this after the function of the variable. Something like nodeLookupByName. I kept trying to find it being used for something more than a lookup.

Additionally, there may be other people running Cyclops who are relying on the existing behaviour.

Good point, I initially intended for this to be a breaking change but now that I think about it making it configurable may be the better approach to cater to different use-cases.

👍 I'll add some tests to validate the intended behaviour.

mwhittington21 · 2024-07-22T04:55:51Z

pkg/controller/cyclenoderequest/transitioner/transitions.go

 	kubeNodes, err := t.listReadyNodes(true)
 	if err != nil {
 		return t.transitionToHealing(err)
 	}
-	if len(kubeNodes) == 0 {


We're losing the ability to detect user error here. Is there a way we can keep user error detection, while also not failing at nonexistent nodes?

One strategy could be: if our selector did match some nodes, but none were still in the cluster, we can call it a success. If it matches no nodes, we don't call it a success. The only problem with this approach is nodegroups scaled to zero. Thoughts?

I don't really see how that's possible unless we have a way of knowing if a node name in the CNR matches a node that existed prior to the CNR being created.

A flag here would help to toggle strict node validation and enable both use cases but I'm wary of adding more settings to a CNR.

…strings

…alidation option

awprice · 2024-08-14T05:23:07Z

deploy/crds/atlassian.com_cyclenodestatuses_crd.yaml

@@ -101,6 +101,13 @@ spec:
                    - Drain
                    - Wait
                    type: string
+                  strictValidation:


So by default if not set this will be false?

Added a new StrictValidation option to the CNR to keep backwards compatible functionality.

I guess this does keep backwards compatible functionality, but is technically a breaking change or a behaviour change - we'll need to document this in the release notes.

We'll also need to ensure cluster owners update the CRDs to include this new field.

Yeah that is the intention, I would like this new functionality to be the default rather than the exception because it reduces the number of times a cycle will fail by default.

However, happy to change it if we deem it's better to set the flag to true by default.

awprice · 2024-08-14T05:25:58Z

pkg/apis/atlassian/v1/common_types.go

+	// StrictValidation is a boolean which determines whether named nodes selected in a CNR must
+	// exist and be valid nodes before cycling can begin. If set to true when invalid nodes are
+	// selected the CNR will be transitioned to the "Failed" phase before cycling can begin again.
+	StrictValidation bool `json:"strictValidation,omitempty"`


Do we ever see StrictValidation being used for other things?

At the moment the description of this field suggests its only for this one thing - when invalid nodes are found, transition to failed. But the naming of the field suggests its to generically enable/disable strict validation.

Naming is hard, but I'd probably suggest renaming the flag to something that describes specifically what it is enabling/disabling

Does it also make sense to put this field into CycleSettings?

Doing this means it's available in a CycleNodeStatus object - do we need it here?

Should we move it elsewhere within CycleNodeRequest to make it not propagate down?

Could we have a "ValidationOptions" field within CycleNodeRequest for holding other validation/configuration options a user might want to configure?

I can see your point. I initially intended for more validations to be affected by PR but then narrowed the scope to just checking for named nodes. Having said that, having ValidationOptions in the CNR would be better because we can then have more granular settings. 🤔

With this PR, we could make it something like, ValidationOptions.SkipMissingNamedNodes. The existing functionality would remain the same and adding the flag would lead to using this new functionality.

awprice · 2024-08-14T05:28:44Z

deploy/crds/atlassian.com_cyclenoderequests_crd.yaml

@@ -44,6 +44,8 @@ spec:
              of an object. Servers should convert recognized schemas to the latest
              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
            type: string
+          clusterName:


Where does this field come from? Why is it only in the CycleNodeRequest resource?

It was part of the operator-sdk render, make generate-crds

awprice

LGTM - nice work! Love the validationOptions field

mwhittington21

LGTM

Skip named nodes that don't exist

4898f24

vincentportella requested review from awprice, Jacobious52, mwhittington21, MinyiZ and vcatlassian July 19, 2024 05:28

vincentportella self-assigned this Jul 19, 2024

vincentportella linked an issue Jul 19, 2024 that may be closed by this pull request

Skip named nodes in a CNR if they do not exist #84

Closed

Fix util tests to account for changes

11a4767

mwhittington21 requested changes Jul 22, 2024

View reviewed changes

vincentportella and others added 4 commits July 22, 2024 15:31

findOffendingNodes returns usable maps of nodes rather than lists of …

01b396e

…strings

Merge branch 'master' into vportella/skip-named-nodes-that-dont-exist

834f0d0

Reduce lenient validation to just missing node names and add strict v…

5e8160c

…alidation option

rename variable for clarity

659206b

vincentportella requested a review from mwhittington21 August 13, 2024 10:46

MinyiZ previously approved these changes Aug 14, 2024

View reviewed changes

awprice requested changes Aug 14, 2024

View reviewed changes

awprice reviewed Aug 14, 2024

View reviewed changes

Update CNR with new ValidationOptions sub-section

2b6d1db

vincentportella dismissed MinyiZ’s stale review via 2b6d1db August 16, 2024 03:39

Add validationOptions to nodegroup spec as well

370be58

vincentportella requested review from awprice and MinyiZ August 16, 2024 05:22

awprice approved these changes Aug 16, 2024

View reviewed changes

mwhittington21 approved these changes Aug 19, 2024

View reviewed changes

vincentportella merged commit 9e73b88 into master Aug 19, 2024
3 checks passed

vincentportella deleted the vportella/skip-named-nodes-that-dont-exist branch August 19, 2024 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip named nodes in the CNR that don't exist #83

Skip named nodes in the CNR that don't exist #83

vincentportella commented Jul 19, 2024 •

edited

Loading

mwhittington21 left a comment

mwhittington21 Jul 22, 2024

vincentportella Jul 22, 2024 •

edited

Loading

mwhittington21 Jul 22, 2024

vincentportella Jul 22, 2024

awprice Aug 14, 2024 •

edited

Loading

vincentportella Aug 14, 2024 •

edited

Loading

awprice Aug 14, 2024

awprice Aug 14, 2024

vincentportella Aug 14, 2024 •

edited

Loading

awprice Aug 14, 2024

vincentportella Aug 14, 2024

awprice left a comment

mwhittington21 left a comment

Skip named nodes in the CNR that don't exist #83

Skip named nodes in the CNR that don't exist #83

Conversation

vincentportella commented Jul 19, 2024 • edited Loading

mwhittington21 left a comment

Choose a reason for hiding this comment

mwhittington21 Jul 22, 2024

Choose a reason for hiding this comment

vincentportella Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

mwhittington21 Jul 22, 2024

Choose a reason for hiding this comment

vincentportella Jul 22, 2024

Choose a reason for hiding this comment

awprice Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

vincentportella Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

awprice Aug 14, 2024

Choose a reason for hiding this comment

awprice Aug 14, 2024

Choose a reason for hiding this comment

vincentportella Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

awprice Aug 14, 2024

Choose a reason for hiding this comment

vincentportella Aug 14, 2024

Choose a reason for hiding this comment

awprice left a comment

Choose a reason for hiding this comment

mwhittington21 left a comment

Choose a reason for hiding this comment

vincentportella commented Jul 19, 2024 •

edited

Loading

vincentportella Jul 22, 2024 •

edited

Loading

awprice Aug 14, 2024 •

edited

Loading

vincentportella Aug 14, 2024 •

edited

Loading

vincentportella Aug 14, 2024 •

edited

Loading