Merge pull request #64 from fuweid/weifu/render-node-after-kwok

*: use two releases to deploy virtual nodes

manifests/virtualcluster/nodecontrollers/Chart.yaml

@@ -0,0 +1,3 @@
+apiVersion: v1
+name: virtualnode-controllers
+version: "0.0.1"

manifests/virtualcluster/nodes/templates/statefulsets.tpl → manifests/virtualcluster/nodecontrollers/templates/statefulsets.tpl

@@ -14,13 +14,13 @@ spec:
       labels:
         app: {{ .Values.name }}
     spec:
-{{- if .Values.controllerNodeSelectors }}
+{{- if .Values.nodeSelectors }}
       affinity:
         nodeAffinity:
           requiredDuringSchedulingIgnoredDuringExecution:
             nodeSelectorTerms:
             - matchExpressions:
-  {{- range $key, $values := .Values.controllerNodeSelectors }}
+  {{- range $key, $values := .Values.nodeSelectors }}
               - key: "{{ $key }}"
                 operator: In
                 values:

manifests/virtualcluster/nodecontrollers/values.yaml

@@ -0,0 +1,3 @@
+name: "vc-testing"
+nodeSelectors: {}
+replicas: 0

manifests/virtualcluster/nodes/Chart.yaml

@@ -1,5 +1,3 @@
-{
-  apiVersion: v1,
-  "name": "virtualnodes",
-  "version": "0.0.1"
-}
+apiVersion: v1
+name: virtualnodes
+version: "0.0.1"

manifests/virtualcluster/nodes/values.yaml

@@ -1,6 +1,5 @@
 name: "vc-testing"
-controllerNodeSelectors: {}
-replicas: 0
 nodeLabels: {}
+replicas: 0
 cpu: 0
 memory: 0

virtualcluster/nodes_common.go

@@ -2,8 +2,10 @@ package virtualcluster
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/Azure/kperf/helmcli"
+
 	"sigs.k8s.io/yaml"
 )
 
@@ -25,15 +27,25 @@ const (
 	// virtualnodeChartName should be aligned with ../manifests/virtualcluster/nodes.
 	virtualnodeChartName = "virtualcluster/nodes"
 
+	// virtualnodeControllerChartName should be aligned with ../manifests/virtualcluster/nodecontrollers.
+	virtualnodeControllerChartName = "virtualcluster/nodecontrollers"
+
 	// virtualnodeReleaseNamespace is used to host virtual nodes.
 	//
 	// NOTE: The Node resource is cluster-scope. Just in case that new node
 	// name is conflict with existing one, we should use fixed namespace
 	// to store all the resources related to virtual nodes.
 	virtualnodeReleaseNamespace = "virtualnodes-kperf-io"
+
+	// reservedNodepoolSuffixName is used to render virtualnodes/nodecontrollers.
+	//
+	// NOTE: Please check the details in ./nodes_create.go.
+	reservedNodepoolSuffixName = "-controller"
 )
 
 type nodepoolConfig struct {
+	// name represents the name of node pool.
+	name string
 	// count represents the desired number of node.
 	count int
 	// cpu represents a logical CPU resource provided by virtual node.
@@ -52,9 +64,25 @@ func (cfg *nodepoolConfig) validate() error {
 		return fmt.Errorf("invalid count=%d or cpu=%d or memory=%d",
 			cfg.count, cfg.cpu, cfg.memory)
 	}
+
+	if cfg.name == "" {
+		return fmt.Errorf("required non-empty name")
+	}
+
+	if strings.HasSuffix(cfg.name, reservedNodepoolSuffixName) {
+		return fmt.Errorf("name can't contain %s as suffix", reservedNodepoolSuffixName)
+	}
 	return nil
 }
 
+func (cfg *nodepoolConfig) nodeHelmReleaseName() string {
+	return cfg.name
+}
+
+func (cfg *nodepoolConfig) nodeControllerHelmReleaseName() string {
+	return cfg.name + reservedNodepoolSuffixName
+}
+
 // NodepoolOpt is used to update default node pool's setting.
 type NodepoolOpt func(*nodepoolConfig)
 
@@ -94,22 +122,31 @@ func WithNodepoolNodeControllerAffinity(nodeSelectors map[string][]string) Nodep
 	}
 }
 
-// toHelmValuesAppliers creates ValuesAppliers.
+// toNodeHelmValuesAppliers creates ValuesAppliers.
 //
 // NOTE: Please align with ../manifests/virtualcluster/nodes/values.yaml
-//
-// TODO: Add YAML ValuesAppliers to support array type.
-func (cfg *nodepoolConfig) toHelmValuesAppliers(nodepoolName string) ([]helmcli.ValuesApplier, error) {
+func (cfg *nodepoolConfig) toNodeHelmValuesAppliers() []helmcli.ValuesApplier {
 	res := make([]string, 0, 4)
 
-	res = append(res, fmt.Sprintf("name=%s", nodepoolName))
-	res = append(res, fmt.Sprintf("replicas=%d", cfg.count))
+	res = append(res, fmt.Sprintf("name=%s", cfg.name))
 	res = append(res, fmt.Sprintf("cpu=%d", cfg.cpu))
 	res = append(res, fmt.Sprintf("memory=%d", cfg.memory))
+	res = append(res, fmt.Sprintf("replicas=%d", cfg.count))
 
-	stringPathApplier := helmcli.StringPathValuesApplier(res...)
+	return []helmcli.ValuesApplier{helmcli.StringPathValuesApplier(res...)}
+}
 
-	nodeSelectorsYaml, err := cfg.renderNodeSelectors()
+// toNodeControllerHelmValuesAppliers creates ValuesAppliers.
+//
+// NOTE: Please align with ../manifests/virtualcluster/nodecontrollers/values.yaml
+func (cfg *nodepoolConfig) toNodeControllerHelmValuesAppliers() ([]helmcli.ValuesApplier, error) {
+	res := make([]string, 0, 2)
+
+	res = append(res, fmt.Sprintf("name=%s", cfg.name))
+	res = append(res, fmt.Sprintf("replicas=%d", cfg.count))
+
+	stringPathApplier := helmcli.StringPathValuesApplier(res...)
+	nodeSelectorsYaml, err := cfg.renderNodeControllerNodeSelectors()
 	if err != nil {
 		return nil, err
 	}
@@ -121,12 +158,13 @@ func (cfg *nodepoolConfig) toHelmValuesAppliers(nodepoolName string) ([]helmcli.
 	return []helmcli.ValuesApplier{stringPathApplier, nodeSelectorsApplier}, nil
 }
 
-// renderNodeSelectors renders nodeSelectors config into YAML string.
+// renderNodeControllerNodeSelectors renders node controller's nodeSelectors
+// config into YAML string.
 //
-// NOTE: Please align with ../manifests/virtualcluster/nodes/values.yaml
-func (cfg *nodepoolConfig) renderNodeSelectors() (string, error) {
+// NOTE: Please align with ../manifests/virtualcluster/nodecontrollers/values.yaml
+func (cfg *nodepoolConfig) renderNodeControllerNodeSelectors() (string, error) {
 	target := map[string]interface{}{
-		"controllerNodeSelectors": cfg.nodeSelectors,
+		"nodeSelectors": cfg.nodeSelectors,
 	}
 
 	rawData, err := yaml.Marshal(target)

virtualcluster/nodes_create.go

@@ -14,45 +14,102 @@ import (
 // TODO:
 // 1. create a new package to define ErrNotFound, ErrAlreadyExists, ... errors.
 // 2. support configurable timeout.
-func CreateNodepool(ctx context.Context, kubeconfigPath string, nodepoolName string, opts ...NodepoolOpt) error {
+//
+// FIXME:
+//
+// Some cloud providers will delete unknown or not-ready nodes. If we render
+// both nodes and controllers in one helm release, helm won't wait for
+// controller ready before creating nodes. The nodes will be deleted by cloud
+// providers. The helm's post-install or post-upgrade hook can ensure that it
+// won't deploy nodes until controllers ready. However, resources created by
+// helm hook aren't part of helm release. We need extra step to cleanup nodes
+// resources when we delete nodepool's helm release. Based on this fact, we
+// separate one helm release into two. One is for controllers and other one
+// is for nodes.
+//
+// However, it's not a guarantee. When controller was deleted and it takes long
+// time to restart, the node will be marked NotReady and deleted by cloud providers.
+// Maybe we can consider to contribute to difference cloud providers with
+// workaround. For example, if node.Spec.ProviderID contains `?ignore=virtual`,
+// the cloud providers should ignore this kind of nodes.
+func CreateNodepool(ctx context.Context, kubeCfgPath string, nodepoolName string, opts ...NodepoolOpt) (retErr error) {
 	cfg := defaultNodepoolCfg
 	for _, opt := range opts {
 		opt(&cfg)
 	}
+	cfg.name = nodepoolName
 
 	if err := cfg.validate(); err != nil {
 		return err
 	}
 
-	getCli, err := helmcli.NewGetCli(kubeconfigPath, virtualnodeReleaseNamespace)
+	getCli, err := helmcli.NewGetCli(kubeCfgPath, virtualnodeReleaseNamespace)
 	if err != nil {
 		return fmt.Errorf("failed to create helm get client: %w", err)
 	}
 
-	_, err = getCli.Get(nodepoolName)
+	_, err = getCli.Get(cfg.nodeHelmReleaseName())
 	if err == nil {
-		return fmt.Errorf("nodepool %s already exists", nodepoolName)
+		return fmt.Errorf("nodepool %s already exists", cfg.nodeHelmReleaseName())
 	}
 
-	ch, err := manifests.LoadChart(virtualnodeChartName)
+	cleanupFn, err := createNodepoolController(ctx, kubeCfgPath, &cfg)
 	if err != nil {
-		return fmt.Errorf("failed to load virtual node chart: %w", err)
+		return err
 	}
+	defer func() {
+		// NOTE: Try best to cleanup. If there is leaky resources after
+		// force stop, like kill process, it needs cleanup manually.
+		if retErr != nil {
+			_ = cleanupFn()
+		}
+	}()
 
-	cfgValues, err := cfg.toHelmValuesAppliers(nodepoolName)
+	ch, err := manifests.LoadChart(virtualnodeChartName)
 	if err != nil {
-		return fmt.Errorf("failed to convert to helm values: %w", err)
+		return fmt.Errorf("failed to load virtual node chart: %w", err)
 	}
+
 	releaseCli, err := helmcli.NewReleaseCli(
-		kubeconfigPath,
+		kubeCfgPath,
 		virtualnodeReleaseNamespace,
-		nodepoolName,
+		cfg.nodeHelmReleaseName(),
 		ch,
 		virtualnodeReleaseLabels,
-		cfgValues...,
+		cfg.toNodeHelmValuesAppliers()...,
 	)
 	if err != nil {
 		return fmt.Errorf("failed to create helm release client: %w", err)
 	}
 	return releaseCli.Deploy(ctx, 120*time.Second)
 }
+
+// createNodepoolController creates node controller release.
+func createNodepoolController(ctx context.Context, kubeCfgPath string, cfg *nodepoolConfig) (_cleanup func() error, _ error) {
+	ch, err := manifests.LoadChart(virtualnodeControllerChartName)
+	if err != nil {
+		return nil, fmt.Errorf("failed to load virtual node controller chart: %w", err)
+	}
+
+	appliers, err := cfg.toNodeControllerHelmValuesAppliers()
+	if err != nil {
+		return nil, err
+	}
+
+	releaseCli, err := helmcli.NewReleaseCli(
+		kubeCfgPath,
+		virtualnodeReleaseNamespace,
+		cfg.nodeControllerHelmReleaseName(),
+		ch,
+		virtualnodeReleaseLabels,
+		appliers...,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create helm release client: %w", err)
+	}
+
+	if err := releaseCli.Deploy(ctx, 120*time.Second); err != nil {
+		return nil, fmt.Errorf("failed to deploy virtual node controller: %w", err)
+	}
+	return releaseCli.Uninstall, nil
+}

virtualcluster/nodes_delete.go

@@ -2,17 +2,32 @@ package virtualcluster
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/Azure/kperf/helmcli"
+
+	"helm.sh/helm/v3/pkg/storage/driver"
 )
 
 // DeleteNodepool deletes a node pool with a given name.
 func DeleteNodepool(_ context.Context, kubeconfigPath string, nodepoolName string) error {
+	cfg := defaultNodepoolCfg
+	cfg.name = nodepoolName
+
+	if err := cfg.validate(); err != nil {
+		return err
+	}
+
 	delCli, err := helmcli.NewDeleteCli(kubeconfigPath, virtualnodeReleaseNamespace)
 	if err != nil {
 		return fmt.Errorf("failed to create helm delete client: %w", err)
 	}
 
-	return delCli.Delete(nodepoolName)
+	// delete virtual node controller first
+	err = delCli.Delete(cfg.nodeControllerHelmReleaseName())
+	if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
+		return fmt.Errorf("failed to cleanup virtual node controller: %w", err)
+	}
+	return delCli.Delete(cfg.nodeHelmReleaseName())
 }

virtualcluster/node_list.go → virtualcluster/nodes_list.go

@@ -3,6 +3,7 @@ package virtualcluster
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"helm.sh/helm/v3/pkg/release"
 
@@ -16,6 +17,19 @@ func ListNodepools(_ context.Context, kubeconfigPath string) ([]*release.Release
 		return nil, fmt.Errorf("failed to create helm list client: %w", err)
 	}
 
-	return listCli.List()
+	releases, err := listCli.List()
+	if err != nil {
+		return nil, fmt.Errorf("failed to list nodepool: %w", err)
+	}
 
+	// NOTE: Skip node controllers
+	res := make([]*release.Release, 0, len(releases)/2)
+	for idx := range releases {
+		r := releases[idx]
+		if strings.HasSuffix(r.Name, reservedNodepoolSuffixName) {
+			continue
+		}
+		res = append(res, r)
+	}
+	return res, nil
 }