-
Notifications
You must be signed in to change notification settings - Fork 40
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
CSI Node daemonset #14
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.
This also requires the SCC, unit tests, and the rbacs for the node daemonset reconciliation.
Which SCC's? |
The node pods are privileged and hence require SCCs in order to access the host resources. We will need to create them. |
hostPathDirectory := corev1.HostPathDirectory | ||
hostPathDirectoryOrCreateType := corev1.HostPathDirectoryOrCreate | ||
storageMedium := corev1.StorageMediumMemory | ||
|
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.
Looks like a lot of static code when generating the daemonset spec. I've a suggestion to use to yaml files and read them directly using go embed? This will reduce the lines of codes and also make it easier to understand the spec. An example of this approach can be seen here
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.
I would prefer to keep this in the code for now. We can revisit this later.
config/rbac/sccs.yaml
Outdated
subjects: | ||
- kind: ServiceAccount | ||
name: topolvm-node | ||
roleRef: |
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.
de-tab this, currently I think they are indented to be part of metadata
23e8c5b
to
85d9d73
Compare
Unit test passing, and I have tested also in an openshift cluster, now the daemonset is created and deleted when a lvmcluster CRD is created/deleted, but I have an error in the privileges assignation.... I think I miss something in the SCC ...
|
Almost there .... Now I am using the openshift "privileged" scc for "topolvm-nanager", because it seems that with the custom scc I created ("topolvm_node_scc.yaml") we do not have all the rights needed. I have "extracted" the scc from the "psps" for node plugin in the topolvm project, and after several tests i have added more rights, but it continues "blocking" the deployment of the node daemonset because rights. with an image generated with the code in this branch, (using the "privileged" scc), I deployed in an openshift cluster, with excellent results., The node daemonsed is created, and the pods are deployed in all the nodes, waiting for the lvmd config file. Creating manually a VG in one of the nodes, and creating the config file "lvmd.yaml" , all the containers in the pod start perfectly.
|
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 rework the commits as well - there is one for a wip and rebase.
|
||
func (n topolvmNode) ensureCreated(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error { | ||
nodeDaemonSet := getNodeDaemonSet(lvmCluster) | ||
result, err := cutil.CreateOrUpdate(ctx, r.Client, nodeDaemonSet, func() error { return nil }) |
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.
This will need a mutatefn
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.
True. If you don't mind I will address this when we have merged the first version.
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
@leelavg: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
nit
Topolvm csi node is a daemonset deploying lvmd, topolvm node csi, csiregistrar and a liveness probe container. Uses an init container for wait deployment until lvmd config file is available Signed-off-by: Juan Miguel Olmo Martínez <[email protected]>
Signed-off-by: Juan Miguel Olmo Martínez <[email protected]>
Verify node daemonset creation and deletion using the controller unit test Signed-off-by: Juan Miguel Olmo Martínez <[email protected]>
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.
Approving on the understanding as discussed that the mutatefn and scc will be implemented shortly.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jmolmo, nbalacha, sp98 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I need to go through unit tests but I think it is good to have a first version of the node daemonset to discuss changes and improvements.
I have lot of doubts about to use a configmap for storing the lvmd configuration. It can work for SNO deployments, but we are going to have problems with more that 1 node clusters, because the configmap cannot be different for each node using a Daemonset.
The only way I see to do that is to use a init container to read a configmap (one per node) and generate a file to be used when lvmd starts. Thoughts?
Update management:
I only can see two changes to manage:
Signed-off-by: Juan Miguel Olmo Martínez [email protected]