Skip to content

Conversation

@tedyu
Copy link
Contributor

@tedyu tedyu commented Dec 7, 2022

What changes were proposed in this pull request?

This is follow-up to commit cc55de3 where PVC_COUNTER was introduced to track outstanding number of PVCs.
PVC_COUNTER should only be decremented when the pod deletion happens (in response to error).

Why are the changes needed?

If the PVC isn't created successfully (where PVC_COUNTER isn't incremented) (possibly due to execution not reaching resource(pvc).create() call), we shouldn't decrement the counter.

success tracks the progress of PVC creation:
value 0 means PVC is not created.
value 1 means PVC has been created.
value 2 means PVC has been created but due to subsequent error, the pod is deleted.

The counter is decremented when either KUBERNETES_DRIVER_OWN_PVC or KUBERNETES_DRIVER_REUSE_PVC has false value since the PVC is not owned by Driver pod.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

@dongjoon-hyun
Please take a look.

I am trying to figure out how to add a test.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR but we need to remove PVC_COUNTER.decrementAndGet() only.

@dongjoon-hyun
Copy link
Member

In other words, please revert all changes and remove one line, PVC_COUNTER.decrementAndGet().

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

That's not the right way :-)

See #38943 (comment)

@dongjoon-hyun
Copy link
Member

Okay. Since we don't agree, I will make my PR too. We can compare side-by-side, @tedyu . :)

@dongjoon-hyun
Copy link
Member

BTW, we need to add the test case to validate the ideas. I'll try to add to my PR. You may can reuse it.

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

If exception happens before we reach the following line:

            kubernetesClient.persistentVolumeClaims().inNamespace(namespace).resource(pvc).create()

the counter shouldn't be decremented.
If exception happens after the following line:

            PVC_COUNTER.incrementAndGet()

the counter should be decremented.

We need to make this code future proof.

@dongjoon-hyun
Copy link
Member

Please make a valid est case for your claim.

@dongjoon-hyun
Copy link
Member

This is handled properly by removing decrement line.

the counter shouldn't be decremented.

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

My point is: when exception happens, the exception may not come from this call:

            kubernetesClient.persistentVolumeClaims().inNamespace(namespace).resource(pvc).create()

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

@dongjoon-hyun
May I borrow your new test case to show that my PR covers that failure scenario ?

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

e.g.
the test can produce exception when the following is called:

         newlyCreatedExecutors(newExecutorId) = (resourceProfileId, clock.getTimeMillis())

@dongjoon-hyun
Copy link
Member

It's totally fine because spark.kubernetes.driver.reusePersistentVolumeClaim=true. We can reuse that PVC later, @tedyu .

e.g. the test can produce exception when the following is called:

         newlyCreatedExecutors(newExecutorId) = (resourceProfileId, clock.getTimeMillis())

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

If possible, can you elaborate a bit ?

If exception happens at newlyCreatedExecutors(newExecutorId) = (or later in the try block), the pod would be deleted.
Why shouldn't PVC_COUNTER be decremented (note: it has been incremented after PVC creation) ?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Dec 7, 2022

@tedyu . It seems that you forgot spark.kubernetes.driver.ownPersistentVolumeClaim=true. Pod deletion doesn't clean up PVCs. It's owned by Driver pod. This is not your bug. That was originated from my bug. :)

If exception happens at newlyCreatedExecutors(newExecutorId) = (or later in the try block), the pod would be deleted.
Why shouldn't PVC_COUNTER be decremented (note: it has been incremented after PVC creation) ?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Dec 7, 2022

Choose a reason for hiding this comment

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

In short, this is wrong because pod deletion doesn't change the total number of PVCs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the logic.
Can you take another look ?

Copy link
Member

Choose a reason for hiding this comment

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

PVC_COUNTER is only used by SPARK-41410 . !reuse is a misleading and useless code, isn't it?

if (reusablePVCs.isEmpty && podAllocOnPVC && maxPVCs <= PVC_COUNTER.get()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable podAllocOnPVC

  private val podAllocOnPVC = conf.get(KUBERNETES_DRIVER_OWN_PVC) &&
    conf.get(KUBERNETES_DRIVER_REUSE_PVC) && conf.get(KUBERNETES_DRIVER_WAIT_TO_REUSE_PVC)

includes one more condition for KUBERNETES_DRIVER_WAIT_TO_REUSE_PVC.

That's why I think it doesn't hurt to check again.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

If you insist to use !reuse in order to focus on an unused condition's PVC_COUNTER variable,
this PR becomes more and more irrelevant to the original SPARK-41410. It doesn't look like a follow-up of SPARK-41410.

@tedyu .

  • I'd like to recommend you to take a look at my PR.
  • Or, use a different JIRA id for your PR.

@tedyu tedyu changed the title [SPARK-41410][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens [SPARK-41419][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens Dec 7, 2022
@tedyu tedyu changed the title [SPARK-41419][K8S][FOLLOW-UP] Decrement PVC_COUNTER when the pod deletion happens [SPARK-41419][K8S] Decrement PVC_COUNTER when the pod deletion happens Dec 7, 2022
@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

@dongjoon-hyun @viirya
I have modified the subject and description of this PR.

Please take another look.

@tedyu
Copy link
Contributor Author

tedyu commented Dec 7, 2022

In ExecutorPodsAllocatorSuite.scala, the pair of configs always have the following values:

      .set(KUBERNETES_DRIVER_OWN_PVC.key, "true")
      .set(KUBERNETES_DRIVER_REUSE_PVC.key, "true")

If one of them is false, requestNewExecutors would proceed with creating pod and PVC.

@dongjoon-hyun
Do you know why there is no test with either of these configs as false ?

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Mar 18, 2023
@github-actions github-actions bot closed this Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants