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

store snapshot of config files on create #824

Merged
merged 2 commits into from Nov 4, 2021

Conversation

tonistiigi
Copy link
Member

Files can be reused when container needs to be booted again.

The second commit enables config files for k8s. @morlay PTAL. If something is wrong we can remove the second commit or you can push a fix to this PR.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Files can be reused when container needs to be booted again.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi added this to the v0.7.0 milestone Nov 4, 2021
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@morlay
Copy link
Collaborator

morlay commented Nov 4, 2021

@tonistiigi I prefer use single one ConfigMap

if we could multi ConfigMap.
When next deployment with less configurations will not delete some ConfigMap
https://github.com/docker/buildx/blob/master/driver/kubernetes/driver.go#L168-L182

Example

# first release
/etc/buildkit
/etc/buildkit/certs   

# second release
# the dir list is from new config content, once certs not includes, /etc/buildkit/certs will not includes.
/etc/buildkit
# ConfigMap for /etc/buildkit/certs will be remain in k8s

If still keep multiple ConfigMaps, need to add label in ConfigMaps, and List by label before delete

For single one ConfigMap. could be:

configFiles := []struct {
	name    string
	absPath string
}{}

d.Spec.Template.Spec.Containers[0].VolumeMounts = make([]corev1.VolumeMount, len(configFiles))
d.Spec.Template.Spec.Volumes = make([]corev1.Volume, len(configFiles))

for i, cf := range configFiles {
	d.Spec.Template.Spec.Containers[0].VolumeMounts[i] = corev1.VolumeMount{
		Name:      "config-" + cf.name,
		MountPath: cf.name,
		SubPath:   filepath.Base(cf.absPath),
	}

	d.Spec.Template.Spec.Volumes[i] = corev1.Volume{
		Name: "config-" + cf.name,
		VolumeSource: corev1.VolumeSource{
			ConfigMap: &corev1.ConfigMapVolumeSource{
				LocalObjectReference: corev1.LocalObjectReference{
					Name: "config", // from same ConfigMap
				},
				Items: []corev1.KeyToPath{
					{
						Key: cf.name,
						Path: filepath.Base(cf.absPath),
					},
				},
			},
		}}
}

Which one do you prefer ?

@tonistiigi
Copy link
Member Author

I'd like to keep the file structure the same as container driver(+rest in future) and reuse the same code.

I didn't quite get in what case the second release with config and no certs can happen. Also not sure how they remain mounted if they are removed from container spec. On deletion cleanup by id should still work. I added incrementing numbers to names.

@tonistiigi
Copy link
Member Author

If you think deletion by a label is safer feel free to add commit for that.

@morlay
Copy link
Collaborator

morlay commented Nov 4, 2021

@tonistiigi
Sure. This PR could be ok to merge.
I may add the deletion by a label later today if I have time.

@crazy-max
Copy link
Member

crazy-max commented Nov 4, 2021

@tonistiigi Could the spec be a map where we also define the base dir. So instead of:

{
  "Nodes": [
    {
      "Files": {
        "buildkitd.toml": "...",
        "certs/docker.io/cert.pem": "...",
        "certs/docker.io/key.pem": "...",
        "certs/docker.io/myca.pem": "..."
      }
    }
  ],
}

we have:

{
  "Nodes": [
    {
      "Files": [
        {
          "/etc/buildkit": {
            "buildkitd.toml": "...",
            "certs/docker.io/cert.pem": "...",
            "certs/docker.io/key.pem": "...",
            "certs/docker.io/myca.pem": "..."
          }
        }
      ]
    }
  ]
}

to be able to handle other use cases in the future outside buildkitd config?

@tonistiigi
Copy link
Member Author

@crazy-max It could be though that we want to change the etc dir in the future. Eg. Rootless could be one case

@tonistiigi tonistiigi merged commit d311561 into docker:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants