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
Wait for component deletion, Add --wait
flag to odo delete
#2732
Wait for component deletion, Add --wait
flag to odo delete
#2732
Conversation
d2c8f45
to
186d9c4
Compare
--wait
flag to odo delete
--wait
flag to odo delete
Codecov Report
@@ Coverage Diff @@
## master #2732 +/- ##
==========================================
- Coverage 43.67% 43.45% -0.23%
==========================================
Files 95 97 +2
Lines 8762 8812 +50
==========================================
+ Hits 3827 3829 +2
- Misses 4571 4615 +44
- Partials 364 368 +4
Continue to review full report at Codecov.
|
7d97ff0
to
1b93056
Compare
/retest |
--wait
flag to odo delete
--wait
flag to odo delete
@adisky Are you verifying the steps or torturing the code 🗡 hahaha.... |
@adisky Please cleanup |
tests/integration/component.go
Outdated
helper.CmdShouldPass("odo", "url", "create", "example-1", "--context", context) | ||
helper.CmdShouldPass("odo", "storage", "create", "storage-name-1", "--size", "1Gi", "--path", "/data-1", "--context", context) | ||
helper.CmdShouldPass("odo", append(args, "push", "--context", context)...) | ||
// delete with --wait flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adisky Deleting these lines may cause some real functionality validation miss. These lines were added due to some reason may be to catch any regression for specific scenario. IMO create a separate spec to verify your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amitkrout this test was not catching any regression, This line would always be true. https://github.com/openshift/odo/blob/master/tests/helper/helper_oc.go#L458
I have fixed the test case and removed not necessary checks. Please check the regression it missed
#2710
#2693
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can run the test case manually in verbose mode, and verify it is passing even if the pvc still exist, however it should fail here https://github.com/openshift/odo/blob/master/tests/integration/component.go#L846
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adisky Understood, but tbh you are destroying one existing scenario with your changes. Why i am saying because the scenario is
create component
create url
Create storage
odo push
create url
create storage
odo push
normal delete component
And you are skipping steps of url create, storage create and push. IIRC the steps of url create,storage create and push twice was to catch some short failure. So don't skip those steps. Rest script changes looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amitkrout sure i would add it, but we cannot add storage as of now, due to #2710
@amitkrout Please check the reported issue. it is signifying running |
588fd6e
to
e4a132b
Compare
/test v4.1-integration-e2e-benchmark |
/test v4.2-integration-e2e-benchmark |
/test v4.1-integration-e2e-benchmark |
@adisky any update on this? |
Added --wait flag to odo delete added propgation policy for deletion, setup watch on dc to recieve deleted event.
d9aa234
to
fc1c72c
Compare
yes updated it. |
oc.VerifyResourceDeleted("routes", "example", project) | ||
oc.VerifyResourceDeleted("service", cmpName, project) | ||
// verify s2i pvc is delete | ||
oc.VerifyResourceDeleted("pvc", "s2idata", project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are three PVCs, we need to check if all of them are deleted
Running oc with args [oc get pvc --namespace twmwqqprru]
[oc] NAME STATUS VOLUME CAPACITY ACCESS MODES STORAGECLASS AGE
[oc] mvfzn-hgfxb-s2idata Bound pvc-jdslkaj-9f92-ljajlds 1Gi RWO gp2-encrypted 1m
[oc] storage-1-hgfxb-pvc Bound pvc-dasjldj-9f92-dhfhd 1Gi RWO gp2-encrypted 1m
[oc] storage-2-hgfxb-pvc Bound pvc-jsalkjd-9f92-lksala 1Gi RWO gp2-encrypted 44s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding, added
Added Integration test for owned resource deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me locally
/lgtm
@amitkrout raw test logs, in case resource exist, test should fail.
|
Thnaks @adisky for discussing it over BJN. As a discussion outcome what i found that, the validation is done for the storage sub string which is printed in StdOut (In this case it's Thanks @adisky for the clarification Test looks good to me. Approvers can you please have a look @girishramnani @kadel @cdrage |
@amitkrout Thanks for the discussion and your inputs :) |
/lgtm |
defer watcher.Stop() | ||
eventCh := watcher.ResultChan() | ||
|
||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that all the case statements in the select are terminating which means we might not need the for loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes even i thought so, and removed the for loop but it did not work, since we get events on eventCh
other than error
deleted
, case events gets selected and executes none of the if conditions, returns from the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, maybe an idea would be to just v4 log those events for debugging purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, sure will update for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe lets do that in a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a follow up #2879
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: girishramnani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
It adds
--wait
flag inodo component delete
, which waits for component and its dependents to get deleted. withforegroundDeletionPolicy
kubernetes deletes the dependent first which are referringdc
withownerReference
. Then it deletes thedc
itself. we have addedwatch
todc
so that it waits for deletion ofdc
Which issue(s) this PR fixes:
Fixes #2682
How to test changes / Special notes to the reviewer:
Run
odo delete -f -w
on a deployed component,Run
oc get pvc
, check th s2i pvc gets deletedRun
oc get route
, check the route created via url gets deleted