Vck initializer #61
Vck initializer #61
Conversation
There was a problem hiding this comment.
I have a few questions and changes I posted below. Apart from them I would suggest changing the directory structure.
Instead of having a different package for the initializer, WDYT about this:
- The
main.gobe part of a package inpkg. - Re-use the helm charts and keep the yaml's there and control it's installation with a flag.
- Re-use the docker folder to keep your Dockerfile
- Re-use the makefile to generate the dockerfile as well as the go binary.
That way you can also use some of the interfaces like hooks and also have this be part of the circleci build process. WDYT?
| "name": "vck-example3", | ||
| "id": "vol2", | ||
| "containers": ["helloworld-1"], | ||
| "mount-path": "/var/datasets" |
There was a problem hiding this comment.
why not have a different mount path for each container? So for example it can be:
"mount": {"helloworld-1": "/var/datasets"}
There was a problem hiding this comment.
how about this
"initializer.kubernetes.io/vck": '{
"name": "vck-example",
"id": "vol2",
"containers": [
{
name: "helloworld-1",
mount: "/var/datasets"
}],
}'
| spec: | ||
| containers: | ||
| - name: vck-initializer | ||
| image: gcr.io/constant-cubist-173123/vck-initializer:0.0.1 |
There was a problem hiding this comment.
We have all of our images in the volumecontroller docker hub for now, do you want to maybe use the same one?
vck-initializer/docs/cleanup.md
Outdated
| ``` | ||
|
|
||
| ``` | ||
| kubectl delete deployment vck-initializer helloworld helloworld-with-annotation |
There was a problem hiding this comment.
I think this should go in the with the examples documentation.
| @@ -0,0 +1,25 @@ | |||
| # Deploy The VCK Initializer | |||
|
|
|||
| The VCK Initializer is a [Kubernetes Initializer](https://kubernetes.io/docs/admin/extensible-admission-controllers/#what-are-initializers) that injects an [Envoy](https://lyft.github.io/envoy) proxy into Deployments based on containers and volumes defined in a [ConfigMap](https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap). | |||
There was a problem hiding this comment.
Does it still use Envoy?
There was a problem hiding this comment.
Nope it doesn't, old comments need to remove
|
|
||
| ## Deploy the VCK Initializer | ||
|
|
||
| Deploy the VCK Initializer with the `-require-annotation` flag set. This will ensure the volume manager data is only injected into Deployments with an `initializer.kubernetes.io/vck` annotation set to a non-empty value as given below. |
There was a problem hiding this comment.
Why do we need that flag? What's the use of the initializer without -require-annotation?
There was a problem hiding this comment.
it was 2 step process I merged it, forgot to remove the documentation
| if deployment.ObjectMeta.GetInitializers() != nil { | ||
| pendingInitializers := deployment.ObjectMeta.GetInitializers().Pending | ||
|
|
||
| if initializerName == pendingInitializers[0].Name { |
There was a problem hiding this comment.
Is it always the case that the first pending initializer is always going to be this initializer?
There was a problem hiding this comment.
yes, I think the controller waits till it reaches first in the queue
| if len(pendingInitializers) == 1 { | ||
| initializedDeployment.ObjectMeta.Initializers = nil | ||
| } else { | ||
| initializedDeployment.ObjectMeta.Initializers.Pending = append(pendingInitializers[:0], pendingInitializers[1:]...) |
There was a problem hiding this comment.
Is it always the case that the first pending initializer is always going to be this initializer?
There was a problem hiding this comment.
we need to wait till other initialisers are processed, correct me if I am wrong, isn't that the purpose of the queue.
| return nil | ||
| } | ||
|
|
||
| func addVolumesAffinity(vckVM *vckv1.VolumeManager, info *data) (*corev1.Volume, *corev1.Affinity, error) { |
There was a problem hiding this comment.
Do you want to maybe rename this to getVolumesAffinity?
There was a problem hiding this comment.
won't get just get the information but we are adding the object to the deployment?
not sure still.
|
|
||
| } | ||
|
|
||
| func addVolumeMount(deployment *v1beta1.Deployment, vckVM *vckv1.VolumeManager, container string, mountPath string) (*corev1.VolumeMount, int, error) { |
There was a problem hiding this comment.
I think vckVM is not used here.
There was a problem hiding this comment.
yea I changed the implementation
| containerID = id | ||
| volumeMount := corev1.VolumeMount{ | ||
| MountPath: mountPath, | ||
| Name: "dataset-claim", |
There was a problem hiding this comment.
You can pass in the volumeVCK and extract name from that instead of explicitly specifying it here.
docs/user.md
Outdated
| @@ -169,6 +169,32 @@ $ kubectl create -f resources/deployments/vck-deployment.yaml | |||
| deployment "vck-example-deployment" created | |||
| ``` | |||
|
|
|||
| ## Create a Deployment using the VCK initializer | |||
There was a problem hiding this comment.
Can you add this topic at the top too?
There was a problem hiding this comment.
I dint get this, do you want me to push this topic to the top?
There was a problem hiding this comment.
| | container.name | yes | Name of the container to append the VCK volume | | | ||
| | container.mount-path | no | Path for the VCK to mount the volume | /var/dataset | | ||
|
|
||
| The id key is optional it picks the first volume by default, similarly the container object is optional it picks all containers by default and appends it to default mount path "/var/datasets". If the container object just contains the name,vck is appended to default mount path "/var/datasets". |
There was a problem hiding this comment.
Can you add an example here which would cover the cases described?
There was a problem hiding this comment.
so we have an examples folder where we could add this? I would rather have an example file than paste the yams here.
There was a problem hiding this comment.
Hmm, I meant an example like we have for the source types at the bottom: https://github.com/IntelAI/vck/pull/61/files/69b16c782919477bbc63340f56f2e9e51e89b358#diff-2872b51e38bcd053c0e88c4fe5c28fd2L215. Just like snippets of the important parts.
| AddFunc: func(obj interface{}) { | ||
| o := obj.(*v1beta1.Deployment) | ||
| err := initializeDeployment(o, i) | ||
| if err != nil { |
There was a problem hiding this comment.
Does this mean that we delete the deployment if the vck initializer is not specified in the right way?
There was a problem hiding this comment.
yes I am deleting the deployment, as I told you if I do not delete the deployment it gets stuck in the limbo state and not allowing user to create a new deployment with same name.
pkg/initializer/initializer.go
Outdated
| var ( | ||
| annotation string | ||
| initializerName string | ||
| requireAnnotation bool |
There was a problem hiding this comment.
Do we still need this and the defaultRequireAnnotation?. Where is requireAnnotation set?
There was a problem hiding this comment.
ohh yea I deleted some necessary code, ill correct that
pkg/initializer/initializer.go
Outdated
| } else { | ||
| initializedDeployment.ObjectMeta.Initializers.Pending = append(pendingInitializers[:0], pendingInitializers[1:]...) | ||
| } | ||
| if requireAnnotation { |
pkg/initializer/initializer.go
Outdated
|
|
||
| func getVolumesAffinity(vckVM *vckv1.VolumeManager, info *data) (*corev1.Volume, *corev1.Affinity, error) { | ||
| if len(info.ID) == 0 { | ||
| item := vckVM.Status.Volumes[0] |
There was a problem hiding this comment.
I think you should error check before doing this, i.e. check if the VCK CR has a status and that it is Running and here check if the Volumes[0] exists.
There was a problem hiding this comment.
yes I agree, it skipped my mind
Adding code for vck web-hook Issue #15