Hannah Nguyen 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) 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 Done 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? This should be the version of the operator; I was just incrementing as I developed/tested. Is there a convention for versioning? 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 makefile I thought tabs were necessary for Makefile recipes? https://www.gnu.org/software/make/manual/make.html 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 I believe that should be taken care of by the Docker setup. 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? S Done 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? Done 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" bi Scrubbing the webhook and certmanager stuff. The prometheus monitor is currently unused when it's commented out; should i just remove that subdirectory entirely? 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? If you want to modify the configs for the Deployment that runs the operator, you can modify the manager_config_patch.yaml file and uncomment this line: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.7.0/#add-the-manager-config-patch-to-configdefaultkustomizationyaml 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? Done 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 j Haven't tested this...going to delete it 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? Done 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 co Added descriptions to the different yaml files in rbac/ and the kustomization.yaml files for the other directories. 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 kud Done 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 remov These annotations are for controller-gen to generate the manifests for a CRD (in this case, a kuducluster). https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html 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 c Done 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? Done 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 ..., return Done 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? Done 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 Done http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@206 PS1, Line 206: masters > nit: tservers Done 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 se If a tserver container dies, because it's part of a statefulset, it will be automatically restarted. This code segment only matters if users patch the spec and change the number of tservers. http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@378 PS1, Line 378: stateset > nit: statefulset? Done http://gerrit.cloudera.org:8080/#/c/17566/1/kubernetes/kudu-operator/controllers/kuducluster_controller.go@454 PS1, Line 454: stateset > nit: statefulset? Done 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 s Done 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? Done 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? Whatever is in this file is pre-pended to any generated code: https://book-v1.book.kubebuilder.io/beyond_basics/boilerplate.html 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 TOD This pings the k8s API server to see if it's healthy, not the KuduCluster. Any logic that invokes ksck should be done in the controller--which is responsible for the KuduCluster--not this main Go program that is responsible for running the manager. The manager owns the controller (see `SetupWithManager()` in `kuducluster_controller.go`) https://kubernetes.io/docs/reference/using-api/health-checks/#api-endpoints-for-health -- 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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Jun 2021 23:06:29 +0000 Gerrit-HasComments: Yes
