Skip to content
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

feat(controller): support volumeClaimTemplates can be referenced by templateRef. Fixes #13977 #7444 #14056

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chengjoey
Copy link
Member

@chengjoey chengjoey commented Jan 7, 2025

Fixes #13977 #7444

Motivation

In order for volumeClaimTemplates to be referenced by the templateRef

The purpose is to automatically bring the corresponding volumeClaimTemplates into the current workflow when using templateRef.

Modifications

  1. add volumeClaimTemplates field in template
  2. workflow operator create the pvcs referred by template

Verification

e2e test

@chengjoey chengjoey marked this pull request as draft January 7, 2025 12:07
@chengjoey chengjoey changed the title fix(controller): support volumeClaimTemplates can be referenced by templateRef feat(controller): support volumeClaimTemplates can be referenced by templateRef Jan 7, 2025
@chengjoey chengjoey added type/feature Feature request solution/workaround There's a workaround, might not be great, but exists labels Jan 7, 2025
@chengjoey chengjoey marked this pull request as ready for review January 9, 2025 07:28
@shuangkun
Copy link
Member

looks good. please resolve conflicts!

@shuangkun shuangkun added the area/controller Controller issues, panics label Jan 18, 2025
@chengjoey chengjoey changed the title feat(controller): support volumeClaimTemplates can be referenced by templateRef feat(controller): support volumeClaimTemplates can be referenced by templateRef. Fixes #13977 #7444 Jan 20, 2025
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any limit on how many times createPVCs() will be called, it looks like it will be called repeatedly.

}
}
if len(vols) > 0 {
woc.execWf.Spec.VolumeClaimTemplates = append(woc.execWf.Spec.VolumeClaimTemplates, vols...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execWf.Spec should be considered immutable (constant), this isn't good practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I might be able to put this part of the logic inside setExecWorkflow(ctx context.Context), and reserve all the PVCs when initializing execWf.

@chengjoey
Copy link
Member Author

Is there any limit on how many times createPVCs() will be called, it looks like it will be called repeatedly.

createPVCsis an idempotent function,len(woc.execWf.Spec.VolumeClaimTemplates) == len(woc.wf.Status.PersistentVolumeClaims) ensured that there will be no duplicate creation

func (woc *wfOperationCtx) createPVCs(ctx context.Context) error {
if !(woc.wf.Status.Phase == wfv1.WorkflowPending || woc.wf.Status.Phase == wfv1.WorkflowRunning) {
// Only attempt to create PVCs if workflow is in Pending or Running state
// (e.g. passed validation, or didn't already complete)
return nil
}
if len(woc.execWf.Spec.VolumeClaimTemplates) == len(woc.wf.Status.PersistentVolumeClaims) {
// If we have already created the PVCs, then there is nothing to do.
// This will also handle the case where workflow has no volumeClaimTemplates.
return nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics solution/workaround There's a workaround, might not be great, but exists type/feature Feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

volumeClaimTemplates in WorkflowTemplate works only with workflowTemplateRef and not templateRef
3 participants