-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow specifying PropagationPolicy when using deleteExisting #2676
Allow specifying PropagationPolicy when using deleteExisting #2676
Conversation
Can one of the admins verify this patch? |
I have a very hard time running kubernetes-client tests locally or in my company's build environment. Will mark this PR ready for review once the checks above have passed. |
c255356
to
49d2eae
Compare
Which tests are you referring to? Mock Server tests in |
I can basically never get the project to fully build in IntelliJ, usually due to weird configuration issues with all of the generated models or some other compile issue in one of the various extensions. One major issue is that fabric8's generated classes all go to I can get it to work using I can't run the regression tests in |
return createTask.apply(item); | ||
try { | ||
return createTask.apply(item); | ||
} catch (KubernetesClientException e) { |
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.
I don't think this is related to my original change to support PropagationPolicy, but my new integration test caught this issue. The default grace period is 30s, so unless you explicitly specify 0 seconds (which we don't here and probably shouldn't if we could) it's possible that the DELETE call returns a 202 instead of 200, meaning it hasn't actually been deleted yet.
BaseOperation/OperationSupport/etc assume that a non-error response means the item was deleted. This is sort of true, but not really since it may have actually been queued for asynchronous deletion (per the 202 response). I looked into whether I could pipe the response Status through as the return value of the various delete()
functions, but that proved to be a significant change (and probably a breaking one, since right now we expect to return a simple Boolean).
The simpler fix here seems to be to add a waiting period as I did here. Alternatively we could continually retry createTask for a period of time, but this might be less impactful on API servers?
I think this is ready for review. The E2E tests all passed, and the unit tests all pass locally. The builds both failed due to some sort of timeout downloading dependencies. Any chance you can kick off another build? |
You're not the first one pointing this out :-( (#2118). This project has grown a lot over time and became quite bulky. I don't know how other manage but I usually load project as maven project in my IntelliJ after For integration tests, I have to add |
Looks like builds are still having issues. Not sure if there's something bigger going on with builds currently |
await for deletion if the just-deleted resource still exists when trying to create
63d9f26
to
173ab63
Compare
Thanks, I'll give that a shot. Any other comments on this PR? It's ready to be merged I think. The build is failing for unrelated reasons, but I can't kick it off myself. It succeeds locally. |
@bbeaudreault there's a conflict |
Whoops, sorry! I'll take care of that shortly |
Is it possible to get access to retry these PR checks? They seem flaky and I hate to keep bugging you guys. I know I can force push to kick them off, but I'd rather not have to re-run all of the checks each time, instead just the ones that failed. The K8S v1.18.6 failed this time, but it has been succeeding consistently before. It failed in a test unrelated to my change. |
I'm not sure you can through GH interface Today it's specially bad, Maven Central is also not very reliable today :( |
Sounds good. I see someone kicked off a new build, and the E2E all passed 🎉 . Unfortunately the build java tasks are still stuck from the previous run -- they didn't restart. |
All of the build tasks are getting stuck. I think it might have to do with the tests and the new retry behavior. |
While testing this change, I found out that Pod pod = client.pods().load(CreateOrReplaceResource.class.getResourceAsStream("/test-pod.yml")).get();
client.resource(pod).inNamespace("default").deletingExisting().createOrReplace(); This would fail with this exception:
The same code works without any problems on v4.12.0 I think we should add a check whether resource exists before deleting it here: Lines 38 to 42 in 7918764
It's not really related to this PR since I had done this refactor. Maybe we can tackle this as a part of this PR or in separate issue. |
@rohanKanojia I'm happy to add this here. Would you be ok with my just catching and handling the KubernetesClientException, rather than adding a new API call? |
I checked and looks like Kubernetes Client only returns Lines 699 to 703 in 7918764
I think we should not throw the exception when deleteTask.apply(item) returns false . We should simply proceed to creation.
|
Cool, I'm making that change now |
FYI that change was of course easy enough to make, but I realized that my local maven actually wasn't running all of the tests. It was failing on karaf, which I thought was the last one, but actually was missing kubernetes-tests. I excluded kubernetes-tests and noticed that the build is actually hanging locally as well! It appears to be hanging in one of my deleteExisting tests. I'm trying to debug it but it's slow going because I can't run it in intellij, so can't easily debug. Still working through it. There appears to be a 400 client error being thrown in the test which causes one of the |
…ted=false, and update tests
I've pushed the change we discussed, along with a little sanity checking. I also pushed the fix for the hung tests. Sorry for the runaround on that one... the lack of a local test env really got me. I was able to hack around it a bit, hence how i discovered the actual issue. Note that I can't use your suggested "Delegate IDE to maven" setting because there are no 5.0.0-SNAPSHOTs uploaded to maven central. I can't do a mvn install first because of my company's required local maven settings. I still can't run the full suite in its entirety, so will keep an eye on the checks below and followup with fixes as needed. |
Looks like everything passed! |
// depending on the grace period, the object might not actually be deleted by the time we try to create | ||
// if that's the case, give it some time. |
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.
Is this comment still accurate?
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, still accurate. All of the stuff I mentioned about DELETE returning a 200 or 202 still applies. So we still need to wait for the deletion to occur before creating, otherwise we'll get a conflict.
kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java
Outdated
Show resolved
Hide resolved
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.
LGTM (besides the constant comment). Thx!
@manusa all set, thanks! |
SonarCloud Quality Gate failed. |
Anything else you guys want to see here? Everything but the code coverage check is passing. I did add tests here, which seem sufficient? Let me know |
Thanks a lot @bbeaudreault |
Description
As the title says, this is something the API supports but kubernetes-client's generics were just not set up properly to allow crafting such a request. This updates the generics to support that use-case. Added an IT and unit test for the new functionality.
Type of change
test, version modification, documentation, etc.)
Checklist