-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: allow to configure the runner name #57
Conversation
Hi @maigl ! Would it be possible to get a short user story for this change? From a provider perspective, instances should be tagged with all the tags you set on the pool, plus controller ID and pool ID. You could add any arbitrary tag you wish. In providers such as OpenStack, this means you can filter servers by tag using something like: openstack seerver list --tag some-specific-tag-assigned-to-pool which would list only instances with that tag. You could potentially specify multiple tags to filter even further. I figured it was easier to categorize and filter resources by tag rather than name. It also fits well with use case like GPU/FPGA enabled pools of runners. Other than that, I see no reason not to make the name of the runner more configurable. It may be worth adding a What do you think? |
Clearly, this feature only helps humans to distinguish runner instances. But still, this feature is more than pure vanity, especially when you have many pools with many many runners. Another example is: In a workflow run you can see:
you see the runner name, but not the labels that where used to pick this runner. In a debugging scenario it would help much to directly see the pool this runner belongs to. |
Sounds good. Do you think it would be worth using something like |
.. I think this could be nice .. however, in our case owner will almost always be 'enterprise'. I have the feeling it would be nice to have the possibility to freely set the name, like a "full name override". |
Fair enough. Will have a proper look over this PR soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small changes, but otherwise looks good!
database/sql/util.go
Outdated
@@ -218,6 +219,9 @@ func (s *sqlDatabase) updatePool(pool Pool, param params.UpdatePoolParams) (para | |||
pool.Image = param.Image | |||
} | |||
|
|||
// no check necessary, we have sane defaults | |||
pool.RunnerPrefix = param.RunnerPrefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned that if we ever add another code path that will use this function and we forget to add proper sane defaults there as well, this will come back to bite us in the behind. I suggest you attach a function to the params.UpdatePoolParams{}
called something like GetRunnerPrefix()
that checks if the prefix is set, and returns a DefaultRunnerPrefix
(let's avoid magic strings) if it's empty.
This way we can be sure that a prefix makes it into the store, and we won't have to care about it being empty when we retrieve an entity and look for a prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example for the bootstrap timeout:
https://github.com/cloudbase/garm/blob/main/params/params.go#L164-L169
Something similar to this could be done for the runner prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am so silly. I said params.UpdatePoolParams{}
should have that function, but in fact this is more useful in params.Pool{}
as that is actually acted upon when creating the actual runner.
Sorry about this. I don't know what I was thinking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow you to just use pool.GetRunnerPrefix()
here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, you could then use pool.GetRunnerPrefix()
, anywhere you call pool.RunnerPrefix
ensuring you'll always get a sane value.
params/params.go
Outdated
@@ -138,6 +138,7 @@ type Tag struct { | |||
|
|||
type Pool struct { | |||
ID string `json:"id"` | |||
RunnerPrefix string `json:"runner_prefix"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run gofmt -w -s
on changed files. I find it useful to set up the IDE/Code editor to do it automatically. I think the vs code golang plugin does this out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all 😄
runner/pool/pool.go
Outdated
name := fmt.Sprintf("garm-%s", uuid.New()) | ||
prefix := pool.RunnerPrefix | ||
if prefix == "" { | ||
prefix = "garm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's define a constant or a variable DefaultRunnerPrefix
. We may need to use it in more than one place and will make our lives easier if we decide to use another default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
runner/pool/pool.go
Outdated
if prefix == "" { | ||
prefix = "garm" | ||
} | ||
name := fmt.Sprintf("%s-%s", prefix, shortid.MustGenerate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the short ID idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. I changed that a little bit, to avoid having '-' and '_' in the short id. However, I'm tempted to add some unicode smileys :)
61ff715
to
dac5667
Compare
Hi, thanks for the review .. and sorry for the late response. I will update the PR soon. |
some remarks .. I noticed that in the garm-cli there are special sub-commands for org pool |
Yes. During the first weeks of garm's existance, I thought it would be useful to have things like: garm-cli repo pool list [repo_id] and: garm-cli repo pool show [repo_ID] [pool_id] But realized that it's a bit cumbersome, and typing less using: garm-cli pool list --repo [repo_id] and: garm-cli pool show [pool_id] # The pool ID is unique is a lot friendlier. To be honest I think we can remove the pool sub-command from orgs and repos. Edit: same goes for the |
Merging as is. Will make that change when I remove the Thanks for the PR! |
This allows to set a individual runner-prefix for each runner pool.
Currently all runner instances are called something like
garm-b55ac285-02bd-46dc-8b6e-c7f2f2e8441a
,no matter how many pools or even garm instances there are. This makes it a little bit tricky to manage larger numbers of runners.
The runner-prefix will be use to prefix name of the runners.
This is especially useful if you want to have multiple runner pools with runners with various capabilities.
Again, this is just an example of how we would imagine such a feature, but some remarks:
Looking forward to hearing your ideas to this.
Michael Kuhnt michael.kuhnt@mercedes-benz.com Mercedes-Benz Tech Innovation GmbH (ProviderInformation)