Adjust effective-creation-timeout smartly per MachineDeployment by elankath · Pull Request #1156 · gardener/machine-controller-manager

elankath · 2026-09-16T22:04:49Z

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1155

Special notes for your reviewer:

Release note:

Adjust MachineDeployment's effective-creation-timeout on Machine Join Failures

gardener-prow · 2026-09-16T22:04:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from elankath. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aaronfern

Feedback after first round of review

aaronfern · 2026-09-22T10:36:34Z

 	return mcdDeepCopy, nil
 }
+
+func (dc *controller) checkAndAdjustMachineReplaceCycleCountAndEffectiveCreationTimeout(ctx context.Context, mcd *v1alpha1.MachineDeployment, machineMap map[types.UID]*v1alpha1.MachineList) (adjusted bool, err error) {


Can you please add a docstring explaining this function? it's quite complex and a quick explanation will help in the long term

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

…tiveCreationTimeout Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

…laceCycleCountThreshold Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

elankath · 2026-09-24T08:21:19Z

Activity: Rebased PR after upstream changes

r4mek · 2026-09-24T08:30:49Z

 		return dc.rollback(ctx, d, machineSets, machineMap)
 	}

+	if adjusted, err := dc.checkAndAdjustMachineReplaceCycleCountAndEffectiveCreationTimeout(ctx, d, machineMap); adjusted || err != nil {


I was wondering if this could be put in defer so that this is the very last operation done on the MCD.

thiyyakat

Thank you for the PR. I've given a few comments after a preliminary review of the changes. I still need to review the mother lode – checkAndAdjustMachineReplaceCycleCountAndEffectiveCreationTimeout 😅 . PTAL.

thiyyakat · 2026-09-24T09:17:25Z

+// IsMachineFailed checks if machine has failed
+func IsMachineFailed(p *v1alpha1.Machine) bool {


The functions in utils.go no longer have Machine in their names. Please undo this change.

Suggested change

// IsMachineFailed checks if machine has failed

func IsMachineFailed(p *v1alpha1.Machine) bool {

// IsFailed checks if machine has failed

func IsFailed(p *v1alpha1.Machine) bool {

thiyyakat · 2026-09-24T09:22:44Z

 	metaObject, err := meta.Accessor(object)
 	if err != nil {
-		return nil, err
+		return
 	}
-	effectiveMachineCreationTimeoutStr, ok := metaObject.GetAnnotations()[v1alpha1.AnnotationKeyMachineEffectiveCreationTimeout]
+	durationStr, ok := metaObject.GetAnnotations()[v1alpha1.AnnotationKeyMachineEffectiveCreationTimeout]


Why not have a generic helper function to fetch annotation values based on the annotation key? It can be used in the Get* functions that you have introduced here. The parsing can be done in the caller.

thiyyakat · 2026-09-24T09:27:25Z


 // getMachineMachineSets returns the MachineSets matching the given Machine.
-func (c *controller) getMachineMachineSets(machine *v1alpha1.Machine) ([]*v1alpha1.MachineSet, error) {
+func (dc *controller) getMachineMachineSets(machine *v1alpha1.Machine) ([]*v1alpha1.MachineSet, error) {


Since this is the machineset controller, is the receiver name change from c to dc accurate/required?

If this was not intentional, please make the change elsewhere in this file as well.

thiyyakat · 2026-09-24T09:33:52Z

 }

-func (c *controller) Run(workers int, stopCh <-chan struct{}) {
+func (dc *controller) Run(workers int, stopCh <-chan struct{}) {


What is the reason behind this change? Since the Run method handles all types of queues, not just for machine deployment?

Maybe I'm not expanding dc correctly in my head :) Do you not mean deployment controller?

It was dc in more places and c in other places. Got lint error on inconsistent receiver name. Just changed to dc. I guess I should just change to c.

thiyyakat · 2026-09-24T09:38:24Z

 }
+
+// FilterNodeConditionOfType filters conditions and returns the NodeCondition belonging to the given conditionType or nil if not present.
+func FilterNodeConditionOfType(conditions []v1.NodeCondition, conditionType v1.NodeConditionType) *v1.NodeCondition {


If I'm not wrong, GetCondition in this file does the same thing. If you don't want to pass the node object, then please modify GetCondition to accept only a list of conditions instead, or get rid of GetCondition and use only the function introduced here. I don't think there's a need for both functions.

Co-authored-by: thiyyakat <meghana.thiyyakat@sap.com> Signed-off-by: Tarun Ramakrishna Elankath <tarun.ramakrishna.elankath@sap.com>

r4mek · 2026-09-24T09:46:33Z

+		}
+	}
+	if adjustedAnnotations := createAdjustedAnnotations(oldInfo, newInfo); len(adjustedAnnotations) > 0 {
+		newMcd := mcd.DeepCopy()


You can skip the deepcopy here as we have done it on L505

elankath self-assigned this Sep 16, 2026

elankath requested a review from a team as a code owner September 16, 2026 22:04

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2026

elankath added the kind/feature label Sep 16, 2026

gardener-prow Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2026

elankath force-pushed the adjust-creation-timeout branch from 65fea5a to e511cc3 Compare September 17, 2026 07:36

gardener-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2026

aaronfern reviewed Sep 22, 2026

View reviewed changes

elankath added 6 commits September 24, 2026 13:36

adjust effective-creation-timeout smartly per MachineDeployment

e771cc3

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

updated unit tests for checkAndAdjustMachineReplaceCycleCountAndEffec…

e59c9c3

…tiveCreationTimeout Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

introduced design document and renamed to constants.DefaultMachineRep…

2f1c423

…laceCycleCountThreshold Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

revised design doc: desc for replace-cycle-count

c2136ae

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

updated proposal and addressed review feedback comment

9c224fc

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

flow diagram: removed redundant activity junction

0ba7155

Signed-off-by: elankath <tarun.ramakrishna.elankath@sap.com>

elankath force-pushed the adjust-creation-timeout branch from b6dd98a to 0ba7155 Compare September 24, 2026 08:14

gardener-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2026

r4mek reviewed Sep 24, 2026

View reviewed changes

thiyyakat reviewed Sep 24, 2026

View reviewed changes

Update pkg/util/provider/machineutils/utils.go

6e94aea

Co-authored-by: thiyyakat <meghana.thiyyakat@sap.com> Signed-off-by: Tarun Ramakrishna Elankath <tarun.ramakrishna.elankath@sap.com>

r4mek reviewed Sep 24, 2026

View reviewed changes

		// IsMachineFailed checks if machine has failed
		func IsMachineFailed(p *v1alpha1.Machine) bool {

Conversation

elankath commented Sep 16, 2026

Uh oh!

gardener-prow Bot commented Sep 16, 2026

Uh oh!

aaronfern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elankath commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elankath Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elankath Sep 24, 2026 •

edited

Loading