Fix scale back to 0 with 300 requests scenario

[samos123]

Fixes #73 and includes pr #70 and others from Alex

• Remove compareScales function
• Refactor SetDesiredScale that is simpler to understand by storing lastSuccessfulScale time
• Improve test coverage for scaler.go

Co-author: @alpe

[alpe]

Good refactoring and simpler to read. I added some comments as I think the logic has changed for scale down. This is not necessarily bad but should be revisited from a product view.

[alpe]

Good to exclude them. I was wondering about the log already

personal preference: do we need this log output? This can create a lot of noise?

nit: here and others: prefer %q or %s in strings

[samos123]

I think we can remove this log once we remove this if statement. The logs are confusing without a log saying that the deployment is ignored. The reason being that earlier in the log it shows aggregating stats of e.g. kubernetes which might make people think it's active on the kubernetes service.

[alpe]

🤔 the lastScaleDown value is set in the constructor. This ensures the scale down delay is taken into account on a node startup and right after the last successful scale down.
There is no delay if a running node becomes leader or if the last scale down is some time ago. A scale down call would execute immediately as the condition is always true.
With the old logic, the first call to scale down triggered a timer. I order to achieve the same, we can have an unset lastScaleDown time and set it on first call to scale down. You can check with s.lastScaleDown.IsZero() for unset.
It should be cleared (unset) on

• desiredScale >= currentScale
• successful scale down
• loosing leader role
• Probably with AtLeastOne()

[nstogner]

Here is my code I was working on in parallel:

	if s.currentScale < s.desiredScale {
		// Scale up immediately.
		go s.scaleFunc(s.desiredScale, false)
		s.desiredScaleDownStart = time.Time{}
	} else if s.currentScale == s.desiredScale {
		s.desiredScaleDownStart = time.Time{}
	} else {
		if s.desiredScaleDownStart.IsZero() {
			s.desiredScaleDownStart = time.Now()
		} else if time.Since(s.desiredScaleDownStart) >= s.scaleDownDelay {
			go s.scaleFunc(s.desiredScale, false)
		}
	}

[samos123]

Thanks for taking the time to do a deeper analysis on how it differs. I agree that setting it to zero value is cleaner and have fixed it.

@nstogner regarding your implementation, it's important that we always check and log the error of s.scaleFunc because otherwise it will be hard to debug if there is a scaling issue. Was there any reason we weren't originally doing error checks? and any reason why your implementation does not? Just making sure I'm not doing something useless with the error checking.

[nstogner]

What scenario leads lingo to fall into this state?

[samos123]

Lingo right now has autoscaler active on all deployments because ActiceRequests returns the active requests for all deployments. This if statement can be removed once we fix #59 and use Pod endpoints and only add endpoints of Deployments that have a lingo model annotation.

[samos123]

You can verify this by running Lingo and checking the logs. Note I added a TODO item that this should eventually get removed but I don't want to do this as part of this PR.

[samos123]

To answer your question, this happens in all scenarios.

[nstogner]

No need for local var, clearer to do s.desiredScale = s.applyMinMax(n)

[samos123]

but s.applyMinMax will call Lock() so that's why I did this. Is that not needed even when it also calls Lock()?

[samos123]

I'm pretty sure this would result in a deadlock unless I use the local var, see code example here which simulates what would happen if I were to call s.desiredCale = s.applyMinMax(n) within the Lock()

func main() {
	mtx := sync.Mutex{}
	mtx.Lock()
	fmt.Println("Locked")
	mtx.Lock()
	fmt.Println("Double locked")
}

[nstogner]

You can call it before the Lock()

[samos123]

but I shouldn't modify s.desiredScale before calling Lock()?

[samos123]

after quick chat, I fixed this by removing the lock in applyMinMax and moving the call to applyMinMax into the lock itself.

[nstogner]

Here is my code I was working on in parallel:

	if s.currentScale < s.desiredScale {
		// Scale up immediately.
		go s.scaleFunc(s.desiredScale, false)
		s.desiredScaleDownStart = time.Time{}
	} else if s.currentScale == s.desiredScale {
		s.desiredScaleDownStart = time.Time{}
	} else {
		if s.desiredScaleDownStart.IsZero() {
			s.desiredScaleDownStart = time.Now()
		} else if time.Since(s.desiredScaleDownStart) >= s.scaleDownDelay {
			go s.scaleFunc(s.desiredScale, false)
		}
	}

[nstogner]

LGTM - added one small suggestion in tests

[nstogner]

You could also use require.True(t, scaleFuncCalled) - https://pkg.go.dev/github.com/stretchr/testify/require#True