Revert "Reduce deployment resync duration" #5741
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lakshmimsft
approved these changes
Jun 16, 2023
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Test Results2 714 tests - 2 2 707 ✔️ - 2 2m 0s ⏱️ -7s Results for commit e7ff841. ± Comparison against base commit 952e214. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
This comment has been minimized.
This comment has been minimized.
|
I will push this fix in the next sprint after 0.22 release. |
youngbupark
added a commit
that referenced
this pull request
Jun 16, 2023
This reverts commit bf8dc0b.
This was referenced Jun 16, 2023
nithyatsu
pushed a commit
that referenced
this pull request
Jun 21, 2023
# Description This reverts commit 6d16267. ## Issue reference <!-- We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation. --> Fixes: #issue_number ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [ ] Code compiles correctly * [ ] Adds necessary unit tests for change * [ ] Adds necessary E2E tests for change * [ ] Unit tests passing * [ ] Extended the documentation / Created issue for it ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖 Generated by Copilot at a80b7b6</samp> ### Summary 🧹🧪🔧 <!-- 1. 🧹 for simplifying the timeout configuration and removing unused code. 2. 🧪 for updating the unit tests and improving the test coverage. 3. 🔧 for refactoring the code and enhancing the functionality. --> This pull request simplifies and improves the timeout configuration and deployment status checking logic for the `corerp` package. It removes unused and redundant code, refactors the `handlers` package to use the API server as the source of truth, and updates the unit tests accordingly. It also deletes the `timeout.go` file, which is no longer needed. > _Simplify the timeout, remove the dead code_ > _`handler` package is ready to explode_ > _Refactor the status, handle the error_ > _`waitUntilDeploymentIsReady` is our terror_ ### Walkthrough * Remove unused `corerp_config` package import from `handler` and `handlers` packages ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-2bdfa10a8abd451bfb18e729290e9182d1b8e6280d8a9e47dd0ce70f6e8d48d6L35-L36), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L26)) * Add `MaxDeploymentTimeout` constant to `handler` and `handlers` packages and use it to set the maximum duration for waiting for a deployment to be ready in `AddRoutes` and `NewKubernetesHandler` functions ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-2bdfa10a8abd451bfb18e729290e9182d1b8e6280d8a9e47dd0ce70f6e8d48d6L315), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-2bdfa10a8abd451bfb18e729290e9182d1b8e6280d8a9e47dd0ce70f6e8d48d6L333), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L45-R49), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L52-R57)) * Change `waitUntilDeploymentIsReady` function to use `errCh` channel instead of `statusCh` channel to communicate errors, and to get the final deployment status from the API server and use the status message and reason from the latest condition as the error message when the context is done ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L129-R131), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L135-R137), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L141-R168)) * Change `startDeploymentInformer` function to pass only `doneCh` channel to `checkDeploymentStatus` function and to receive errors from `errCh` channel ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L172-R178), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L184-R190)) * Change `checkDeploymentStatus` function to only check for the complete deployment condition and log the status type, status, and reason when the condition is true ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L201-R220)) * Modify `TestWaitUntilDeploymentIsReady_Timeout` function to use a single test case with a fixed timeout value, and to expect the error message and reason from the latest condition of the deployment ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-1539f100bbb77ae21828edd7fd8b07a632dc9629ff90ce80d3067626f8051947L309-R309), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-1539f100bbb77ae21828edd7fd8b07a632dc9629ff90ce80d3067626f8051947L340-R322), [link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-1539f100bbb77ae21828edd7fd8b07a632dc9629ff90ce80d3067626f8051947L347-R338)) * Modify `TestWaitUntilDeploymentIsReady_DifferentResourceName` function to expect the error message to be wrapped with the failure to get the deployment from the API server ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-1539f100bbb77ae21828edd7fd8b07a632dc9629ff90ce80d3067626f8051947L400-R378)) * Add `metav1` package import to `handlers` package to access the `GetOptions` type in `waitUntilDeploymentIsReady` function ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7R35)) * Change the value of `DefaultCacheResyncInterval` constant in `handlers` package ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-033dc8a7c9b970cc30f7420ce8cf2f12a26258b276779683ba3f761c481bf0a7L45-R49)) * Delete `pkg/corerp/config/timeout.go` file ([link](https://github.com/project-radius/radius/pull/5741/files?diff=unified&w=0#diff-9e80ec717929fd422805a06a07858d4f9871437e41ec36df9f0cd583ea46f428))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This reverts commit 6d16267.
Issue reference
Fixes: #issue_number
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Auto-generated summary
🤖 Generated by Copilot at a80b7b6
Summary
🧹🧪🔧
This pull request simplifies and improves the timeout configuration and deployment status checking logic for the
corerppackage. It removes unused and redundant code, refactors thehandlerspackage to use the API server as the source of truth, and updates the unit tests accordingly. It also deletes thetimeout.gofile, which is no longer needed.Walkthrough
corerp_configpackage import fromhandlerandhandlerspackages (link, link)MaxDeploymentTimeoutconstant tohandlerandhandlerspackages and use it to set the maximum duration for waiting for a deployment to be ready inAddRoutesandNewKubernetesHandlerfunctions (link, link, link, link)waitUntilDeploymentIsReadyfunction to useerrChchannel instead ofstatusChchannel to communicate errors, and to get the final deployment status from the API server and use the status message and reason from the latest condition as the error message when the context is done (link, link, link)startDeploymentInformerfunction to pass onlydoneChchannel tocheckDeploymentStatusfunction and to receive errors fromerrChchannel (link, link)checkDeploymentStatusfunction to only check for the complete deployment condition and log the status type, status, and reason when the condition is true (link)TestWaitUntilDeploymentIsReady_Timeoutfunction to use a single test case with a fixed timeout value, and to expect the error message and reason from the latest condition of the deployment (link, link, link)TestWaitUntilDeploymentIsReady_DifferentResourceNamefunction to expect the error message to be wrapped with the failure to get the deployment from the API server (link)metav1package import tohandlerspackage to access theGetOptionstype inwaitUntilDeploymentIsReadyfunction (link)DefaultCacheResyncIntervalconstant inhandlerspackage (link)pkg/corerp/config/timeout.gofile (link)