Might be worth using atomic.Value for this: https://pkg.go.dev/go.uber.org/atomic#Value

Would t.TempDir work here? That would automatically clean up the dir when the test exits. (We should avoid ioutil package for new code, all the methods now have equivalents elsewhere).

Rather than doing the token stuff here, what do you think about pulling it out in to the Pause and Resume request structs? They're already abstracting how the Handler should contact the endpoint, so it seems odd (to me) for Handler to have the token code rather than them (I'm imagining something like a type concurrencyStateRequest struct { token atomic.Value } struct with Pause and Resume functions).

It did cross my mind, though I was leaning towards having the Pause/Resume functions grab the refreshed token when called, rather than running a goroutine in the handler to keep it refreshed. I had a little nagging worry that getting the token in the pause/resume functions might end up grabbing the wrong value vs. getting it in the handler (though to be fair it was a fleeting thought and I didn't really pursue it too far). That said, let me give it another go.

(Figures that I juuuuust dropped the request struct from the previous PR 😄 )

@julz before I go too far down the refactor train, would something like this be along these lines of what you're thinking?

type ConcurrencyEndpoint struct {
	endpoint string
	token atomic.Value
}

func (c ConcurrencyEndpoint) Pause() error { c.Request("pause") }

func (c ConcurrencyEndpoint) Resume() error { c.Request("resume") }

func (c ConcurrencyEndpoint) Request(request string) func() error { /* send request */ }

Turns out the end of the line wasn't all that far away... got this added in the last batch of commits

these consts having to be exported and shared between pkg/deploy and pkg/queue are a bit sad, wonder if there's a way we could avoid that. What if we got the token path from an env variable? That seems a bit more decoupled and maybe a bit cleaner (this is how we used to handle a similar thing in the past where we looked for some info via the downward api).

bump ^

FWIW, exporting/sharing constant names seems to be a fairly common pattern to pass values to pkg/deploy (queue.Name and queue.RequestQueueDrainPath are passed this way, as well as several from networking). Would it make sense to convert them all to environmental variables (all meaning the QP ones)? If so, perhaps that would be better done in a separate PR.

If that's the route we want to go, for now I could either leave the CE constants where they are or move them to pkg/queue/constants.go

I'm not really convinced queue.Name and queue.RequestQueueDrainPath (or the port related constants in networking) are a very good parallel here, since they're properties of the queue, not properties of how the queue is deployed. The main issue is that we're exporting three separate related constants, which makes the api of pkg/queue more complicated (and coupled) than it needs to be. If instead we passed the mounted location of the token, this would be a fair bit simpler and cleaner.

FWIW this is what we did both times we previously did a feature involving mounting a volume in QP (1, 2), and it seemed quite nice that way.

Fair enough, was just checking if we should also update the other ones.

done

I think we can use os.WriteFile here and drop createTempTokenFile.

I think I'd convert this in to a NewConcurrencyEndpoint constructor so this is generally useful outside the test (and so consumers of the struct can't get themselves in trouble using it before the token has been stored).

this seems like we're refreshing the token on every request rather than on a timer, but if so we don't really need the atomic, and RefreshToken doesn't need to be exported? Maybe worth converting this to something like:

ce := NewConcurrencyEndpoint(endpoint, tokenPath)
go func() {
  for range time.NewTicker(1 * time.Minute).C {
    ce.RefreshToken()
  }
}()

(which would also let us add a test for RefreshToken, which would be nice).

bump ^

I think I'd be tempted to do the Join here so we could drop this constant (just to avoid having 3 package-global constants for this), but maybe we can do that up in a follow-up / next time we touch this (if you agree)

works for me