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.

Sign up for GitHub

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

Jump to bottom

Fabric8 deletion behaviour fix #2241

Merged
merged 2 commits into from
Jan 17, 2020
Merged

Fabric8 deletion behaviour fix #2241

merged 2 commits into from
Jan 17, 2020

Conversation

sknot-rh
Copy link
Member

@sknot-rh sknot-rh commented Nov 25, 2019 •
edited
Loading

Signed-off-by: Stanislav Knot sknot@redhat.com

Type of change

  • Bugfix

Description

Fixes #2223

After fabric8io/kubernetes-client#1807 was merged in fabric8 I think this bug should be fixed.
//edit: using 4.6.4 did not seem to help, using suggested cascading(true) where possible.

Checklist

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

@sknot-rh sknot-rh added this to the 0.15.0 milestone Nov 25, 2019
@sknot-rh
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

❗ Build Failed ❗

@sknot-rh
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

❗ Build Failed ❗

@sknot-rh
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

❗ Build Failed ❗

@sknot-rh
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

✔️ Test Summary ✔️

TEST_PROFILE: acceptance
TEST_CASE: *ST
TOTAL: 18
PASS: 18
FAIL: 0
SKIP: 0

@sknot-rh
Copy link
Member Author

After a few tests runs it seems there is still an issue with deleting resources, so I reverted my reversion and added cascading(true) to all occurrences of delete(resource).

@scholzj scholzj modified the milestones: 0.15.0, 0.16.0 Dec 3, 2019
@sknot-rh sknot-rh force-pushed the delete-f8 branch 2 times, most recently from 0e9a0bf to 73ba533 Compare December 16, 2019 07:38
@sknot-rh
Fabric8 deletion behaviour fix
45340a4 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
rebase
565f44f 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
ppatierno
ppatierno approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

lgtm. I guess all deleting operations were fixed (I didn't search for all of them tbh). :-)

tombentley
tombentley reviewed Jan 8, 2020
View reviewed changes
mockkube/src/test/java/io/strimzi/test/io/strimzi/test/mockkube/MockKubeTest.java
@@ -256,7 +256,7 @@ public void podNameScopedCreateListGetDelete(Class<RT> cls,
gotResource = mixedOp.apply(client).withName(pod.getMetadata().getName()).get();
assertThat(gotResource, is(nullValue()));

assertThat(mixedOp.apply(client).withName(pod.getMetadata().getName()).delete(), is(false));
assertThat(mixedOp.apply(client).withName(pod.getMetadata().getName()).cascading(true).delete(), is(false));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the MockKube properly supports cascading deletion (i.e. I suspect it behaves the same for a cascading as a non-cascading delete), but if the tests pass I guess it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the test pass.

@scholzj
Copy link
Member

scholzj commented Jan 8, 2020

TBH, I'm a bit unsure abotu this.

  • It seems ot be adding it to delete operations where no cascading should be happening
  • It seems to add it only to tests. Do we have no delete operations what so ever in our regular code? Is it that we delete everything with GC and don't use it in the actual applications?

The STs seem to have passed, so I'm fine with this as it does not seem to do any damage. But I'm not sure I understand this.

@sknot-rh
Copy link
Member Author

sknot-rh commented Jan 9, 2020

I remember this "not so clear" modification was used in some topic ST. I tried to remove cascading(true) there and the test become flaky. Adding cascading(true) where applicable should prevent tests from being flaky in the matter of deleting resources.

@scholzj scholzj modified the milestones: 0.16.0, 0.17.0 Jan 10, 2020
tombentley
tombentley approved these changes Jan 15, 2020
View reviewed changes
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

I agree with Jakub that (at least some of) these changes sholdn't really be necessary, but neither should they be harmful. And it's at least plausible that the cascading delete (even in the absence of child resources) make deletion synchronous, and thus a test more reliable.

@sknot-rh sknot-rh added the ready for merge Label for PRs which are ready for merge label Jan 16, 2020
@scholzj scholzj merged commit 2eaece4 into strimzi:master Jan 17, 2020
@sknot-rh sknot-rh deleted the delete-f8 branch January 22, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombentley tombentley tombentley approved these changes

@ppatierno ppatierno ppatierno approved these changes

@scholzj scholzj Awaiting requested review from scholzj

Assignees
No one assigned
Labels
ready for merge Label for PRs which are ready for merge
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

Fix Fabric8 client delete behavior due to new 4.6.x version
5 participants
@sknot-rh @strimzi-ci @scholzj @tombentley @ppatierno