[slice] Update AdmissionCheck depending on success/failure. #521
Conversation
mbobrovskyi
changed the title
mbobrovskyi
force-pushed
the
slice/admission-check
branch
from
cf51f5f to
6f0c2c6
Compare
mbobrovskyi
changed the title
mbobrovskyi
requested review from
mwysokin,
mimowo,
gabesaba,
PBundyra,
mwielgus and
pajakd
as code owners
mbobrovskyi
force-pushed
the
slice/admission-check
branch
from
6f0c2c6 to
de11b5e
Compare
| if len(relevantChecks) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Should there be exactly one? If yes, should we assert that that is the case?
If we have >1 slice for the workload, will we just use 1 AC?
There was a problem hiding this comment.
Yeah, you right. Than I think we should merge #520 first.
There was a problem hiding this comment.
As we discussed, we don’t need to have multiple admission checks.
There was a problem hiding this comment.
Should we throw an error if there is more than 1, since this should never occur?
There was a problem hiding this comment.
Not sure, to be honest. Maybe we should create a webhook to validate whether an AdmissionCheck already exists. What do you think?
mbobrovskyi
force-pushed
the
slice/admission-check
branch
2 times, most recently
from
f1777b1 to
80c5088
Compare
|
@gabesaba I've addressed all comments. Could you please take a look? |
| currentCondition := ptr.Deref(apimeta.FindStatusCondition(ac.Status.Conditions, kueue.AdmissionCheckActive), metav1.Condition{}) | ||
| newCondition := metav1.Condition{ | ||
| Type: kueue.AdmissionCheckActive, | ||
| Status: metav1.ConditionTrue, |
There was a problem hiding this comment.
Is there a mechanism (in Kueue?) which eventually transitions this to false if this controller fails?
There was a problem hiding this comment.
As I see in the provisioning request controller, we set it to False only when there is a validation error in the admission check config (see here). We are not using the admission check config, so we don’t need it. Therefore, I think we should just set it to true.
| if len(relevantChecks) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Should we throw an error if there is more than 1, since this should never occur?
mbobrovskyi
force-pushed
the
slice/admission-check
branch
from
80c5088 to
3caa690
Compare
mbobrovskyi
force-pushed
the
slice/admission-check
branch
2 times, most recently
from
6b0683b to
07a7b68
Compare
Handle other Slice statuses.
…n with TopologyAssignment.
mbobrovskyi
force-pushed
the
slice/admission-check
branch
from
07a7b68 to
abf65b0
Compare
| return ctrl.Result{}, err | ||
| } | ||
| if err != nil { | ||
| return ctrl.Result{}, r.createSlice(ctx, wl, ac) |
There was a problem hiding this comment.
why do we create slice in this branch?
There was a problem hiding this comment.
We don’t have statuses on the Slice yet, so we need to stop reconciliation for now. Once we receive the status, we can restart reconciliation and handle it then.
There was a problem hiding this comment.
I think a package like this is a good idea. Nice :)
|
lgtm, thanks! |
Fixes / Features