Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add max-reconcile-rate and use latest tools #58

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

bobh66
Copy link
Collaborator

@bobh66 bobh66 commented May 26, 2022

Signed-off-by: Bob Haddleton bob.haddleton@nokia.com

Description of your changes

Updated the provider using the template from crossplane/provider-template@404847c

to support multithreading.

The included update to the latest crossplane-runtime version exposed a problem with the Update function setting the Available
condition in the Create() sequence, since Create calls Update, and the Reconciler then overwrites the same condition with Creating. This was resolved by refactoring the Observe function to also set Available whenever there are no differences reported.

Also added a semaphore to the terraformInit function so that there can be only one execution of terraform init running at any one time. This is to avoid problems with a shared Plugin Cache being modified by two executions of terraform init simultaneously.

And the change to the latest zap/zapr caused the timestamp in the logs to change to Epoch time (seconds since 1/1/1970) instead of ISO8601 format, so I added code to request all times be formatted as ISO8601 for readability.

Fixes #57

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Code is running in our development environment.

@bobh66 bobh66 changed the title Add max-reconcile-rate and use latest tools WIP: Add max-reconcile-rate and use latest tools May 26, 2022
@bobh66
Copy link
Collaborator Author

bobh66 commented May 26, 2022

I set WIP on this while I investigate an anomaly that showed up in our test environment. Please hold off on merging until I can understand what's going on

@bobh66 bobh66 changed the title WIP: Add max-reconcile-rate and use latest tools Add max-reconcile-rate and use latest tools Jul 10, 2022
@bobh66 bobh66 requested a review from ytsarev July 10, 2022 15:26
Copy link
Collaborator

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

@bobh66 did you manage to investigate?

@bobh66
Copy link
Collaborator Author

bobh66 commented Jul 27, 2022

Hi @ytsarev - yes, I found that the issue with the update to provider-terraform had nothing to do with the multithreading but was caused by the combination of the updated crossplane-runtime and the fact that provider-terraform set Available() only once in the Update() function (which gets called for Create()). In the new crossplane-runtime version the Reconciler sets Creating() after calling external.Create() which wiped out the Available() which had been set by the provider, since the Reconciler doesn't consider the case where the resource is available immediately upon creation. I updated provider-terraform to work more like provider-kubernetes and refresh the Available() status condition on every call to Observe() so it is always correct.

This PR should be good to go.

@bobh66 bobh66 changed the title Add max-reconcile-rate and use latest tools DNM: Add max-reconcile-rate and use latest tools Jul 29, 2022
@bobh66
Copy link
Collaborator Author

bobh66 commented Jul 29, 2022

@ytsarev The CRDs generated by these changes are missing the apiVersion - the same thing broke provider-kubernetes main branch when it merged. Hold off on this merge until I can find and fix the problem. Thanks

@bobh66 bobh66 changed the title DNM: Add max-reconcile-rate and use latest tools Add max-reconcile-rate and use latest tools Jul 29, 2022
@bobh66
Copy link
Collaborator Author

bobh66 commented Jul 29, 2022

@ytsarev - I found the issue that was causing the missing apiVersion and fixed in latest commit. Thanks

Copy link
Collaborator

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

@bobh66 nice findings! Could you please rebase this branch with main. Commit log does not look clean.

Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Copy link
Collaborator

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Thanks a lot, looks like it is ready for merge

@bobh66 bobh66 merged commit 89b2d87 into crossplane-contrib:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multithreading
2 participants