-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Exceeding quota with volumeClaimTemplates #3490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please set-up time in my diary to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix e2e test.
@@ -1673,7 +1680,7 @@ func (woc *wfOperationCtx) hasDaemonNodes() bool { | |||
} | |||
|
|||
func (woc *wfOperationCtx) markWorkflowRunning() { | |||
woc.markWorkflowPhase(wfv1.NodeRunning, false) | |||
woc.markWorkflowPhase(wfv1.NodeRunning, false, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous error/pending message needs to clear on running phase
@@ -4101,7 +4101,7 @@ status: | |||
defer cancel() | |||
woc := newWorkflowOperationCtx(wf, controller) | |||
woc.operate() | |||
assert.Equal(t, wfv1.NodePending, woc.wf.Status.Phase) | |||
assert.Equal(t, wfv1.NodeRunning, woc.wf.Status.Phase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resubmit will memoization wf phase should be running after operate
function. I will create separate PR to fix the phase in memorization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is another bug fix - can we combine into a single valid PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I will fix the memoization bug also in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @jessesuen discussed, just need to fix the phase in resubmit memorized scenario.
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
. Fixes Exceeding quota with volumeClaimTemplates #3419