#442  Helm / K8s Deployment improvements
Open
max opened 2 weeks ago

I thought I'd lend some ideas for improvement for the Helm / Kubernetes install:

  1. Better Documentation
    • The Docs code.onedev.io/projects/162/blob/main/pages/deploy-into-k8s.md#installation-using-helm need to go in a bit more detail. Ideally they should link to Readme that outlines not only the repo & chart, but also all the values available in values.yaml along with their descriptions.
    • helm-docs is a great tool for this purpose.
    • May want to include a blurb around the pitfalls of running SSH; not ideal behind a LoadBalancer, nodePort conflicts, and generally ClusterIP isn't accessible outside of the cluster.
    • HTTP/S would be ideal for given the above and it should be strongly encouraged to use TLS.
  2. Helm Chart should be versioned separately from OneDev
    • The OneDev server and Helm Chart are not co-dependent upon each and therefor makes sense that they'd have separate release trains
  3. Add a CHANGELOG for the Helm Chart
  4. ClusterRole is too wide
    • These are way to wide in scope and it would be unsafe to run with this much access
    • These should be scoped to the same NS OneDev is deployed into OR allow the user to specify a secondary NS
    • Shouldn't need patch nodes
    • Not sure I understand the need for the clusterrolebindings rule. The Service Account for agent builds should be predetermined with a predetermined role

Unfortunately #4 is a showstopper for me. However, the project looks awesome and if I find some time I will try to address the above via a PR.

Robin Shen commented 2 weeks ago

Thanks for the feedback. The helm chart implementation is contributed by other user, and I am working on top of the contribution to make it production ready, with improvements both in manual and chart. I will check your suggestions while improving it.

As to clusterrole permission:

  1. The namespace resource: this is desired to run each CI build in separate namespaces, identified by build number and some prefixes
  2. The clusterrolebindings resource: it is impractical to predefine desired permissions of CI jobs (running as pods). Some CI jobs just compile/test the code, and other CI jobs might need to deploy built images into the cluster, this is the reason why the kubernetes executor allows to defined a cluster role, and OneDev binds cluster role to service account of the CI job as part of creating the job pod
  3. The node patch permission: OneDev allows to cache certain parts of the job pod file, for instance to cache downloaded dependencies. This cache resides on the node running the job. If second run of job happens on a different node, the cache will not be available. To increase cache hit rate, OneDev adds some customer labels including cache info to nodes to utilize the node affinity feature to schedule jobs to used nodes if possible
  4. Other permissions not listed here: desired for running/monitoring CI jobs on a different namespace
Robin Shen commented 2 weeks ago

Also the patch node permission can be removed. If removed, do not forget to disable the "create cache labels" in Kubernetes executor.

max commented 2 weeks ago

#1:
This isn't a good practice. The Namespace(s) should be user-defined. For tracking the ci pods, you'd simply add a unique id suffix to the name. This is how other executors like Jenkins or Airflow handle it. It is an anti-pattern to dynamically create namespaces for ephemeral pods.

#2:
CI jobs should not be touching K8s resources or have access to them by default. This should be provided as credential that is maintained by the user. The app (or any app for that matter) shouldnt be allowed arbitrarily create them.

#3:
Nothing should be cached on the nodes. If Statefullness is needed, a PVC should be used. You risk filling up the node disk and you punish the os/kubelet disk with IOPS. Kubelet nodes, especially those running etcd, are sensitive to disk latency. The practice of writing to a host should be avoided. Most enterprises disable emptyDir for example.

#4:
See #1

Robin Shen commented 2 weeks ago

#1: Resources with prefix/suffix can get difficult to cleanup if these resources are left over for some reason. Also the namespace per CI approach can provide an isolated environment for each CI job. For instance, multiple instances of same CI job might be running, and they can access defined service name without interfering with each other.

#2: Can you elaborate? In case a CI job wants to deploy some resources into some namespaces, how to give the job pod deploy permissions with a credential? Even the namespace to deploy in the job can be dynamic, for instance, the job might want to deploy into namespace identified by git branch, etc.

#3: I thought about mounting pvc to store cache, but that is problematic for concurrent jobs, as a single pvc is generally not possible to be mounted to multiple nodes for read/write, not to mention the configuration overhead. I also thought about uploading/downloading cache to particular slot of some file system service, but that is much slower, and can even be slower than the case without cache if you have a fast network. I guess there is no perfect approach for caching.

Robin Shen commented 2 weeks ago

For #2, also want to mention that only authorized jobs can use a particular Kubernetes executor. So one can only allow trusted jobs to run with a certain Kubernetes executor with extra cluster roles.

max commented 2 weeks ago

#1: Resources with prefix/suffix can get difficult to cleanup if these resources are left over for some reason.

Any resource (including a namespace) could get left over. The K8s API provides a convenient way of monitoring these resources with a Watcher(). The Suffix/UID would be tracked somewhere; probably the same place you track Namespaces.

Also the namespace per CI approach can provide an isolated environment for each CI job. For instance, multiple instances of same CI job might be running, and they can access defined service name without interfering each other.

A Namespace in itself doesn't isolate resources. It's just a way of logically grouping resources together. Further, if a CI Job needs an isolated environment, the last thing I'd want to do is give it the "keys to the castle" of my K8s cluster.

Can you elaborate? In case a CI job wants to deploy some resources into some namespaces, how to give the job pod deploy permissions with a credential? Even the namespace to deploy in the job can be dynamic, for instance, the job might want to deploy into namespace identified by git branch, etc.

Via a ServiceAccount

#3: I thought about mounting pvc to store cache, but that is problematic for concurrent jobs, as a single pvc is generally not possible to be mounted to multiple nodes for read/write, not to mention the configuration overhead. I also thought about uploading/downloading cache to particular slot of some file system service, but that is much slower, and can even be slower than the case without cache if you have a fast network. I guess there is no perfect approach for caching.

I wouldn't put too much emphasis on caching; I think for most folks that are using this, re-downloading a few dependencies isn't that big of a deal. At an enterprise scale, maybe. Every major cloud provider these days has ReadWriteMany volume types now for this.

Anyways, I won't berate these issues any more; just wanted to give you some feedback. Thanks

Robin Shen commented 2 weeks ago

I will think about your comments for future development. Thanks a lot for all the feedback.

issue 1 of 1
Type
Improvement
Priority
Normal
Assignee
Issue Votes (0)
Watchers (3)
Reference
issue onedev/server#442
Please wait...
Page is in error, reload to recover