fix: controlling issues #5756
Merged
fix: controlling issues #5756
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
…orkflowResult model
…hing more resilient to external problems, expose more Kubernetes error details
…orkers and services
vsukhin
reviewed
Aug 13, 2024
| if nodeName != prevNodeName || podIP != prevPodIP || prevStatus != status || prevCurrent != current { | ||
| // TODO: the final status should always have the finishedAt too, | ||
| // there should be no need for checking isFinished diff | ||
| if nodeName != prevNodeName || isFinished != prevIsFinished || podIP != prevPodIP || prevStatus != status || prevCurrent != current { |
There was a problem hiding this comment.
such code will not be simple, to maintain (
There was a problem hiding this comment.
I fully agree, although this code is rather not meant to be maintained. This PR, along actual bugfixes, is meant to "enable" the new orchestration with similar watching system.
The (mainly) WatchInstrumentedPod and TestWorkflowResult have so many edge cases handled, clock calibration, and auto-healing mechanisms implemented, that at this point it's probably better to just rewrite them based on the observations that we have for the last few months of running Test Workflows. After that, we will likely not need conditions like these at all.
I'm guessing that half of the healing mechanisms and edge cases handlers are no longer needed, considering iterations of orchestration improvements.
vsukhin
reviewed
Aug 13, 2024
pkg/testworkflows/testworkflowcontroller/watchinstrumentedpod.go
Outdated
Show resolved
Hide resolved
vsukhin
reviewed
Aug 13, 2024
vsukhin
approved these changes
Aug 13, 2024
rangoo94
added a commit
that referenced
this pull request
Aug 14, 2024
* fix: continue paused container, when the abort is requested * fix: ensure the lightweight container watcher will get `finishedAt` timestamp * chore: add minor todos * fix: configure no preemption policy by default for Test Workflows * fix: allow Test Workflow status notifier to update "Aborted" status with details * fix: ensure the parallel workers will not end without result * fix: properly build timestamps and detect finished resul in the TestWorkflowResult model * fix: use Pod/Job StatusConditions for detecting the status, make watching more resilient to external problems, expose more Kubernetes error details * chore: do not require job/pod events when fetching logs of parallel workers and services * fixup unit tests * fix: delete preemption policy setup * fixup unit tests * fix: adjust resume time to avoid negative duration * fix: calibrate clocks * chore: use consts * fixup unit tests
rangoo94
added a commit
that referenced
this pull request
Aug 14, 2024
* fix: continue paused container, when the abort is requested * fix: ensure the lightweight container watcher will get `finishedAt` timestamp * chore: add minor todos * fix: configure no preemption policy by default for Test Workflows * fix: allow Test Workflow status notifier to update "Aborted" status with details * fix: ensure the parallel workers will not end without result * fix: properly build timestamps and detect finished resul in the TestWorkflowResult model * fix: use Pod/Job StatusConditions for detecting the status, make watching more resilient to external problems, expose more Kubernetes error details * chore: do not require job/pod events when fetching logs of parallel workers and services * fixup unit tests * fix: delete preemption policy setup * fixup unit tests * fix: adjust resume time to avoid negative duration * fix: calibrate clocks * chore: use consts * fixup unit tests
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.
Pull request description
Checklist (choose whats happened)
Breaking changes
Changes
Fixes