Skip to content

Commit

Permalink
feat: Add feature gates flag (#368)
Browse files Browse the repository at this point in the history
**Reason for Change**:
- Add `feature-gates` flag to the Kaito controller.
- Support the `karpenter` feature gate with default value, `false`. 
- Enable setting the feature gate flag via helm chart.

In addition to some chores:
- Exclude the auto generated and mock files from unit test coverage.
- Upgrade k8s version using in the tests to the current supported
version.

**Requirements**

- [x] added unit tests and e2e tests (if applicable).

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 4321, add "Fixes #4321" to the next
line. -->

**Notes for Reviewers**:

---------

Signed-off-by: Heba Elayoty <[email protected]>
  • Loading branch information
helayoty authored May 1, 2024
1 parent 536f259 commit a4bcec4
Show file tree
Hide file tree
Showing 13 changed files with 153 additions and 19 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/e2e-workflow.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ on:
default: "eastus"
k8s_version:
type: string
default: "1.27"
default: "1.29.2"
secrets:
E2E_CLIENT_ID:
required: true
Expand Down
6 changes: 4 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ GINKGO := $(TOOLS_BIN_DIR)/$(GINKGO_BIN)-$(GINKGO_VER)

AZURE_SUBSCRIPTION_ID ?= $(AZURE_SUBSCRIPTION_ID)
AZURE_LOCATION ?= eastus
AKS_K8S_VERSION ?= 1.27.2
AKS_K8S_VERSION ?= 1.29.2
AZURE_RESOURCE_GROUP ?= demo
AZURE_CLUSTER_NAME ?= kaito-demo
AZURE_RESOURCE_GROUP_MC=MC_$(AZURE_RESOURCE_GROUP)_$(AZURE_CLUSTER_NAME)_$(AZURE_LOCATION)
Expand Down Expand Up @@ -84,7 +84,9 @@ fmt: ## Run go fmt against code.
## --------------------------------------
.PHONY: unit-test
unit-test: ## Run unit tests.
go test -v $(shell go list ./pkg/... ./api/... | grep -v /vendor) -race -coverprofile=coverage.txt -covermode=atomic
go test -v $(shell go list ./pkg/... ./api/... | \
grep -v -e /vendor -e /api/v1alpha1/zz_generated.deepcopy.go -e /pkg/utils/test/...) \
-race -coverprofile=coverage.txt -covermode=atomic
go tool cover -func=coverage.txt

inference-api-e2e:
Expand Down
6 changes: 3 additions & 3 deletions api/v1alpha1/workspace_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"testing"

"github.com/azure/kaito/pkg/k8sclient"
"github.com/azure/kaito/pkg/utils"
"github.com/azure/kaito/pkg/utils/consts"
"github.com/azure/kaito/pkg/utils/plugin"
"k8s.io/apimachinery/pkg/runtime"

Expand Down Expand Up @@ -694,8 +694,8 @@ func TestWorkspaceValidateUpdate(t *testing.T) {
func TestTuningSpecValidateCreate(t *testing.T) {
RegisterValidationTestModels()
// Set ReleaseNamespace Env
os.Setenv(utils.DefaultReleaseNamespaceEnvVar, DefaultReleaseNamespace)
defer os.Unsetenv(utils.DefaultReleaseNamespaceEnvVar)
os.Setenv(consts.DefaultReleaseNamespaceEnvVar, DefaultReleaseNamespace)
defer os.Unsetenv(consts.DefaultReleaseNamespaceEnvVar)

// Create fake client with default ConfigMap
scheme := runtime.NewScheme()
Expand Down
2 changes: 2 additions & 0 deletions charts/kaito/workspace/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ spec:
{{- toYaml .Values.securityContext | nindent 12 }}
image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
args:
- --feature-gates={{- range $k, $v := .Values.featureGates }}{{ $k }}={{ $v}}{{- end }}
env:
- name: WEBHOOK_SERVICE
value: {{ include "kaito.fullname" . }}
Expand Down
2 changes: 2 additions & 0 deletions charts/kaito/workspace/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ securityContext:
capabilities:
drop:
- "ALL"
featureGates:
Karpenter: "false"
webhook:
port: 9443
presetRegistryName: mcr.microsoft.com/aks/kaito
Expand Down
8 changes: 8 additions & 0 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strconv"
"time"

"github.com/azure/kaito/pkg/featuregates"
"github.com/azure/kaito/pkg/k8sclient"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/karpenter/pkg/apis/v1beta1"
Expand Down Expand Up @@ -64,13 +65,15 @@ func main() {
var enableLeaderElection bool
var enableWebhook bool
var probeAddr string
var featureGates string
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
"Enable leader election for controller manager. "+
"Enabling this will ensure there is only one active controller manager.")
flag.BoolVar(&enableWebhook, "webhook", true,
"Enable webhook for controller manager. Default is true.")
flag.StringVar(&featureGates, "feature-gates", "Karpenter=false", "Enable Kaito feature gates. Default, Karpenter=false.")
opts := zap.Options{
Development: true,
}
Expand Down Expand Up @@ -144,6 +147,11 @@ func main() {

// wait 2 seconds to allow reconciling webhookconfiguration and service endpoint.
time.Sleep(2 * time.Second)

if err = featuregates.ParseAndValidateFeatureGates(featureGates); err != nil {
klog.ErrorS(err, "unable to set `feature-gates` flag")
exitWithErrorFunc()
}
}

klog.InfoS("starting manager")
Expand Down
5 changes: 3 additions & 2 deletions pkg/controllers/workspace_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/azure/kaito/pkg/tuning"
"github.com/azure/kaito/pkg/utils/consts"
batchv1 "k8s.io/api/batch/v1"

"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
Expand Down Expand Up @@ -66,8 +67,8 @@ func (c *WorkspaceReconciler) Reconcile(ctx context.Context, req reconcile.Reque
return c.deleteWorkspace(ctx, workspaceObj)
} else {
// Ensure finalizer
if !controllerutil.ContainsFinalizer(workspaceObj, utils.WorkspaceFinalizer) {
controllerutil.AddFinalizer(workspaceObj, utils.WorkspaceFinalizer)
if !controllerutil.ContainsFinalizer(workspaceObj, consts.WorkspaceFinalizer) {
controllerutil.AddFinalizer(workspaceObj, consts.WorkspaceFinalizer)
updateCopy := workspaceObj.DeepCopy()
if updateErr := c.Update(ctx, updateCopy, &client.UpdateOptions{}); updateErr != nil {
klog.ErrorS(updateErr, "failed to ensure the finalizer to the workspace",
Expand Down
4 changes: 2 additions & 2 deletions pkg/controllers/workspace_gc_finalizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

kaitov1alpha1 "github.com/azure/kaito/api/v1alpha1"
"github.com/azure/kaito/pkg/machine"
"github.com/azure/kaito/pkg/utils"
"github.com/azure/kaito/pkg/utils/consts"
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -39,6 +39,6 @@ func (c *WorkspaceReconciler) garbageCollectWorkspace(ctx context.Context, wObj
}
klog.InfoS("successfully removed the workspace finalizers",
"workspace", klog.KObj(wObj))
controllerutil.RemoveFinalizer(wObj, utils.WorkspaceFinalizer)
controllerutil.RemoveFinalizer(wObj, consts.WorkspaceFinalizer)
return ctrl.Result{}, nil
}
47 changes: 47 additions & 0 deletions pkg/featuregates/featuregates.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

package featuregates

import (
"errors"
"fmt"

"github.com/azure/kaito/pkg/utils/consts"
cliflag "k8s.io/component-base/cli/flag"
)

var (
// FeatureGates is a map that holds the feature gates and their default values for Kaito.
FeatureGates = map[string]bool{
consts.FeatureFlagKarpenter: false,
// Add more feature gates here
}
)

// ParseAndValidateFeatureGates parses the feature gates flag and sets the environment variables for each feature.
func ParseAndValidateFeatureGates(featureGates string) error {
gateMap := map[string]bool{}
if err := cliflag.NewMapStringBool(&gateMap).Set(featureGates); err != nil {
return err
}
if len(gateMap) == 0 {
// no feature gates set
return nil
}

var invalidFeatures string
for key, val := range gateMap {
if _, ok := FeatureGates[key]; !ok {
invalidFeatures = fmt.Sprintf("%s, %s", invalidFeatures, key)
continue
}
FeatureGates[key] = val
}

if invalidFeatures != "" {
return errors.New("invalid feature gate(s) " + invalidFeatures)
}

return nil
}
63 changes: 63 additions & 0 deletions pkg/featuregates/featuregates_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
package featuregates

import (
"testing"

"gotest.tools/assert"
)

func TestParseFeatureGates(t *testing.T) {
tests := []struct {
name string
featureGates string
expectedError bool
expectedValue string
}{
{
name: "WithValidEnableFeatureGates",
featureGates: "Karpenter=true",
expectedError: false,
expectedValue: "true",
},
{
name: "WithDuplicateFeatureGates",
featureGates: "Karpenter=false,Karpenter=true",
expectedError: false,
expectedValue: "true", // Apply the last value.
},
{
name: "WithInvalidFeatureGates",
featureGates: "invalid",
expectedError: true,
},
{
name: "WithUnsupportedFeatureGate",
featureGates: "unsupported=true,Karpenter=false",
expectedError: true,
},
{
name: "WithValidDisableFeatureGates",
featureGates: "Karpenter=false",
expectedError: false,
expectedValue: "false",
},
{
name: "WithEmptyFeatureGates",
featureGates: "",
expectedError: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := ParseAndValidateFeatureGates(tt.featureGates)
if tt.expectedError {
assert.Check(t, err != nil, "expected error but got nil")
} else {
assert.NilError(t, err)
}
})
}
}
5 changes: 3 additions & 2 deletions pkg/tuning/preset-tuning_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/azure/kaito/pkg/utils"
"github.com/azure/kaito/pkg/utils/consts"

kaitov1alpha1 "github.com/azure/kaito/api/v1alpha1"
"github.com/azure/kaito/pkg/model"
Expand Down Expand Up @@ -169,7 +170,7 @@ func TestEnsureTuningConfigMap(t *testing.T) {
}{
"Config already exists in workspace namespace": {
callMocks: func(c *test.MockClient) {
os.Setenv("RELEASE_NAMESPACE", "release-namespace")
os.Setenv(consts.DefaultReleaseNamespaceEnvVar, "release-namespace")
c.On("Get", mock.IsType(context.Background()), mock.Anything, mock.IsType(&corev1.ConfigMap{}), mock.Anything).Return(nil)
},
workspaceObj: &kaitov1alpha1.Workspace{
Expand All @@ -192,7 +193,7 @@ func TestEnsureTuningConfigMap(t *testing.T) {
},
"Config doesn't exist in template namespace": {
callMocks: func(c *test.MockClient) {
os.Setenv("RELEASE_NAMESPACE", "release-namespace")
os.Setenv(consts.DefaultReleaseNamespaceEnvVar, "release-namespace")
c.On("Get", mock.IsType(context.Background()), mock.Anything, mock.IsType(&corev1.ConfigMap{}), mock.Anything).Return(errors.NewNotFound(schema.GroupResource{}, "config-template"))
},
workspaceObj: &kaitov1alpha1.Workspace{
Expand Down
11 changes: 4 additions & 7 deletions pkg/utils/common.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

package utils

import (
"fmt"
"io/ioutil"
"os"
)

const (
// WorkspaceFinalizer is used to make sure that workspace controller handles garbage collection.
WorkspaceFinalizer = "workspace.finalizer.kaito.sh"
DefaultReleaseNamespaceEnvVar = "RELEASE_NAMESPACE"
"github.com/azure/kaito/pkg/utils/consts"
)

func Contains(s []string, e string) bool {
Expand Down Expand Up @@ -76,8 +73,8 @@ func GetReleaseNamespace() (string, error) {
}

// Fallback: Read the namespace from an environment variable
if namespace, exists := os.LookupEnv(DefaultReleaseNamespaceEnvVar); exists {
if namespace, exists := os.LookupEnv(consts.DefaultReleaseNamespaceEnvVar); exists {
return namespace, nil
}
return "", fmt.Errorf("failed to determine release namespace from file %s and env var %s", namespaceFilePath, DefaultReleaseNamespaceEnvVar)
return "", fmt.Errorf("failed to determine release namespace from file %s and env var %s", namespaceFilePath, consts.DefaultReleaseNamespaceEnvVar)
}
11 changes: 11 additions & 0 deletions pkg/utils/consts/consts.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

package consts

const (
// WorkspaceFinalizer is used to make sure that workspace controller handles garbage collection.
WorkspaceFinalizer = "workspace.finalizer.kaito.sh"
DefaultReleaseNamespaceEnvVar = "RELEASE_NAMESPACE"
FeatureFlagKarpenter = "Karpenter"
)

0 comments on commit a4bcec4

Please sign in to comment.