feat: add stopping stage update run implementation#374
feat: add stopping stage update run implementation#374britaniar merged 7 commits intokubefleet-dev:mainfrom
Conversation
britaniar
force-pushed
the
stopUpdateRun
branch
from
040bf28 to
878fa65
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
britaniar
force-pushed
the
stopUpdateRun
branch
from
878fa65 to
723e9c9
Compare
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
britaniar
force-pushed
the
stopUpdateRun
branch
from
723e9c9 to
4bee175
Compare
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
| validateUpdateRunMetricsEmitted(wantMetrics...) | ||
| }) | ||
|
|
||
| It("Should delete all the clusterResourceBindings in the delete stage and complete the update run", func() { |
There was a problem hiding this comment.
Can we add a case where we stop in between the deletion stage ?
There was a problem hiding this comment.
I tried previously and we have no control over the clusters in the stage so the stop timing here is difficult.
There was a problem hiding this comment.
We can add a finalizer on the bindings so that they are not deleted.
| validateUpdateRunMetricsEmitted(wantMetrics...) | ||
| }) | ||
|
|
||
| It("Should wait for cluster to finish updating before update run is completely stopped", func() { |
There was a problem hiding this comment.
This "It" section is redundant unless you want to check updateRun is in stopping state CONSISTENTLY.
| validateUpdateRunMetricsEmitted(wantMetrics...) | ||
| }) | ||
|
|
||
| It("Should delete all the clusterResourceBindings in the delete stage and complete the update run", func() { |
There was a problem hiding this comment.
We can add a finalizer on the bindings so that they are not deleted.
| "github.com/kubefleet-dev/kubefleet/pkg/utils/condition" | ||
| ) | ||
|
|
||
| var _ = Describe("UpdateRun stop tests", func() { |
There was a problem hiding this comment.
Not a blocker to this PR, but this test is too complicated, including many unnecessary tests for stopping. If we can create a small number of updating/deleting clusters and a simpler strategy to just test the stop cases, that's better. We actually have one https://github.com/kubefleet-dev/kubefleet/blob/main/pkg/controllers/updaterun/controller_integration_test.go#L473C1-L490C2 This one does not have any to-be-deleted bindings though. Please feel free to add if you want to update.
|
Will address any unresolved stop integration test specific comments in a follow up PR. Current integration test and UTs should have major coverage. |
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
britaniar
force-pushed
the
stopUpdateRun
branch
from
0ac9549 to
eb10bb3
Compare
3 participants
Description of your changes
I have:
Added a check to make sure no cluster start updating and let currently updating cluster finish within a stage before marking the stage and update run as stopped.
Update the stage condition status as stopping or stopped.
Update integration tests and added UTs.
Run
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
While waiting for cluster to finish updating, stage and update run will have a progressing unknown condition with the reason as stopping.