Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17566 )
Change subject: [k8s] Implement an operator for a KuduCluster CRD ...................................................................... Patch Set 1: (27 comments) Thanks for the contribution! Most of my questions are just to get more situated with the structure of this patch. http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/Makefile File kubernetes/kudu-operator/Makefile: PS1: nit: could you add a license header to this file? Same for other files that don't have one. http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/Makefile@6 PS1, Line 6: 0.0.7 Is this supposed to be the Kudu version? Or a different project? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/Makefile@90 PS1, Line 90: nit: I know gofmt converts things to tabs, but does it also run on makefiles? If not, could we stick with the more prominent makefile styling? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/crd/bases/kuduoperator.capstone_kuduclusters.yaml File kubernetes/kudu-operator/config/crd/bases/kuduoperator.capstone_kuduclusters.yaml: PS1: I expected somewhere in this patch there to be a place where we define what it means to run a Kudu master and Kudu tserver. Did I miss it, or is that all taken care of by our Docker setup? I would've guessed it'd be in this file. http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/crd/bases/kuduoperator.capstone_kuduclusters.yaml@11 PS1, Line 11: capstone nit: maybe name this something more generic, like "cluster" or something? Same in PROJECT http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml File kubernetes/kudu-operator/config/default/kustomization.yaml: http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml@6 PS1, Line 6: alices nit: kudu-operator-wordpress? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml@19 PS1, Line 19: # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in : # crd/kustomization.yaml : #- ../webhook Seems like this isn't in this patch. Can we scrub away all the "webhook" bits, or are they useful to keep in despite the missing directory? I guess the same goes for certmanager and prometheus. I see prometheus in this patch, but is it used if this is commented out? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml@32 PS1, Line 32: : # Mount the controller config file for loading manager configurations : # through a ComponentConfig type : #- manager_config_patch.yaml When should users uncomment this line? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml@37 PS1, Line 37: # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in : # crd/kustomization.yaml : #- manager_webhook_patch.yaml : : # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. : # Uncomment 'CERTMANAGER' sections in crd/kustomization.yaml to enable the CA injection in the admission webhooks. : # 'CERTMANAGER' needs to be enabled to use ca injection : #- webhookcainjection_patch.yaml Same here? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/default/kustomization.yaml@45 PS1, Line 45: : # the following config is for teaching kustomize how to do var substitution : vars: : # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix. : #- name: CERTIFICATE_NAMESPACE # namespace of the certificate CR : # objref: : # kind: Certificate : # group: cert-manager.io : # version: v1 : # name: serving-cert # this name should match the one in certificate.yaml : # fieldref: : # fieldpath: metadata.namespace : #- name: CERTIFICATE_NAME : # objref: : # kind: Certificate : # group: cert-manager.io : # version: v1 : # name: serving-cert # this name should match the one in certificate.yaml : #- name: SERVICE_NAMESPACE # namespace of the service : # objref: : # kind: Service : # version: v1 : # name: webhook-service : # fieldref: : # fieldpath: metadata.namespace : #- name: SERVICE_NAME : # objref: : # kind: Service : # version: v1 : # name: webhook-service Is the expectation that users who want to customize their deployments can just uncomment some of these? Does that work as is? If not, maybe remove them? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/manager/kustomization.yaml File kubernetes/kudu-operator/config/manager/kustomization.yaml: http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/manager/kustomization.yaml@15 PS1, Line 15: hannahvnguyen nit: maybe call this apache/kudu or kudu or apache-kudu? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/config/rbac/kustomization.yaml File kubernetes/kudu-operator/config/rbac/kustomization.yaml: PS1: It'd be nice if you could describe either here, in the README, or in the commit message what these custom configs grant and to whom. IMO it isn't too straight-forward the relationship between all the verbs in leader_election_role and leader elections in Kudu. It also be nice if you could give a top level description on how these different directories work together (maybe in the README?), or at least what each directory does. Some are somewhat straightforward, like /crd, but it's unclear how /rbac fits in anywhere, or what /scorecard is. http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go File kubernetes/kudu-operator/controllers/kuducluster_controller.go: http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@47 PS1, Line 47: kuduv1 "github.com/apache/kudu/tree/master/kubernetes/kudu-operator/api/v1" To disambiguate Kudu version and Kudu Operator version, maybe call this kuduoperatorv1? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@67 PS1, Line 67: //+kubebuilder:rbac:groups=kuduoperator.capstone,resources=kuduclusters,verbs=get;list;watch;create;update;patch;delete : //+kubebuilder:rbac:groups=kuduoperator.capstone,resources=kuduclusters/status,verbs=get;update;patch : //+kubebuilder:rbac:groups=kuduoperator.capstone,resources=kuduclusters/finalizers,verbs=update : //+kubebuilder:rbac:groups=core,resources=pods,verbs=get : //+kubebuilder:rbac:groups=core,resources=pods/exec,verbs=create : //+kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;create;update;patch;delete : //+kubebuilder:rbac:groups=apps,resources=statefulsets,verbs=get;list;watch;create;update;patch;delete Are these actually comments? Or annotations of some sort? Can they be removed? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@74 PS1, Line 74: Reconcile nit: could you describe at a high level what reconciliation means in this context, and how that is achieved, and when? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@102 PS1, Line 102: nil Should we return tservers_err? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@112 PS1, Line 112: // Reconciles the state of the Kudu masters. nit: maybe also mention the less obvious output param? i.e. "If ..., returns the time it takes to ... so callers can .." http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@154 PS1, Line 154: kmasters nit: k_masters maybe? To keep the naming convention uniform? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@172 PS1, Line 172: // Check if the statefulset for the masters is the correct size. If not, delete the current one and requeue. : // Recreating all the masters together avoids the issue of master consensus conflict. Just noting that this seems super unsafe since we would then lose access to our tables! The 1.15 should make this bit easier, as would this patch https://gerrit.cloudera.org/c/17528/ Maybe add a TODO to make this more safe by adding a master? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@206 PS1, Line 206: masters nit: tservers http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@229 PS1, Line 229: // Check if the statefulset for the tservers is the correct size. If not, update the current one. : // Since the number of tservers has changed, launch the rebalancer, and requeue. : tserver_replicas := kuducluster.Spec.NumTservers : if *ktservers.Spec.Replicas != tserver_replicas { : ktservers.Spec.Replicas = &tserver_replicas Does this only take effect when users patch the desired number of tablet servers? Does it get run if one of the processes dies? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@378 PS1, Line 378: stateset nit: statefulset? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@454 PS1, Line 454: stateset nit: statefulset? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@504 PS1, Line 504: namesForKuduMasters nit: maybe call this addressesForKuduMasters() or getMasterAddresses() or somesuch? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@520 PS1, Line 520: addresses nit: maybe name this masterAddresses? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/hack/boilerplate.go.txt File kubernetes/kudu-operator/hack/boilerplate.go.txt: PS1: What's this for? http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/main.go File kubernetes/kudu-operator/main.go: http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/main.go@92 PS1, Line 92: if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { : setupLog.Error(err, "unable to set up health check") : os.Exit(1) : } nit: does this just ping some endpoint to check for health? Maybe add a TODO saying this is where we should add ksck (assuming that's correct) -- To view, visit http://gerrit.cloudera.org:8080/17566 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c855f3bab7bbd501f6237a23781f7ae200ba3db Gerrit-Change-Number: 17566 Gerrit-PatchSet: 1 Gerrit-Owner: Hannah Nguyen <hannahvnguye...@gmail.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 16 Jun 2021 21:11:09 +0000 Gerrit-HasComments: Yes
