diff --git a/api/v1/weightsandbiases_conversion_overrides.go b/api/v1/weightsandbiases_conversion_overrides.go index 754ffdba..7ee7ecd0 100644 --- a/api/v1/weightsandbiases_conversion_overrides.go +++ b/api/v1/weightsandbiases_conversion_overrides.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "sort" "strings" @@ -141,19 +142,13 @@ func mapLegacyOverrides(values map[string]interface{}, dst *appsv2.WeightsAndBia return nil } -// mapPerAppLegacyOverrides fails when a version is set but its manifest can't be -// resolved: the manifest decides which values sections are applications, so -// continuing would silently discard every per-application override while -// reporting success. -// -// An absent version is not an error here — v1 routinely left the version to the -// deployer channel, and there is nothing to drop when there is nothing to -// resolve. "A version is required" is enforced on the v2 object by -// validateWandbSpec, where the error names the field. +// mapPerAppLegacyOverrides fails when no version is derivable, or when a +// version's manifest can't be resolved: the manifest decides which values +// sections are applications, so continuing would silently discard every +// per-application override while reporting success. func mapPerAppLegacyOverrides(values map[string]interface{}, version, globalSize string, overrides map[string]appsv2.LegacyOverrides) error { if version == "" { - logger.Info("no version derived from v1 values; skipping per-application legacy overrides") - return nil + return errors.New("manifest version required: no version derived from v1 values (spec.values.app.image.tag or spec.values.api.image.tag)") } apps, err := legacyManifestApps(version) if err != nil { diff --git a/api/v1/weightsandbiases_conversion_overrides_test.go b/api/v1/weightsandbiases_conversion_overrides_test.go index 9c2de740..2f2d94f8 100644 --- a/api/v1/weightsandbiases_conversion_overrides_test.go +++ b/api/v1/weightsandbiases_conversion_overrides_test.go @@ -33,16 +33,32 @@ import ( const testLegacyVersion = "0.83.0-test" -// disableConversionManifestFetch keeps unit tests off the network; tests opt -// in via withConversionManifest*. -func disableConversionManifestFetch() { +// defaultConversionManifest keeps unit tests off the network. It resolves to a +// manifest with no applications, which is all conversion needs to succeed now +// that a derivable version is mandatory: tests that assert per-application +// overrides install their own via withConversionManifest*, and tests that need +// the fetch to fail install an erroring resolver. +func defaultConversionManifest() { SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - return serverManifest.Manifest{}, errors.New("manifest fetch disabled in unit tests") + return serverManifest.Manifest{}, nil }) } +// withFailingConversionManifest makes manifest resolution fail with err and +// returns its call counter. +func withFailingConversionManifest(t *testing.T, err error) *atomic.Int32 { + t.Helper() + var calls atomic.Int32 + SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { + calls.Add(1) + return serverManifest.Manifest{}, err + }) + t.Cleanup(defaultConversionManifest) + return &calls +} + func TestMain(m *testing.M) { - disableConversionManifestFetch() + defaultConversionManifest() os.Exit(m.Run()) } @@ -55,7 +71,7 @@ func withConversionManifest(t *testing.T, apps map[string]serverManifest.Applica calls.Add(1) return serverManifest.Manifest{Applications: apps}, nil }) - t.Cleanup(disableConversionManifestFetch) + t.Cleanup(defaultConversionManifest) return &calls } @@ -71,11 +87,20 @@ func withConversionManifestApps(t *testing.T, names ...string) *atomic.Int32 { } // withVersion adds the app.image.tag mapVersion reads, so per-app extraction -// has a version to resolve the manifest with. +// has a version to resolve the manifest with. Any other keys the fixture put +// under app are preserved. func withVersion(values map[string]interface{}) map[string]interface{} { - values["app"] = map[string]interface{}{ - "image": map[string]interface{}{"tag": testLegacyVersion}, + app, ok := values["app"].(map[string]interface{}) + if !ok { + app = map[string]interface{}{} + } + image, ok := app["image"].(map[string]interface{}) + if !ok { + image = map[string]interface{}{} } + image["tag"] = testLegacyVersion + app["image"] = image + values["app"] = app return values } @@ -90,13 +115,7 @@ func TestConvertTo_LegacyOverridesAbsent(t *testing.T) { } func TestConvertTo_LegacyOverridesGlobalEnvPrecedence(t *testing.T) { - // No version in values: global env must convert without any manifest fetch. - SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - t.Fatal("manifest must not be resolved when no version is derived") - return serverManifest.Manifest{}, nil - }) - t.Cleanup(disableConversionManifestFetch) - + withConversionManifestApps(t) dst := &appsv2.WeightsAndBiases{} src := newV1(map[string]interface{}{ "global": map[string]interface{}{ @@ -387,10 +406,7 @@ func TestConvertTo_LegacyOverridesResourcesDefaultSizeIsSmall(t *testing.T) { } func TestConvertTo_LegacyOverridesManifestUnavailable(t *testing.T) { - SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - return serverManifest.Manifest{}, errors.New("registry unreachable") - }) - t.Cleanup(disableConversionManifestFetch) + withFailingConversionManifest(t, errors.New("registry unreachable")) dst := &appsv2.WeightsAndBiases{} src := newV1(withVersion(map[string]interface{}{ @@ -415,10 +431,7 @@ func TestConvertTo_LegacyOverridesManifestUnavailable(t *testing.T) { // manifest decides what counts as an application, so we can't know there was // nothing to map. func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T) { - SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - return serverManifest.Manifest{}, errors.New("registry unreachable") - }) - t.Cleanup(disableConversionManifestFetch) + withFailingConversionManifest(t, errors.New("registry unreachable")) dst := &appsv2.WeightsAndBiases{} src := newV1(withVersion(map[string]interface{}{ @@ -427,36 +440,65 @@ func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T) require.Error(t, src.ConvertTo(dst)) } -// TestConvertTo_NoVersionSkipsManifestFetch: without a version there is nothing -// to resolve, so conversion proceeds and global env still converts. -func TestConvertTo_NoVersionSkipsManifestFetch(t *testing.T) { - var calls atomic.Int32 - SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - calls.Add(1) - return serverManifest.Manifest{}, errors.New("registry unreachable") - }) - t.Cleanup(disableConversionManifestFetch) +// TestConvertTo_NoVersionFailsBeforeManifestFetch: without a version the +// manifest can't be resolved, and without the manifest there is no way to tell +// which values sections are applications — so the write is rejected rather than +// converted with every per-application override silently dropped. +func TestConvertTo_NoVersionFailsBeforeManifestFetch(t *testing.T) { + calls := withFailingConversionManifest(t, errors.New("registry unreachable")) dst := &appsv2.WeightsAndBiases{} - src := newV1(map[string]interface{}{ + src := newV1NoVersion(map[string]interface{}{ "global": map[string]interface{}{ "env": map[string]interface{}{"HTTP_PROXY": "http://proxy"}, }, + "api": map[string]interface{}{ + "env": map[string]interface{}{"API_VAR": "1"}, + }, }) - require.NoError(t, src.ConvertTo(dst)) - require.Contains(t, dst.Spec.Wandb.LegacyOverrides, appsv2.LegacyOverridesGlobalKey) - require.Equal(t, int32(0), calls.Load(), "no version means no manifest fetch") + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "manifest version required") + require.Equal(t, int32(0), calls.Load(), "the version is checked before any fetch is attempted") +} + +// TestConvertTo_EmptyAppTagFailsLikeAbsentTag: mapVersion treats an empty tag as +// no version at all, so it must be rejected the same way. +func TestConvertTo_EmptyAppTagFailsLikeAbsentTag(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1NoVersion(map[string]interface{}{ + "app": map[string]interface{}{ + "image": map[string]interface{}{"tag": ""}, + }, + }) + + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "manifest version required") +} + +// TestConvertTo_VersionFromApiTagAloneSatisfiesRequirement: the api fallback is +// a real version source, so an app-tag-less CR still converts. +func TestConvertTo_VersionFromApiTagAloneSatisfiesRequirement(t *testing.T) { + withConversionManifestApps(t, "api") + dst := &appsv2.WeightsAndBiases{} + src := newV1NoVersion(map[string]interface{}{ + "api": map[string]interface{}{ + "image": map[string]interface{}{"tag": "0.79.2"}, + "env": map[string]interface{}{"API_VAR": "1"}, + }, + }) + + require.NoError(t, src.ConvertTo(dst)) + require.Equal(t, "0.79.2", dst.Spec.Wandb.Version) + require.Equal(t, []corev1.EnvVar{{Name: "API_VAR", Value: "1"}}, + dst.Spec.Wandb.LegacyOverrides["api"].Env) } func TestConvertTo_LegacyOverridesManifestFailureCooldown(t *testing.T) { // The cooldown keeps repeat conversions from stalling on an unreachable registry. - var calls atomic.Int32 - SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) { - calls.Add(1) - return serverManifest.Manifest{}, errors.New("registry unreachable") - }) - t.Cleanup(disableConversionManifestFetch) + calls := withFailingConversionManifest(t, errors.New("registry unreachable")) for i := 0; i < 3; i++ { dst := &appsv2.WeightsAndBiases{} diff --git a/api/v1/weightsandbiases_conversion_test.go b/api/v1/weightsandbiases_conversion_test.go index a352d62d..33e19e3c 100644 --- a/api/v1/weightsandbiases_conversion_test.go +++ b/api/v1/weightsandbiases_conversion_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -44,9 +45,14 @@ func withConversionReader(t *testing.T, secrets ...*corev1.Secret) { } // activeSpecSecret builds a `-spec-active`-shaped Secret with the -// given values map JSON-encoded into data.values. +// given values map JSON-encoded into data.values. Conversion prefers these +// values over the CR's, so — like newV1 — they get a version unless the fixture +// already supplies one. func activeSpecSecret(t *testing.T, namespace, crName string, values map[string]interface{}) *corev1.Secret { t.Helper() + if values != nil && derivedLegacyVersion(values) == "" { + values = withVersion(values) + } raw, err := json.Marshal(values) require.NoError(t, err) return &corev1.Secret{ @@ -58,7 +64,21 @@ func activeSpecSecret(t *testing.T, namespace, crName string, values map[string] } } +// newV1 builds a convertible v1 CR: conversion rejects values from which no +// version can be derived, so a tag is injected unless the fixture already +// carries one. Tests that exercise the missing-version path use newV1NoVersion. func newV1(values map[string]interface{}) *WeightsAndBiases { + if values == nil { + values = map[string]interface{}{} + } + if derivedLegacyVersion(values) == "" { + values = withVersion(values) + } + return newV1NoVersion(values) +} + +// newV1NoVersion builds the CR from values verbatim, supplying nothing. +func newV1NoVersion(values map[string]interface{}) *WeightsAndBiases { return &WeightsAndBiases{ ObjectMeta: metav1.ObjectMeta{ Name: "wandb", @@ -70,9 +90,22 @@ func newV1(values map[string]interface{}) *WeightsAndBiases { } } +// derivedLegacyVersion mirrors mapVersion: app.image.tag, then api.image.tag. +func derivedLegacyVersion(values map[string]interface{}) string { + for _, section := range []string{"app", "api"} { + tag, found, err := unstructured.NestedString(values, section, "image", "tag") + if err == nil && found && tag != "" { + return tag + } + } + return "" +} + +// TestConvertTo_EmptyValues: absent values short-circuit applyValueMappings +// before the version requirement, so a CR with no values at all still converts. func TestConvertTo_EmptyValues(t *testing.T) { dst := &appsv2.WeightsAndBiases{} - require.NoError(t, newV1(nil).ConvertTo(dst)) + require.NoError(t, newV1NoVersion(nil).ConvertTo(dst)) require.Equal(t, "wandb", dst.Name) require.Empty(t, dst.Spec.Wandb.Hostname) require.Empty(t, dst.Spec.Wandb.License) @@ -80,6 +113,16 @@ func TestConvertTo_EmptyValues(t *testing.T) { require.NotContains(t, dst.Annotations, OIDCPendingAnnotation) } +// TestConvertTo_EmptyValuesMapFailsWithoutVersion: a present-but-empty values map +// does run the mappings, and yields no version, so it is rejected. The contrast +// with TestConvertTo_EmptyValues is the nil short-circuit, not the version. +func TestConvertTo_EmptyValuesMapFailsWithoutVersion(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + err := newV1NoVersion(map[string]interface{}{}).ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "manifest version required") +} + func TestConvertTo_NoGlobalKey(t *testing.T) { dst := &appsv2.WeightsAndBiases{} src := newV1(map[string]interface{}{ @@ -217,13 +260,20 @@ func TestConvertTo_VersionEmptyAppFallsBackToApi(t *testing.T) { require.Equal(t, "0.79.2", dst.Spec.Wandb.Version, "empty app tag should not consume the slot; api fallback applies") } +// TestConvertTo_VersionAbsent: neither app nor api carries an image tag, so no +// version can be derived and the conversion is rejected. The version is what +// selects the server manifest, and without it the per-application overrides in +// the values cannot be mapped. func TestConvertTo_VersionAbsent(t *testing.T) { dst := &appsv2.WeightsAndBiases{} - src := newV1(map[string]interface{}{ + src := newV1NoVersion(map[string]interface{}{ "global": map[string]interface{}{"host": "http://x"}, }) - require.NoError(t, src.ConvertTo(dst)) - require.Empty(t, dst.Spec.Wandb.Version) + err := src.ConvertTo(dst) + require.Error(t, err) + require.Contains(t, err.Error(), "manifest version required") + require.Contains(t, err.Error(), "spec.values.app.image.tag", + "the error should name the fields the deployer can set") } func TestConvertTo_VersionWithoutGlobal(t *testing.T) {