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

[qwtsc]

Purpose of the PR

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

Main Changes

Verifying these changes

• 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?

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

Documentation Status

• Doc - TODO
• Doc - Done
• Doc - No Need

[coderzc]

It seems Enum already overrides write toString

[javeme]

maybe we need to implement DefaultJobState.toString()?

[qwtsc]

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]

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 ⚠️
...thm/centrality/betweenness/BetweennessMessage.java	0.00%	2 Missing ⚠️
.../algorithm/community/cc/ClusteringCoefficient.java	0.00%	0 Missing and 1 partial ⚠️
...computer/algorithm/sampling/RandomWalkMessage.java	94.11%	1 Missing ⚠️
...che/hugegraph/computer/driver/DefaultJobState.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[imbajin]

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

[javeme]

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

it may be this issue: #287 (comment)

[qwtsc]

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]

@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.

[javeme]

Thank you for your detailed analysis~

[imbajin]

CI error https://github.com/apache/incubator-hugegraph-computer/actions/runs/7013868991/job/19080685026?pr=290

retry

incubator-hugegraph-computer/computer-test/src/main/java/org/apache/hugegraph/computer/core/network/netty/NettyTransportClientTest.java

Line 316 in 0c447be

Mockito.doThrow(new RuntimeException("test exception")).when(serverHandler)

BTW, the u could import the hugegraph-style.xml to avoid most of the align/import problems

[qwtsc]

BTW, the u could import the hugegraph-style.xml to avoid most of the align/import problems

Got it.

I cannot repeat your failure, but it failed with another reason. I am concerned with the quality of test cases.

[coderzc]

@qwtsc Please correct the title of PR.

[qwtsc]

@qwtsc Please correct the title of PR.

done