flux-framework / flux-k8s

Project to manage Flux tasks needed to standardize kubernetes HPC scheduling interfaces
Apache License 2.0
21 stars 10 forks source link

Adds job cancellation of flux jobs #22

Closed xyloid closed 2 years ago

xyloid commented 2 years ago

This PR adds flux job cancellation function to kubeflux scheduler which enables kubeflux to cancel a flux job after it's corresponding pod completed (Its PodPhase can be either PodSucceeded or PodFailed). Thus the previously allocated resources can be freed and reused.

dongahn commented 2 years ago

I just added myself as a reviewer. Welcome @xyloid!!

Two high level comments first:

  1. Please add a high level summary of what this PR is about. Reviewing individual commits without the high level synapsis makes it hard to assess whether this PR has a good transactional topic or not. (As an example you can look at recently merged PRs in other repos like flux-core, flux-sched, flux-accounting -- e.g., https://github.com/flux-framework/flux-sched/pull/871)
  2. As part of that, please comment why your PR includes @milroy's. Is that because a WIP PR was used to create this PR.

Flux's RFC1 on code-development workflow (called C4 which is a variant of fork/pull model) might be useful if you haven't reviewed it yet.

xyloid commented 2 years ago

I just added myself as a reviewer. Welcome @xyloid!!

Two high level comments first:

1. Please add a high level summary of what this PR is about. Reviewing individual commits without the high level synapsis makes it hard to assess whether this PR has a good transactional topic or not. (As an example you can look at recently merged PRs in other repos like flux-core, flux-sched, flux-accounting -- e.g., [Add `sched.feasibility` service for flux-core's jobspec feasibility validator  flux-sched#871](https://github.com/flux-framework/flux-sched/pull/871))

2. As part of that, please comment why your PR includes @milroy's. Is that because a WIP PR was used to create this PR.

Flux's RFC1 on code-development workflow (called C4 which is a variant of fork/pull model) might be useful if you haven't reviewed it yet.

Thank you for the comments! I will work on comment 1. As for comment 2, I think I mistakenly clicked fetch upstream on my github. Otherwise, I don't know why those commits show up in this branch. Let me try if I can remove them from this branch.

dongahn commented 2 years ago

As for comment 2, I think I mistakenly clicked fetch upstream on my github. Otherwise, I don't know why those commits show up in this branch. Let me try if I can remove them from this branch.

I think those commits disappeared now!!

I don't know exactly how this state was created. But I typically use the following sequence before post a PR. In your forked dev branch:

% git remote add upstream git@github.com:flux-framework/flux-k8s.git (if you haven't done)
% git fetch upstream
% git rebase upstream/master

This will allow your changed to be on top of the head of the upstream master.

xyloid commented 2 years ago

As for comment 2, I think I mistakenly clicked fetch upstream on my github. Otherwise, I don't know why those commits show up in this branch. Let me try if I can remove them from this branch.

I think those commits disappeared now!!

I don't know exactly how this state was created. But I typically use the following sequence before post a PR. In your forked dev branch:

% git remote add upstream git@github.com:flux-framework/flux-k8s.git (if you haven't done)
% git fetch upstream
% git rebase upstream/master

This will allow your changed to be on top of the head of the upstream master.

I used git rebase -i HEAD~n to drop those commits. Thank you for the help with git! I guess I clicked something wrong in github webpage that related to branch history. Next time I will use command line.

xyloid commented 2 years ago

After a short discussion with @cmisale , I think this PR must be spitted out into 2 1- the actual changes to the go code 2- the example for the pi calculation

I have created a new PR(#23) for the pi calculation, and now this PR only contains go code changes.