feat: use 'foreground' delete policy to cancel k8s job

qwtsc commented 9 months ago

Purpose of the PR

bypass the test case issue, and add more info when error occurs.

Main Changes

Verifying these changes

[x] Trivial rework / code cleanup without any test coverage. (No Need)
[ ] Already covered by existing tests, such as (please modify tests here).
[ ] Need tests and can be verified as follows.

Does this PR potentially affect the following parts?

[x] Nope
[ ] Dependencies (add/update license info)
[ ] Modify configurations
[ ] The public API
[ ] Other affects (typed here)

Documentation Status

[ ] Doc - TODO
[ ] Doc - Done
[x] Doc - No Need

coderzc commented 9 months ago

It seems Enum already overrides write toString

javeme commented 9 months ago

maybe we need to implement DefaultJobState.toString()?

qwtsc commented 9 months ago

maybe we need to implement DefaultJobState.toString()?

Done, and I have found the issue that blocked the minikube test. please review the latest changed files.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (ff85e34) 85.03% compared to head (1133673) 85.07%. Report is 3 commits behind head on master.

Files	Patch %	Lines
...egraph/computer/algorithm/sampling/RandomWalk.java	92.06%	2 Missing and 3 partials :warning:
...thm/centrality/betweenness/BetweennessMessage.java	0.00%	2 Missing :warning:
.../algorithm/community/cc/ClusteringCoefficient.java	0.00%	0 Missing and 1 partial :warning:
...computer/algorithm/sampling/RandomWalkMessage.java	94.11%	1 Missing :warning:
...che/hugegraph/computer/driver/DefaultJobState.java	75.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #290 +/- ## ============================================ + Coverage 85.03% 85.07% +0.03% - Complexity 3246 3278 +32 ============================================ Files 345 349 +4 Lines 12298 12411 +113 Branches 1102 1113 +11 ============================================ + Hits 10458 10559 +101 - Misses 1315 1320 +5 - Partials 525 532 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

javeme commented 9 months ago

thanks and could give more context about the issue/bug?

it may be this issue: https://github.com/apache/incubator-hugegraph-computer/pull/287#discussion_r1403393492

qwtsc commented 9 months ago

I still can't find the root cause. Let's keep this PR open for another two or three days. If it's still mysterious to me, we can append it to PR #287.

qwtsc commented 9 months ago

@coderzc @javeme @imbajin I finally find the actual reason.

the default delete policy in k8s has changed to background since v1.20.

Plus the way you added finalizer is replaceCR, so you will sometimes miss the delete event generated by your client.

private boolean finalizer(HugeGraphComputerJob computerJob) {
    if (computerJob.addFinalizer(FINALIZER_NAME)) {
--->   // this is partial reason, can u use patch CR instead?
        this.replaceCR(computerJob);
        return true;
    }

    ComputerJobStatus status = computerJob.getStatus();
    if (computerJob.isMarkedForDeletion()) {
        if (!JobStatus.finished(status.getJobStatus())) {
            status.setJobStatus(JobStatus.CANCELLED.name());
            this.updateStatus(computerJob);
        } else {
            if (computerJob.removeFinalizer(FINALIZER_NAME)) {
                this.replaceCR(computerJob);
            }
        }
        return true;
    } else {
        if (JobStatus.finished(status.getJobStatus())) {
            if (this.autoDestroyPod) {
                this.deleteCR(computerJob);
            }
            return true;
        }
    }

    return false;
}

k8s will deleted its owned resource in the background whether the CR is deleted or not, so the calculated job status will always remain INITIALIZING, then the operator won't deploy again.

    ComputerJobComponent observed = this.observeComponent(computerJob);
----> //calculated status remains INITIALIZING forever, so won't deploy it again.
    if (!this.updateStatus(observed) && request.retryTimes() == 0) {
        LOG.debug("Wait status to be stable before taking further actions");
        return OperatorResult.NO_REQUEUE;
    }

    if (Objects.equals(computerJob.getStatus().getJobStatus(),
                       JobStatus.RUNNING.name())) {
        String crName = computerJob.getMetadata().getName();
        LOG.info("ComputerJob {} already running, no action", crName);
        return OperatorResult.NO_REQUEUE;
    }

    ComputerJobDeployer deployer = new ComputerJobDeployer(this.kubeClient,
                                                           this.config);
    deployer.deploy(observed);

    return OperatorResult.NO_REQUEUE;

Then as human, we observe the unit-test case failed again and again.
solution

My solution is to adjust the delete policy back to foreground. Everyone is happy now.