Skip to content

Conversation

vijtrip2
Copy link
Contributor

Description of changes:
Commit#1

Commit#2

  • Fixes the patching bug in lateInitialize code to unblock sagemaker team
  • BUG: rm.LateInitialize() method was adding lateInitialized fields to the same object passed in the parameter and returning as output.
        lateInitializedLatest, err := rm.LateInitialize(ctx, latest)
	rlog.Exit("rm.LateInitialize", err)
	// Always patch after late initialize because some fields may have been initialized while
	// others require a retry after some delay.
	// This patching does not hurt because if there is no diff then 'patchResourceMetadataAndSpec'
	// acts as a no-op.
	if ackcompare.IsNotNil(lateInitializedLatest) {
		patchErr := r.patchResourceMetadataAndSpec(ctx, latest, lateInitializedLatest)
		// Throw the patching error if reconciler is unable to patch the resource with late initializations
		if patchErr != nil {
			err = patchErr
		}
	}
  • Since lateInitializedLatest and latest were same object, above patchResourceMetadataAndSpec call sees no diff and does not patch lateInitialized fields into etcd.
  • This PR solves the bug by adding lateInitialized fields in a copy of latest and returning that copy (without modifying latest)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Member

@ryansteakley ryansteakley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested these changes locally using late initialize for trainingjob field and it now sucessfully late initializes the field and the resource is correctly patched

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat 👍

@RedbackThomson
Copy link
Contributor

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Aug 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson, ryansteakley, vijtrip2

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [A-Hilaly,RedbackThomson,vijtrip2]

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

@ack-bot ack-bot merged commit 07d7ea7 into aws-controllers-k8s:main Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants