Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Checksum reconciled Instances and Bindings #582

Merged
merged 3 commits into from
Mar 20, 2017

Conversation

pmorie
Copy link
Contributor

@pmorie pmorie commented Mar 17, 2017

Solves #573 for reprovisions

TLDR:

  • Checksum last reconciled ready=true spec
  • Controller does not do work if the spec's checksum matches the last reconciled spec

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@pmorie
Copy link
Contributor Author

pmorie commented Mar 17, 2017

I didn't realize I wasn't working from master :(

I'll rebase tomorrow. I was experiencing some local env weirdness today that I didn't have time to sort out, but integration tests should work.

@pmorie pmorie force-pushed the checksum-reconciled-instance branch from 7d40022 to dda64f6 Compare March 18, 2017 09:27
@pmorie pmorie changed the title Checksum reconciled instance WIP: Checksum reconciled instance Mar 18, 2017
@pmorie
Copy link
Contributor Author

pmorie commented Mar 18, 2017

Tested that this has the desired effect on Instance. Implemented for Binding but not tested yet.

@pmorie pmorie changed the title WIP: Checksum reconciled instance WIP: Checksum reconciled Instances and Bindings Mar 18, 2017
@pmorie pmorie force-pushed the checksum-reconciled-instance branch from f396242 to 0e1a8f2 Compare March 20, 2017 02:14
@pmorie
Copy link
Contributor Author

pmorie commented Mar 20, 2017

Verified as working on Instances and Bindings

@pmorie pmorie force-pushed the checksum-reconciled-instance branch from 0e1a8f2 to 89dbfc3 Compare March 20, 2017 02:20
@pmorie pmorie changed the title WIP: Checksum reconciled Instances and Bindings Checksum reconciled Instances and Bindings Mar 20, 2017
@pmorie pmorie force-pushed the checksum-reconciled-instance branch from 89dbfc3 to 05097cf Compare March 20, 2017 14:25
specString += fmt.Sprintf("parameters:\n\n%v\n\n", string(spec.Parameters.Raw))
}

specString += fmt.Sprintf("osbGuid: %v\n", spec.OSBGUID)
Copy link

Choose a reason for hiding this comment

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

What is the need in producing the hash with anything other than the OSBGUID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The OSBGuid is static and not expected to change. In fact, I could probably have omitted it from the checksum. The fields which are important to checksum are the ones that will mean that the instance needs to be updated at the broker. Overall I was trying to capture those; it's okay to have fields like the OSBGUID which are static.

@arschles
Copy link
Contributor

This LGTM. I have verified that instances are not re-provisioned and bindings are not re-bound as well.

Name: "test-instance",
},
SecretName: "test-secret",
Checksum: func() *string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this not just be:
Checksum: "boo"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, it's a pointer to a string :-/

allErrs = append(allErrs, ValidateBinding(new)...)
allErrs = append(allErrs, ValidateBinding(old)...)
return allErrs
return internalValidateBinding(new, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

@pmorie why only check the new binding? Bindings can't be updated?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 LGTM3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants