From e4e8b3f1e053833be0f4cc10afdbb6cdfc967276 Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Fri, 10 Jul 2020 12:37:14 -0400 Subject: [PATCH] engine: fix a bug where resource_deps on non-workloads didn't work correctly (#3574) --- .../engine/buildcontrol/build_control_test.go | 82 +++++++++++++++++-- internal/engine/buildcontroller_test.go | 27 ++++++ internal/engine/exit/controller_test.go | 14 ++-- .../engine/portforward/controller_test.go | 14 ++-- internal/engine/upper.go | 19 +++-- internal/store/engine_state_test.go | 10 ++- internal/store/runtime_state.go | 24 ++++-- internal/testutils/manifestutils/manifest.go | 2 +- 8 files changed, 153 insertions(+), 39 deletions(-) diff --git a/internal/engine/buildcontrol/build_control_test.go b/internal/engine/buildcontrol/build_control_test.go index 7dba9ae8df..0c4d048ccf 100644 --- a/internal/engine/buildcontrol/build_control_test.go +++ b/internal/engine/buildcontrol/build_control_test.go @@ -61,6 +61,55 @@ func TestCurrentlyBuildingUncategorizedDisablesOtherK8sTargets(t *testing.T) { f.assertNoTargetNextToBuild() } +func TestK8sDependsOnLocal(t *testing.T) { + f := newTestFixture(t) + defer f.TearDown() + + k8s1 := f.upsertK8sManifest("k8s1", withResourceDeps("local1")) + k8s2 := f.upsertK8sManifest("k8s2") + local1 := f.upsertLocalManifest("local1") + + f.assertNextTargetToBuild("local1") + + local1.State.AddCompletedBuild(model.BuildRecord{ + StartTime: time.Now(), + FinishTime: time.Now(), + }) + local1.State.RuntimeState = store.LocalRuntimeState{HasSucceededAtLeastOnce: true} + + f.assertNextTargetToBuild("k8s1") + k8s1.State.CurrentBuild = model.BuildRecord{StartTime: time.Now()} + f.assertNextTargetToBuild("k8s2") + + _ = k8s2 +} + +func TestLocalDependsOnNonWorkloadK8s(t *testing.T) { + f := newTestFixture(t) + defer f.TearDown() + + local1 := f.upsertLocalManifest("local1", withResourceDeps("k8s1")) + k8s1 := f.upsertK8sManifest("k8s1", withK8sNonWorkload()) + k8s2 := f.upsertK8sManifest("k8s2", withK8sNonWorkload()) + + f.assertNextTargetToBuild("k8s1") + + k8s1.State.AddCompletedBuild(model.BuildRecord{ + StartTime: time.Now(), + FinishTime: time.Now(), + }) + k8s1.State.RuntimeState = store.K8sRuntimeState{NonWorkload: true, HasEverDeployedSuccessfully: true} + + f.assertNextTargetToBuild("local1") + local1.State.AddCompletedBuild(model.BuildRecord{ + StartTime: time.Now(), + FinishTime: time.Now(), + }) + f.assertNextTargetToBuild("k8s2") + + _ = k8s2 +} + func TestCurrentlyBuildingLocalResourceDisablesK8sScheduling(t *testing.T) { f := newTestFixture(t) defer f.TearDown() @@ -173,16 +222,20 @@ func (f *testFixture) upsertManifest(m model.Manifest) *store.ManifestTarget { return mt } -func (f *testFixture) upsertK8sManifest(name model.ManifestName) *store.ManifestTarget { - return f.upsertManifest(manifestbuilder.New(f, name). - WithK8sYAML(testyaml.SanchoYAML). - Build()) +func (f *testFixture) upsertK8sManifest(name model.ManifestName, opts ...manifestOption) *store.ManifestTarget { + b := manifestbuilder.New(f, name) + for _, o := range opts { + b = o(b) + } + return f.upsertManifest(b.WithK8sYAML(testyaml.SanchoYAML).Build()) } -func (f *testFixture) upsertLocalManifest(name model.ManifestName) *store.ManifestTarget { - return f.upsertManifest(manifestbuilder.New(f, name). - WithLocalResource(fmt.Sprintf("exec-%s", name), nil). - Build()) +func (f *testFixture) upsertLocalManifest(name model.ManifestName, opts ...manifestOption) *store.ManifestTarget { + b := manifestbuilder.New(f, name) + for _, o := range opts { + b = o(b) + } + return f.upsertManifest(b.WithLocalResource(fmt.Sprintf("exec-%s", name), nil).Build()) } func (f *testFixture) manifestNeedingCrashRebuild() *store.ManifestTarget { @@ -199,3 +252,16 @@ func (f *testFixture) manifestNeedingCrashRebuild() *store.ManifestTarget { mt.State.NeedsRebuildFromCrash = true return mt } + +type manifestOption func(manifestbuilder.ManifestBuilder) manifestbuilder.ManifestBuilder + +func withResourceDeps(deps ...string) manifestOption { + return manifestOption(func(m manifestbuilder.ManifestBuilder) manifestbuilder.ManifestBuilder { + return m.WithResourceDeps(deps...) + }) +} +func withK8sNonWorkload() manifestOption { + return manifestOption(func(m manifestbuilder.ManifestBuilder) manifestbuilder.ManifestBuilder { + return m.WithK8sNonWorkload() + }) +} diff --git a/internal/engine/buildcontroller_test.go b/internal/engine/buildcontroller_test.go index c407c0270b..94dd6e54d3 100644 --- a/internal/engine/buildcontroller_test.go +++ b/internal/engine/buildcontroller_test.go @@ -1368,6 +1368,33 @@ func TestManifestsWithTwoCommonAncestors(t *testing.T) { f.assertAllBuildsConsumed() } +func TestLocalDependsOnNonWorkloadK8s(t *testing.T) { + f := newTestFixture(t) + defer f.TearDown() + + local1 := manifestbuilder.New(f, "local"). + WithLocalResource("exec-local", nil). + WithResourceDeps("k8s1"). + Build() + k8s1 := manifestbuilder.New(f, "k8s1"). + WithK8sYAML(testyaml.SanchoYAML). + WithK8sNonWorkload(). + Build() + f.Start([]model.Manifest{local1, k8s1}) + + f.waitForCompletedBuildCount(2) + + call := f.nextCall("k8s1 build") + assert.Equal(t, k8s1.K8sTarget(), call.k8s()) + + call = f.nextCall("local build") + assert.Equal(t, local1.LocalTarget(), call.local()) + + err := f.Stop() + assert.NoError(t, err) + f.assertAllBuildsConsumed() +} + func TestManifestsWithCommonAncestorAndTrigger(t *testing.T) { f := newTestFixture(t) m1, m2 := NewManifestsWithCommonAncestor(f) diff --git a/internal/engine/exit/controller_test.go b/internal/engine/exit/controller_test.go index b965a2d959..3ab21d243d 100644 --- a/internal/engine/exit/controller_test.go +++ b/internal/engine/exit/controller_test.go @@ -106,7 +106,7 @@ func TestExitControlCIFirstRuntimeFailure(t *testing.T) { f.store.WithState(func(state *store.EngineState) { mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, store.Pod{ + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{ PodID: "pod-a", Status: "ErrImagePull", Containers: []store.Container{ @@ -142,7 +142,7 @@ func TestExitControlCISuccess(t *testing.T) { StartTime: time.Now(), FinishTime: time.Now(), }) - state.ManifestTargets["fe"].State.RuntimeState = store.NewK8sRuntimeState(m, readyPod("pod-a")) + state.ManifestTargets["fe"].State.RuntimeState = store.NewK8sRuntimeStateWithPods(m, readyPod("pod-a")) }) f.c.OnChange(f.ctx, f.store) @@ -150,7 +150,7 @@ func TestExitControlCISuccess(t *testing.T) { f.store.WithState(func(state *store.EngineState) { mt := state.ManifestTargets["fe2"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, readyPod("pod-b")) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, readyPod("pod-b")) }) // Verify that if one pod fails with an error, it fails immediately. @@ -171,7 +171,7 @@ func TestExitControlCIJobSuccess(t *testing.T) { StartTime: time.Now(), FinishTime: time.Now(), }) - state.ManifestTargets["fe"].State.RuntimeState = store.NewK8sRuntimeState(m, readyPod("pod-a")) + state.ManifestTargets["fe"].State.RuntimeState = store.NewK8sRuntimeStateWithPods(m, readyPod("pod-a")) }) f.c.OnChange(f.ctx, f.store) @@ -179,7 +179,7 @@ func TestExitControlCIJobSuccess(t *testing.T) { f.store.WithState(func(state *store.EngineState) { mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, successPod("pod-a")) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, successPod("pod-a")) }) // Verify that if one pod fails with an error, it fails immediately. @@ -201,7 +201,7 @@ func TestExitControlCIDontBlockOnAutoInitFalse(t *testing.T) { StartTime: time.Now(), FinishTime: time.Now(), }) - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, readyPod("pod-a")) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, readyPod("pod-a")) manifestAutoInitFalse := manifestbuilder.New(f, "local").WithLocalResource("echo hi", nil). WithTriggerMode(model.TriggerModeManualIncludingInitial).Build() @@ -213,7 +213,7 @@ func TestExitControlCIDontBlockOnAutoInitFalse(t *testing.T) { f.store.WithState(func(state *store.EngineState) { mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, successPod("pod-a")) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, successPod("pod-a")) }) // Verify that if one pod fails with an error, it fails immediately. diff --git a/internal/engine/portforward/controller_test.go b/internal/engine/portforward/controller_test.go index 24361489e7..eb70693132 100644 --- a/internal/engine/portforward/controller_test.go +++ b/internal/engine/portforward/controller_test.go @@ -42,7 +42,7 @@ func TestPortForward(t *testing.T) { state = f.st.LockMutableStateForTesting() mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id", Phase: v1.PodRunning}) f.st.UnlockMutableState() @@ -53,7 +53,7 @@ func TestPortForward(t *testing.T) { state = f.st.LockMutableStateForTesting() mt = state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id2", Phase: v1.PodRunning}) f.st.UnlockMutableState() @@ -63,7 +63,7 @@ func TestPortForward(t *testing.T) { state = f.st.LockMutableStateForTesting() mt = state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, store.Pod{PodID: "pod-id2", Phase: v1.PodPending}) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id2", Phase: v1.PodPending}) f.st.UnlockMutableState() f.onChange() @@ -91,7 +91,7 @@ func TestPortForwardAutoDiscovery(t *testing.T) { state.UpsertManifestTarget(store.NewManifestTarget(m)) mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id", Phase: v1.PodRunning}) f.st.UnlockMutableState() @@ -126,7 +126,7 @@ func TestPortForwardAutoDiscovery2(t *testing.T) { state.UpsertManifestTarget(store.NewManifestTarget(m)) mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, store.Pod{ + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{ PodID: "pod-id", Phase: v1.PodRunning, Containers: []store.Container{ @@ -155,7 +155,7 @@ func TestPortForwardChangePort(t *testing.T) { }) state.UpsertManifestTarget(store.NewManifestTarget(m)) mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, store.Pod{PodID: "pod-id", Phase: v1.PodRunning}) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id", Phase: v1.PodRunning}) f.st.UnlockMutableState() f.onChange() @@ -190,7 +190,7 @@ func TestPortForwardRestart(t *testing.T) { }) state.UpsertManifestTarget(store.NewManifestTarget(m)) mt := state.ManifestTargets["fe"] - mt.State.RuntimeState = store.NewK8sRuntimeState(mt.Manifest, + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(mt.Manifest, store.Pod{PodID: "pod-id", Phase: v1.PodRunning}) f.st.UnlockMutableState() diff --git a/internal/engine/upper.go b/internal/engine/upper.go index 726e06c6a4..c324abf479 100644 --- a/internal/engine/upper.go +++ b/internal/engine/upper.go @@ -393,19 +393,22 @@ func handleBuildCompleted(ctx context.Context, engineState *store.EngineState, c manifest := mt.Manifest if manifest.IsK8s() { + state := ms.K8sRuntimeState() deployedUIDSet := cb.Result.DeployedUIDSet() if len(deployedUIDSet) > 0 { - state := ms.K8sRuntimeState() state.DeployedUIDSet = deployedUIDSet - ms.RuntimeState = state } deployedPodTemplateSpecHashSet := cb.Result.DeployedPodTemplateSpecHashes() if len(deployedPodTemplateSpecHashSet) > 0 { - state := ms.K8sRuntimeState() state.DeployedPodTemplateSpecHashSet = deployedPodTemplateSpecHashSet - ms.RuntimeState = state } + + if err == nil { + state.HasEverDeployedSuccessfully = true + } + + ms.RuntimeState = state } if mt.Manifest.IsDC() { @@ -437,10 +440,12 @@ func handleBuildCompleted(ctx context.Context, engineState *store.EngineState, c } if mt.Manifest.IsLocal() { - ms.RuntimeState = store.LocalRuntimeState{ - Status: model.RuntimeStatusNotApplicable, - HasSucceededAtLeastOnce: err == nil, + lrs := ms.LocalRuntimeState() + lrs.Status = model.RuntimeStatusNotApplicable + if err == nil { + lrs.HasSucceededAtLeastOnce = true } + ms.RuntimeState = lrs } } diff --git a/internal/store/engine_state_test.go b/internal/store/engine_state_test.go index aa076cdbca..8b523827c6 100644 --- a/internal/store/engine_state_test.go +++ b/internal/store/engine_state_test.go @@ -81,8 +81,12 @@ func TestRuntimeStateNonWorkload(t *testing.T) { WithK8sYAML(testyaml.SecretYaml). Build() state := newState([]model.Manifest{m}) - assert.Equal(t, model.RuntimeStatusOK, - state.ManifestTargets[m.Name].State.K8sRuntimeState().RuntimeStatus()) + runtimeState := state.ManifestTargets[m.Name].State.K8sRuntimeState() + assert.Equal(t, model.RuntimeStatusPending, runtimeState.RuntimeStatus()) + + runtimeState.HasEverDeployedSuccessfully = true + + assert.Equal(t, model.RuntimeStatusOK, runtimeState.RuntimeStatus()) } func TestStateToViewUnresourcedYAMLManifest(t *testing.T) { @@ -126,7 +130,7 @@ func TestMostRecentPod(t *testing.T) { podB := Pod{PodID: "pod-b", StartedAt: time.Now().Add(time.Minute)} podC := Pod{PodID: "pod-c", StartedAt: time.Now().Add(-time.Minute)} m := model.Manifest{Name: "fe"} - podSet := NewK8sRuntimeState(m, podA, podB, podC) + podSet := NewK8sRuntimeStateWithPods(m, podA, podB, podC) assert.Equal(t, "pod-b", podSet.MostRecentPod().PodID.String()) } diff --git a/internal/store/runtime_state.go b/internal/store/runtime_state.go index 3d5770d75d..d008dbddeb 100644 --- a/internal/store/runtime_state.go +++ b/internal/store/runtime_state.go @@ -72,7 +72,8 @@ type K8sRuntimeState struct { DeployedUIDSet UIDSet // for the most recent successful deploy DeployedPodTemplateSpecHashSet PodTemplateSpecHashSet // for the most recent successful deploy - LastReadyOrSucceededTime time.Time + LastReadyOrSucceededTime time.Time + HasEverDeployedSuccessfully bool NonWorkload bool } @@ -81,16 +82,20 @@ func (K8sRuntimeState) RuntimeState() {} var _ RuntimeState = K8sRuntimeState{} -func NewK8sRuntimeState(m model.Manifest, pods ...Pod) K8sRuntimeState { - - podMap := make(map[k8s.PodID]*Pod, len(pods)) +func NewK8sRuntimeStateWithPods(m model.Manifest, pods ...Pod) K8sRuntimeState { + state := NewK8sRuntimeState(m) for _, pod := range pods { p := pod - podMap[p.PodID] = &p + state.Pods[p.PodID] = &p } + state.HasEverDeployedSuccessfully = len(pods) > 0 + return state +} + +func NewK8sRuntimeState(m model.Manifest) K8sRuntimeState { return K8sRuntimeState{ NonWorkload: m.K8sTarget().NonWorkload, - Pods: podMap, + Pods: make(map[k8s.PodID]*Pod), LBs: make(map[k8s.ServiceName]*url.URL), DeployedUIDSet: NewUIDSet(), DeployedPodTemplateSpecHashSet: NewPodTemplateSpecHashSet(), @@ -107,6 +112,10 @@ func (s K8sRuntimeState) RuntimeStatusError() error { } func (s K8sRuntimeState) RuntimeStatus() model.RuntimeStatus { + if !s.HasEverDeployedSuccessfully { + return model.RuntimeStatusPending + } + if s.NonWorkload { return model.RuntimeStatusOK } @@ -137,6 +146,9 @@ func (s K8sRuntimeState) RuntimeStatus() model.RuntimeStatus { } func (s K8sRuntimeState) HasEverBeenReadyOrSucceeded() bool { + if !s.HasEverDeployedSuccessfully { + return false + } if s.NonWorkload { return true } diff --git a/internal/testutils/manifestutils/manifest.go b/internal/testutils/manifestutils/manifest.go index eb0f8d0101..53a2566326 100644 --- a/internal/testutils/manifestutils/manifest.go +++ b/internal/testutils/manifestutils/manifest.go @@ -7,6 +7,6 @@ import ( func NewManifestTargetWithPod(m model.Manifest, pod store.Pod) *store.ManifestTarget { mt := store.NewManifestTarget(m) - mt.State.RuntimeState = store.NewK8sRuntimeState(m, pod) + mt.State.RuntimeState = store.NewK8sRuntimeStateWithPods(m, pod) return mt }