Skip to content
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

[WIP] Adding starter code for clustersets #109

Closed
wants to merge 1 commit into from

Conversation

MbolotSuse
Copy link
Contributor

@MbolotSuse MbolotSuse commented Aug 22, 2024

This PR is a POC of adding a clusterset type that allows admins to selectively limit the Resources that a given group of clusters can consume.

Overview

  • Updates several items on the cluster type.
    • The cluster type is now namespaced.
      • The pods for the cluster (server/agent) are now in the same namespace as the cluster
    • The type contains a Limit field allowing users to specify the server/agent limits (separately) that apply to each replica. Note that this is applied separately to each replica - so with 2 agents and a limit of 1 CPU for the worker node, each agent will get a limit of 1 CPU for a cluster-wide total of 2 CPU.
    • The type contains a node selector field allowing users to constrain which nodes worker/servers run on.
    • The pods are now deployed with anti-affinity which causes agents to not be scheduled on the same host cluster node as other agents. Servers have the same anti-affinity with other servers. This does not apply to servers/agents (so a server can run on the same host node as an agent, but not on the same node as another server).
  • Adds a new clusterset type.
    • MaxLimits provides an upper limit for the limits in the cluster. Note that this works like the limits from a ResourceQuota - if no limit is specified for that resource, clusters in this set have no limit. These limits are the sum across all servers/workers.
    • DefaultLimits provides the defaults for new clusters which don't specify a limit (mostly a UX item so that new clusters don't need to explicitly set these values).
    • DefaultNodeSelector provides a starting node selector for all clusters in the set.
  • Adds webhook validation to enforce the clusterset values on new/updated clusters.
  • Updates the cluster type with field-level docs.
  • Adds a script to automatically generate CRDs from go types, and ports existing validation to the go-type annotations.

Using/testing

  1. Clone the branch.
  2. Build the binary make build.
  3. Build/Push the image docker build -f package/Dockerfile . -t $REPO/$IMAGE:$TAG
  4. Update values.yaml with image.repostory: $REPO/$IMAGE and tag: $TAG
  5. Generate a key using openssl: openssl genrsa -out rootCAKey.pem 4096.
  6. Generate a cert using openssl: openssl req -x509 -sha256 -nodes -key rootCAKey.pem -days 3650 -out rootCACert.pem --addext "subjectAltName=DNS:k3k-webhook.k3k-system.svc".
  7. Copy the cert: cat rootCACert.pem | base64 | tr -d '\n'
  8. Create the namespace and upload the cert as a secret kubectl create -f ns.yaml && k create secret tls webhook-secret -n k3k-system --cert=rootCACert.pem --key=rootCAKey.pem. See below for the namespace (needs helm annotations to be imported when installing the chart).
  9. Update charts/k3k/templates/webhooks.yaml with the value copied in the previous step (past in the caBundle field where "ReplaceMe" is for both the Valdating and Mutating Webhook).
  10. Deploy the chart helm install k3k ./charts/k3k -n k3k-system
apiVersion: v1
kind: Namespace
metadata:
  annotations:
    meta.helm.sh/release-name: k3k
    meta.helm.sh/release-namespace: k3k-system
  labels:
    app.kubernetes.io/managed-by: Helm
    kubernetes.io/metadata.name: k3k-system
  name: k3k-system

Note

  • Right now this PR is in a POC state, and needs more testing/refinement before it could be merged. Some examples of this include:
    • There's an unnecessary clusterset controller
    • There's no clusterset webhook
    • The webhook needs to be manually configured with a secret
    • The chart doesn't tolerate an existing k3k-system namespace
    • Some of the fields are misnamed (cluster.Limit, cluster.Limit.WorkerLimit).
    • Limits and Defaults should probably be two different objects (like LimitRange and ResourceQuota) so that users can give RBAC to set defaults without giving RBAC to controller the upper limits.
    • Webhook needs a mutex so that it locks when calculating if a cluster exceeds the limits for a namespace.
    • The bootstrap secret needs an ownerref to the cluster so that it gets removed when the cluster does.
    • The CLI needs to be reviewed for the potential of adding new fields for various limits.
    • We should evaluate using --disable-agent on the server nodes by default (or even by force), which would prevent users from scheduling pods on the server node.

@@ -29,6 +29,10 @@ func delete(clx *cli.Context) error {
if err != nil {
return err
}
namespace := "default"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use pkg/controller/util package and add the defaultNamespace there as a constant like we do with the K3kSystemNamespace

}

if err := cluster.AddPodController(ctx, mgr); err != nil {
klog.Fatalf("Failed to add the new cluster controller: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
klog.Fatalf("Failed to add the new cluster controller: %v", err)
klog.Fatalf("Failed to add the new pod controller: %v", err)

the agent nodes
type: object
type: object
defaultNodeSelector:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the defaults I think we need to add a validation here so that they can't be empty, or in the code to have default values for default limits and default node selector

return fmt.Errorf("error when finding cluster set: %w", err)
}
if !found {
// this cluster isn't in a cluster set, don't apply any defaults
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a log here stating that not just a comment

@MbolotSuse MbolotSuse force-pushed the cluster-set branch 2 times, most recently from 1cf73c5 to 1cb8651 Compare September 18, 2024 18:51
Adds types for cluster sets, which allows constraining a few elements of
clusters including: overall resource usage, and which nodes it can use.
@galal-hussein
Copy link
Collaborator

Closed in favor of #128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants