Merge branch 'main' into zacharyb/more-logging

.vscode/settings.json

@@ -1 +1,10 @@
-{}
+{
+  "go.lintTool": "golangci-lint",
+  "go.lintFlags": ["run", "--enable=errcheck"],
+  "go.toolsEnvVars": {
+    "GO111MODULE": "on"
+  },
+  "go.lintOnSave": "package",
+  "go.vetOnSave": "package",
+  "go.useLanguageServer": true
+}

CHANGELOG.md

@@ -2,6 +2,20 @@
 
 All notable changes to this project will be documented in this file.
 
+## [1.13.0](https://github.com/wandb/operator/compare/v1.12.5...v1.13.0) (2024-09-09)
+
+
+### Features
+
+* Release Version Pinning Init ([#28](https://github.com/wandb/operator/issues/28)) ([dfe8bda](https://github.com/wandb/operator/commit/dfe8bda333a73b95bce748c953119292134a6347))
+
+### [1.12.5](https://github.com/wandb/operator/compare/v1.12.4...v1.12.5) (2024-09-03)
+
+
+### Bug Fixes
+
+* Debug logging errors ([#26](https://github.com/wandb/operator/issues/26)) ([a641621](https://github.com/wandb/operator/commit/a64162125994b9c43f5253f3fb9f097773740bc7))
+
 ### [1.12.4](https://github.com/wandb/operator/compare/v1.12.3...v1.12.4) (2024-09-03)
 
 

README.md

@@ -5,3 +5,11 @@
 - `make install run` - runs the operator outside of the cluster. This is in a
   non production mode, so the console will fail to load operator logs because
   its not inside the cluster
+
+## Testing
+
+### Counterfeiter
+
+```bash
+go generate ./...
+```

controllers/weightsandbiases_controller.go

@@ -111,6 +111,14 @@ func (r *WeightsAndBiasesReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		specManager.SetUserInput(userInputSpec)
 	}
 
+	var releaseID string
+	if userInputSpec != nil {
+		if releaseIDValue, ok := userInputSpec.Values["_releaseId"].(string); ok {
+			releaseID = releaseIDValue
+			log.Info("Version Pinning is enabled", "releaseId:", releaseID)
+		}
+	}
+
 	crdSpec := operator.Spec(wandb)
 
 	currentActiveSpec, err := specManager.GetActive()
@@ -125,7 +133,11 @@ func (r *WeightsAndBiasesReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 	var deployerSpec *spec.Spec
 	if !r.IsAirgapped {
-		deployerSpec, err = r.DeployerClient.GetSpec(license, currentActiveSpec)
+		deployerSpec, err = r.DeployerClient.GetSpec(deployer.GetSpecOptions{
+			License:     license,
+			ActiveState: currentActiveSpec,
+			ReleaseId:   releaseID,
+		})
 		if err != nil {
 			log.Info("Failed to get spec from deployer", "error", err)
 			// This scenario may occur if the user disables networking, or if the deployer
@@ -160,22 +172,34 @@ func (r *WeightsAndBiasesReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 
 	// First takes precedence
-	desiredSpec.Merge(crdSpec)
+	if err := desiredSpec.Merge(crdSpec); err != nil {
+		log.Error(err, "Failed to merge CRD spec into desired spec")
+		return ctrlqueue.RequeueWithError(err)
+	}
 	if r.Debug {
 		log.Info("Desired spec after merging crdSpec", "spec", desiredSpec.SensitiveValuesMasked())
 	}
 
-	desiredSpec.Merge(userInputSpec)
+	if err := desiredSpec.Merge(userInputSpec); err != nil {
+		log.Error(err, "Failed to merge user input spec into desired spec")
+		return ctrlqueue.RequeueWithError(err)
+	}
 	if r.Debug {
 		log.Info("Desired spec after merging userInputSpec", "spec", desiredSpec.SensitiveValuesMasked())
 	}
 
-	desiredSpec.Merge(deployerSpec)
+	if err := desiredSpec.Merge(deployerSpec); err != nil {
+		log.Error(err, "Failed to merge deployer spec into desired spec")
+		return ctrlqueue.RequeueWithError(err)
+	}
 	if r.Debug {
 		log.Info("Desired spec after merging deployerSpec", "spec", desiredSpec.SensitiveValuesMasked())
 	}
 
-	desiredSpec.Merge(operator.Defaults(wandb, r.Scheme))
+	if err := desiredSpec.Merge(operator.Defaults(wandb, r.Scheme)); err != nil {
+		log.Error(err, "Failed to merge operator defaults into desired spec")
+		return ctrlqueue.RequeueWithError(err)
+	}
 	if r.Debug {
 		log.Info("Desired spec after merging operator defaults", "spec", desiredSpec.SensitiveValuesMasked())
 	}

controllers/weightsandbiases_controller_test.go

@@ -2,19 +2,22 @@ package controllers
 
 import (
 	"context"
+	"time"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	wandbcomv1 "github.com/wandb/operator/api/v1"
 	"github.com/wandb/operator/pkg/wandb/spec"
 	"github.com/wandb/operator/pkg/wandb/spec/channel/deployer/deployerfakes"
 	"github.com/wandb/operator/pkg/wandb/spec/charts"
+	"github.com/wandb/operator/pkg/wandb/spec/state"
+	"github.com/wandb/operator/pkg/wandb/spec/state/secrets"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"time"
 )
 
 var deployerSpec = spec.Spec{
@@ -137,6 +140,78 @@ var _ = Describe("WeightsandbiasesController", func() {
 			})
 		})
 	})
+	Describe("Reconcile with _releaseId set", func() {
+		BeforeEach(func() {
+			ctx := context.Background()
+			recorder = record.NewFakeRecorder(10)
+			deployerClient := &deployerfakes.FakeDeployerInterface{}
+			deployerClient.GetSpecReturns(&deployerSpec, nil)
+			reconciler = &WeightsAndBiasesReconciler{
+				Client:         k8sClient,
+				IsAirgapped:    false,
+				DeployerClient: deployerClient,
+				Scheme:         scheme.Scheme,
+				Recorder:       recorder,
+				DryRun:         true,
+			}
+			wandb := wandbcomv1.WeightsAndBiases{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-release-id",
+					Namespace: "default",
+				},
+				Spec: wandbcomv1.WeightsAndBiasesSpec{
+					Chart: wandbcomv1.Object{Object: map[string]interface{}{}},
+					Values: wandbcomv1.Object{Object: map[string]interface{}{
+						"global": map[string]interface{}{
+							"host": "https://qa-google.wandb.io",
+						},
+					}},
+				},
+			}
+			err := k8sClient.Create(ctx, &wandb)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Create UserSpec with _releaseId
+			userSpec := &spec.Spec{
+				Values: map[string]interface{}{
+					"_releaseId": "0b901113-8135-48ae-bdaf-6fa82b4b2d28",
+				},
+			}
+			err = state.New(ctx, k8sClient, &wandb, scheme.Scheme, secrets.New(ctx, k8sClient, &wandb, scheme.Scheme)).SetUserInput(userSpec)
+			Expect(err).ToNot(HaveOccurred())
+
+			res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: wandb.Name, Namespace: wandb.Namespace}})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(res).To(Equal(ctrl.Result{RequeueAfter: time.Duration(1 * time.Hour)}))
+		})
+
+		AfterEach(func() {
+			ctx := context.Background()
+			wandb := wandbcomv1.WeightsAndBiases{}
+			err := k8sClient.Get(ctx, types.NamespacedName{Name: "test-release-id", Namespace: "default"}, &wandb)
+			Expect(err).ToNot(HaveOccurred())
+			err = k8sClient.Delete(ctx, &wandb)
+			Expect(err).ToNot(HaveOccurred())
+			err = k8sClient.Delete(ctx, &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "test-release-id-spec-active", Namespace: "default"}})
+			Expect(err).ToNot(HaveOccurred())
+			_, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: wandb.Name, Namespace: wandb.Namespace}})
+			Expect(err).ToNot(HaveOccurred())
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: "test-release-id", Namespace: "default"}, &wandb)
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("Should use the specified _releaseId from UserSpec in the final spec", func() {
+			ctx := context.Background()
+			wandb := wandbcomv1.WeightsAndBiases{}
+			err := k8sClient.Get(ctx, types.NamespacedName{Name: "test-release-id", Namespace: "default"}, &wandb)
+			Expect(err).ToNot(HaveOccurred())
+
+			specManager := state.New(ctx, k8sClient, &wandb, scheme.Scheme, secrets.New(ctx, k8sClient, &wandb, scheme.Scheme))
+			activeSpec, err := specManager.GetActive()
+			Expect(err).ToNot(HaveOccurred())
+			Expect(activeSpec.Values["_releaseId"]).To(Equal("0b901113-8135-48ae-bdaf-6fa82b4b2d28"))
+		})
+	})
 	Describe("Reconcile and Apply", func() {
 
 	})

golangci.yaml

@@ -0,0 +1,3 @@
+linters:
+  enable:
+    - errcheck

main.go

@@ -119,7 +119,7 @@ func main() {
 		Recorder:       mgr.GetEventRecorderFor("weightsandbiases"),
 		Client:         mgr.GetClient(),
 		Scheme:         mgr.GetScheme(),
-		DeployerClient: &deployer.DeployerClient{DeployerChannelUrl: deployerAPI},
+		DeployerClient: &deployer.DeployerClient{DeployerAPI: deployerAPI},
 		Debug:          debug,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "WeightsAndBiases")

pkg/wandb/spec/channel/deployer/deployer.go

@@ -8,30 +8,32 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/wandb/operator/pkg/wandb/spec"
 	"github.com/wandb/operator/pkg/wandb/spec/charts"
 )
 
 const (
-	DeployerAPI = "https://deploy.wandb.ai/api/v1/operator/channel"
+	DeployerAPI            = "https://deploy.wandb.ai"
+	DeployerChannelPath    = "/api/v1/operator/channel"
+	DeployerReleaseAPIPath = "/api/v1/operator/channel/release/:versionId"
 )
 
+type GetSpecOptions struct {
+	License     string
+	ActiveState *spec.Spec
+	ReleaseId   string
+}
+
 //counterfeiter:generate . DeployerInterface
 type DeployerInterface interface {
-	GetSpec(license string, activeState *spec.Spec) (*spec.Spec, error)
+	GetSpec(opts GetSpecOptions) (*spec.Spec, error)
 }
 
 type DeployerClient struct {
-	DeployerChannelUrl string
-}
-
-func (c *DeployerClient) getURL() string {
-	if c.DeployerChannelUrl == "" {
-		c.DeployerChannelUrl = DeployerAPI
-	}
-	return c.DeployerChannelUrl
+	DeployerAPI string
 }
 
 type SpecUnknownChart struct {
@@ -40,10 +42,25 @@ type SpecUnknownChart struct {
 	Chart    interface{}    `json:"chart"`
 }
 
+func (c *DeployerClient) getDeployerURL(opts GetSpecOptions) string {
+	var url string
+	if c.DeployerAPI != "" {
+		url = c.DeployerAPI
+	} else {
+		url = DeployerAPI
+	}
+
+	if opts.ReleaseId != "" {
+		return url + strings.Replace(DeployerReleaseAPIPath, ":versionId", opts.ReleaseId, 1)
+	}
+	return url + DeployerChannelPath
+}
+
 // GetSpec returns the spec for the given license. If the license or an empty
 // string it will pull down the latest stable version.
-func (c *DeployerClient) GetSpec(license string, activeState *spec.Spec) (*spec.Spec, error) {
-	url := c.getURL()
+func (c *DeployerClient) GetSpec(opts GetSpecOptions) (*spec.Spec, error) {
+	url := c.getDeployerURL(opts)
+
 	client := &http.Client{}
 
 	maxRetries := 5
@@ -54,8 +71,8 @@ func (c *DeployerClient) GetSpec(license string, activeState *spec.Spec) (*spec.
 		}
 
 		req.Header.Set("Content-Type", "application/json")
-		if license != "" {
-			req.SetBasicAuth("license", license)
+		if opts.License != "" {
+			req.SetBasicAuth("license", opts.License)
 		}
 
 		resp, err := client.Do(req)

pkg/wandb/spec/channel/deployer/deployer_test.go

@@ -1,11 +1,13 @@
 package deployer
 
 import (
-	"github.com/wandb/operator/pkg/wandb/spec"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
+	"strings"
 	"testing"
+
+	"github.com/wandb/operator/pkg/wandb/spec"
 )
 
 func TestDeployerClient_GetSpec(t *testing.T) {
@@ -64,9 +66,12 @@ func TestDeployerClient_GetSpec(t *testing.T) {
 			server := tt.fields.testServer(tt.args.license)
 			defer server.Close()
 			c := &DeployerClient{
-				DeployerChannelUrl: server.URL,
+				DeployerAPI: server.URL,
 			}
-			got, err := c.GetSpec(tt.args.license, tt.args.activeState)
+			got, err := c.GetSpec(GetSpecOptions{
+				License:     tt.args.license,
+				ActiveState: tt.args.activeState,
+			})
 			if (err != nil) != tt.wantErr {
 				t.Errorf("GetSpec() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -82,26 +87,46 @@ func TestDeployerClient_GetSpec(t *testing.T) {
 	}
 }
 
-func TestDeployerClient_getURL(t *testing.T) {
-	type fields struct {
-		DeployerChannelUrl string
-		HttpClient         *http.Client
-	}
+func TestDeployerClient_getDeployerURL(t *testing.T) {
 	tests := []struct {
-		name   string
-		fields fields
-		want   string
+		name               string
+		deployerChannelUrl string
+		deployerReleaseURL string
+		releaseId          string
+		want               string
 	}{
-		{"No Deployer Channel URL provided", fields{"", nil}, DeployerAPI},
-		{"User Provided Deployer Channel URL", fields{"https://test-url.example.com", nil}, "https://test-url.example.com"},
+		{
+			name:      "No releaseId, default channel URL",
+			releaseId: "",
+			want:      DeployerAPI + DeployerChannelPath,
+		},
+		{
+			name:               "No releaseId, custom channel URL",
+			deployerChannelUrl: "https://custom-channel.example.com",
+			releaseId:          "",
+			want:               "https://custom-channel.example.com" + DeployerChannelPath,
+		},
+		{
+			name:      "With releaseId, default release URL",
+			releaseId: "123",
+			want:      DeployerAPI + strings.Replace(DeployerReleaseAPIPath, ":versionId", "123", 1),
+		},
+		{
+			name:               "With releaseId, custom release URL",
+			deployerChannelUrl: "https://custom-release.example.com",
+			releaseId:          "456",
+			want:               "https://custom-release.example.com/api/v1/operator/channel/release/456",
+		},
 	}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := &DeployerClient{
-				DeployerChannelUrl: tt.fields.DeployerChannelUrl,
+				DeployerAPI: tt.deployerChannelUrl,
 			}
-			if got := c.getURL(); got != tt.want {
-				t.Errorf("getURL() = %v, want %v", got, tt.want)
+			got := c.getDeployerURL(GetSpecOptions{ReleaseId: tt.releaseId})
+			if got != tt.want {
+				t.Errorf("getDeployerURL() = %v, want %v", got, tt.want)
 			}
 		})
 	}