Skip to content

Reset API intents on every dev cycle to avoid queueing #5636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/skaffold/runner/dev.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,15 @@ var (
)

func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer, logger *kubernetes.LogAggregator, forwarderManager portforward.Forwarder) error {
// never queue intents from user, even if they're not used
defer r.intents.Reset()

if r.changeSet.needsReload {
return ErrorConfigurationChanged
}

buildIntent, syncIntent, deployIntent := r.intents.GetIntents()
logrus.Tracef("dev intents: build %t, sync %t, deploy %t\n", buildIntent, syncIntent, deployIntent)
Copy link
Contributor

@gsquared94 gsquared94 Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we notify the user either in the API response (but that'd require making it synchronous) or a string output that we're skipping this request since nothing has changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would require a bit more plumbing, since in the runner we're just asking whether the intent's gate is open, not whether the user requested to open it. i think this would be a good improvement, but it should probably happen outside the scope of this PR.

needsSync := syncIntent && len(r.changeSet.needsResync) > 0
needsBuild := buildIntent && len(r.changeSet.needsRebuild) > 0
needsTest := r.changeSet.needsRetest
Expand Down
48 changes: 48 additions & 0 deletions pkg/skaffold/runner/dev_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ func (t *TestMonitor) Register(deps func() ([]string, error), onChange func(file
}

func (t *TestMonitor) Run(bool) error {
if t.testBench.intentTrigger {
return nil
}

evt := t.events[t.testBench.currentCycle]

for _, file := range evt.Modified {
Expand Down Expand Up @@ -428,6 +432,7 @@ func TestDevAutoTriggers(t *testing.T) {
expectedActions []Actions
autoTriggers triggerState // the state of auto triggers
singleTriggers triggerState // the state of single intent triggers at the end of dev loop
userIntents []func(i *intents)
}{
{
description: "build on; sync on; deploy on",
Expand Down Expand Up @@ -491,6 +496,46 @@ func TestDevAutoTriggers(t *testing.T) {
singleTriggers: triggerState{false, false, true},
expectedActions: []Actions{{}, {}},
},
{
description: "build off; sync off; deploy off; user requests build, but no change so intent is discarded",
watchEvents: []filemon.Events{},
autoTriggers: triggerState{false, false, false},
singleTriggers: triggerState{false, false, false},
expectedActions: []Actions{},
userIntents: []func(i *intents){
func(i *intents) {
i.setBuild(true)
},
},
},
{
description: "build off; sync off; deploy off; user requests build, and then sync, but no change so both intents are discarded",
watchEvents: []filemon.Events{},
autoTriggers: triggerState{false, false, false},
singleTriggers: triggerState{false, false, false},
expectedActions: []Actions{},
userIntents: []func(i *intents){
func(i *intents) {
i.setBuild(true)
i.setSync(true)
},
},
},
{
description: "build off; sync off; deploy off; user requests build, and then sync, but no change so both intents are discarded",
watchEvents: []filemon.Events{},
autoTriggers: triggerState{false, false, false},
singleTriggers: triggerState{false, false, false},
expectedActions: []Actions{},
userIntents: []func(i *intents){
func(i *intents) {
i.setBuild(true)
},
func(i *intents) {
i.setSync(true)
},
},
},
}
// first build-test-deploy sequence always happens
firstAction := Actions{
Expand All @@ -506,6 +551,7 @@ func TestDevAutoTriggers(t *testing.T) {
t.Override(&sync.WorkingDir, func(string, docker.Config) (string, error) { return "/", nil })
testBench := &TestBench{}
testBench.cycles = len(test.watchEvents)
testBench.userIntents = test.userIntents
artifacts := []*latest.Artifact{
{
ImageName: "img1",
Expand All @@ -522,6 +568,8 @@ func TestDevAutoTriggers(t *testing.T) {
testBench: testBench,
}, artifacts, &test.autoTriggers)

testBench.intents = runner.intents

err := runner.Dev(context.Background(), ioutil.Discard, artifacts)

t.CheckNoError(err)
Expand Down
8 changes: 8 additions & 0 deletions pkg/skaffold/runner/intent.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ func newIntents(autoBuild, autoSync, autoDeploy bool) *intents {
return i
}

func (i *intents) Reset() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about reset since all the other methods are also unexported and the only use is in the current package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

i.lock.Lock()
i.build = i.autoBuild
i.sync = i.autoSync
i.deploy = i.autoDeploy
i.lock.Unlock()
}

func (i *intents) resetBuild() {
i.lock.Lock()
i.build = i.autoBuild
Expand Down
23 changes: 18 additions & 5 deletions pkg/skaffold/runner/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,14 @@ type Actions struct {
}

type TestBench struct {
buildErrors []error
syncErrors []error
testErrors []error
deployErrors []error
namespaces []string
buildErrors []error
syncErrors []error
testErrors []error
deployErrors []error
namespaces []string
userIntents []func(*intents)
intents *intents
intentTrigger bool

devLoop func(context.Context, io.Writer, func() error) error
firstMonitor func(bool) error
Expand Down Expand Up @@ -178,6 +181,16 @@ func (t *TestBench) WatchForChanges(ctx context.Context, out io.Writer, doDev fu
if err := t.firstMonitor(true); err != nil {
return err
}

t.intentTrigger = true
for _, intent := range t.userIntents {
intent(t.intents)
if err := t.devLoop(ctx, out, doDev); err != nil {
return err
}
}

t.intentTrigger = false
for i := 0; i < t.cycles; i++ {
t.enterNewCycle()
t.currentCycle = i
Expand Down