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

worker: remove ContainerProvider #3546

Merged
merged 2 commits into from May 21, 2019
Merged

worker: remove ContainerProvider #3546

merged 2 commits into from May 21, 2019

Conversation

ddadlani
Copy link
Contributor

@ddadlani ddadlani commented Mar 19, 2019

combine containerProvider and worker
add workerHelper which stores helper functions

Signed-off-by: Divya Dadlani ddadlani@pivotal.io
Co-authored-by: Krishna Mannem kmannem@pivotal.io

fixes #3052

@vito
Copy link
Member

vito commented Mar 19, 2019

😮

@ddadlani
Copy link
Contributor Author

ddadlani commented Mar 19, 2019

Preliminary thoughts?

One of the things we are not happy with is the length of createVolumes

func (worker *gardenWorker) createVolumes(logger lager.Logger, isPrivileged bool, creatingContainer db.CreatingContainer, spec ContainerSpec) ([]VolumeMount, error) {
var volumeMounts []VolumeMount
var ioVolumeMounts []VolumeMount
scratchVolume, err := worker.volumeClient.FindOrCreateVolumeForContainer(
logger,
VolumeSpec{
Strategy: baggageclaim.EmptyStrategy{},
Privileged: isPrivileged,
},
creatingContainer,
spec.TeamID,
"/scratch",
)
if err != nil {
return nil, err
}
scratchMount := VolumeMount{
Volume: scratchVolume,
MountPath: "/scratch",
}
volumeMounts = append(volumeMounts, scratchMount)
hasSpecDirInInputs := anyMountTo(spec.Dir, getDestinationPathsFromInputs(spec.Inputs))
hasSpecDirInOutputs := anyMountTo(spec.Dir, getDestinationPathsFromOutputs(spec.Outputs))
if spec.Dir != "" && !hasSpecDirInOutputs && !hasSpecDirInInputs {
workdirVolume, volumeErr := worker.volumeClient.FindOrCreateVolumeForContainer(
logger,
VolumeSpec{
Strategy: baggageclaim.EmptyStrategy{},
Privileged: isPrivileged,
},
creatingContainer,
spec.TeamID,
spec.Dir,
)
if volumeErr != nil {
return nil, volumeErr
}
volumeMounts = append(volumeMounts, VolumeMount{
Volume: workdirVolume,
MountPath: spec.Dir,
})
}
inputDestinationPaths := make(map[string]bool)
for _, inputSource := range spec.Inputs {
var inputVolume Volume
localVolume, found, err := inputSource.Source().VolumeOn(logger, worker)
if err != nil {
return nil, err
}
cleanedInputPath := filepath.Clean(inputSource.DestinationPath())
if found {
inputVolume, err = worker.volumeClient.FindOrCreateCOWVolumeForContainer(
logger,
VolumeSpec{
Strategy: localVolume.COWStrategy(),
Privileged: isPrivileged,
},
creatingContainer,
localVolume,
spec.TeamID,
cleanedInputPath,
)
if err != nil {
return nil, err
}
} else {
inputVolume, err = worker.volumeClient.FindOrCreateVolumeForContainer(
logger,
VolumeSpec{
Strategy: baggageclaim.EmptyStrategy{},
Privileged: isPrivileged,
},
creatingContainer,
spec.TeamID,
cleanedInputPath,
)
if err != nil {
return nil, err
}
destData := lager.Data{
"dest-volume": inputVolume.Handle(),
"dest-worker": inputVolume.WorkerName(),
}
err = inputSource.Source().StreamTo(logger.Session("stream-to", destData), inputVolume)
if err != nil {
return nil, err
}
}
ioVolumeMounts = append(ioVolumeMounts, VolumeMount{
Volume: inputVolume,
MountPath: cleanedInputPath,
})
inputDestinationPaths[cleanedInputPath] = true
}
for _, outputPath := range spec.Outputs {
cleanedOutputPath := filepath.Clean(outputPath)
// reuse volume if output path is the same as input
if inputDestinationPaths[cleanedOutputPath] {
continue
}
outVolume, volumeErr := worker.volumeClient.FindOrCreateVolumeForContainer(
logger,
VolumeSpec{
Strategy: baggageclaim.EmptyStrategy{},
Privileged: isPrivileged,
},
creatingContainer,
spec.TeamID,
cleanedOutputPath,
)
if volumeErr != nil {
return nil, volumeErr
}
ioVolumeMounts = append(ioVolumeMounts, VolumeMount{
Volume: outVolume,
MountPath: cleanedOutputPath,
})
}
sort.Sort(byMountPath(ioVolumeMounts))
volumeMounts = append(volumeMounts, ioVolumeMounts...)
return volumeMounts, nil
}
and are looking to cut that down.

The main advantage that this refactor gives us is the isolation of findOrInitializeContainer

creatingContainer, createdContainer, containerHandle, err = worker.helper.findOrInitializeContainer(logger, owner, metadata)
which allows us to lock CreatingContainer creation for #3301

The second advantage is that it attempts to decouple volume and container creation in preparation for concourse/rfcs#20

Divya Dadlani and others added 2 commits May 16, 2019 10:52
combine containerProvider and worker
add workerHelper which stores helper functions

Signed-off-by: Divya Dadlani <ddadlani@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>

#3052
Signed-off-by: Divya Dadlani <ddadlani@pivotal.io>
Co-authored-by: Krishna Mannem <kmannem@pivotal.io>
@kcmannem
Copy link
Member

Our other streams of work are based on this branch. We wanna integrate it into master. We pulled apart as much as we could without artifacts gc (volumes not dependent on containers). Another reason is to remove a the CreatingContainerLock which is unnecessary.

@kcmannem kcmannem requested a review from vito May 16, 2019 15:03
@ddadlani ddadlani marked this pull request as ready for review May 16, 2019 21:07
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

tbh not much to review here since it's primarily slingin' code around, but it passes testflight/watsjs, so let's see how it fares in prod. 💥

@vito vito merged commit 9ff6b99 into master May 21, 2019
@vito vito deleted the refactor/3052 branch May 21, 2019 15:08
@jamieklassen jamieklassen added this to the v5.3.0 milestone Jun 10, 2019
@jamieklassen jamieklassen added the release/undocumented This didn't warrant being documented or put in release notes. label Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/undocumented This didn't warrant being documented or put in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple container and volume creation in FindOrCreateContainer
4 participants