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.

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

Explicitly marked cascade deletion as Foreground #16540

Merged
merged 4 commits into from
Apr 7, 2020

Conversation

skabashnyuk
Copy link
Contributor

@skabashnyuk skabashnyuk commented Apr 3, 2020

What does this PR do?

Explicitly marked cascade deletion as Foreground.
My assumption is that reason for #16512 is deleted but not garbage collected objects.

In this PR fabric8io/kubernetes-client#1742 introduced changes in delete behavior. As you can see here #16512 (comment)
Instead of {"apiVersion":"v1","kind":"DeleteOptions","orphanDependents":false} it sends now {"apiVersion":"v1","kind":"DeleteOptions","orphanDependents":true}. This issue was reported here fabric8io/kubernetes-client#1840.

After studying this code

I think that instead of trying to revert this behavior we should use new semantic with propagationPolicy= foreground since we don't expect some leftovers after we call delete operation. See more: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#controlling-how-the-garbage-collector-deletes-dependents.

What issues does this PR fix or reference?

#16512

Release Notes

n/a

Docs PR

n/a

@skabashnyuk skabashnyuk requested review from sparkoo and mshaposhnik and removed request for nickboldt, l0rd and rhopp April 3, 2020 13:14
@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Apr 3, 2020
@skabashnyuk skabashnyuk changed the title Explicitly marked cascade selection as Foreground [WIP] Explicitly marked cascade selection as Foreground Apr 3, 2020
@che-bot
Copy link
Contributor

che-bot commented Apr 3, 2020

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I'm not sure the changes are necessary; the requirement for foreground deletion is for k8s <1.7 which was released mid 2017, and shipped in OKD 3.7, which is no longer supported.

I recall pod orphaning issues coming up a few times in Che's history, but I don't think propagation has been the problem.

Edit: Reading the discussion on #16512, this makes more sense, ignore the comment above. However, I worry that this is solving the problem in an indirect way. We delete secrets directly as far as I can tell -- if the root cause of this issue is that we're trying to create a workspace before the secret is deleted, we could be avoiding the issue with this PR because a foreground delete will take longer (resulting in the workspace being in a STOPPING state for a few seconds longer) -- in other words, unless the secret that isn't being deleted is owned by some other object, I don't know that this PR addresses the issue directly. I'll have to play with the changes directly.

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@skabashnyuk
Copy link
Contributor Author

CC @ibuziuk @l0rd FYI.

@che-bot
Copy link
Contributor

che-bot commented Apr 6, 2020

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@dmytro-ndp
Copy link
Contributor

@mshaposhnik: please, take changes from master branch with fix of Happy path tests.

@skabashnyuk
Copy link
Contributor Author

@mshaposhnik: please, take changes from master branch with fix of Happy path tests.

@dmytro-ndp ok

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@sparkoo sparkoo left a comment

Choose a reason for hiding this comment

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

make sense now

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@skabashnyuk skabashnyuk changed the title [WIP] Explicitly marked cascade selection as Foreground Explicitly marked cascade selection as Foreground Apr 6, 2020
Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
@skabashnyuk skabashnyuk changed the title Explicitly marked cascade selection as Foreground Explicitly marked cascade deletion as Foreground Apr 6, 2020
@eclipse-che eclipse-che deleted a comment from che-bot Apr 6, 2020
@che-bot
Copy link
Contributor

che-bot commented Apr 6, 2020

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

@skabashnyuk skabashnyuk merged commit 20f0b2a into eclipse-che:master Apr 7, 2020
@skabashnyuk skabashnyuk deleted the 16512 branch April 7, 2020 06:00
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 7, 2020
ibuziuk pushed a commit that referenced this pull request Apr 10, 2020
* Explicitly marked cascade deletion as Foreground

Signed-off-by: Sergii Kabashniuk <skabashniuk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants