Move some code around #70
Conversation
Move the metrics code into its own package. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
On some providers the default character set used by shortid may lead to errors when creating runners, due to the fact that underscores are not allowed in their names. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
|
@maigl Moved the metrics stuff in a dedicated package. Also made some really small changes here and there. Functionally it should be the same. Let me know if you see any issues with any of this. |
|
@maigl apropos, the |
util/util.go
Outdated
| newID = strings.Replace(newID, "_", randomCharacter(), -1) | ||
| newID = strings.Replace(newID, "-", randomCharacter(), -1) |
There was a problem hiding this comment.
I'm no expert here. But I assume that this might compromise the uniqueness of the ids?! You can define the alphabet within the shortid lib, but if you remove '-' and '_', but it what other chars instead.
https://github.com/teris-io/shortid/blob/master/shortid.go#L85
The algorithm seems to always want 64 distinct chars. But I thought replacing '_' and '-' by '%' and '$' is not much better .. we could use some unicode 😄 here or kanjis
I really would want to avoid some non-unique issue here.
There was a problem hiding this comment.
I really would want to avoid some non-unique issue here.
indeed. It erred out on OpenStack when the name included an underscore 😄. The need for 64 unique characters leaves us with few options. We can leave the hyphen and maybe add a + ? Something that has a lowest chance of failing anywhere garm may be used. Will have to look at the other clouds and what limitations there are in regards to names.
There was a problem hiding this comment.
I'll remove this hack as it made my eye twitch as well when I wrote it.
There was a problem hiding this comment.
Funny.. my openstack is fine with '_'
There was a problem hiding this comment.
ohh wait. My bad. The error was on LXD:
Jan 27 14:18:20 garm garm[11696]: 2023/01/27 14:18:20 failed to add instance to provider: creating instance: creating instance: creating instance: Invalid instance name: Name can only contain alphanumeric and hyphen charactersThe same thing seems to be true for Azure:
https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules (underscores are not allowed)
From the Azure docs above, in regards to VirtualMachines:
Can't use spaces, control characters, or these characters:
~ ! @ # $ % ^ & * ( ) = + _ [ ] { } \ | ; : . ' " , < > / ?
Windows VMs can't include period or end with hyphen.
Linux VMs can't end with period or hyphen.
Which is depressing. Need to find a way to get around this.
There was a problem hiding this comment.
but base 64 contains '/' and '+' and also '=' for padding at the end.
I also like the shortid though, 😄
There was a problem hiding this comment.
base 62 does not 😄 should be alphanumeric (including upper/lower case)
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks like this:
There was a problem hiding this comment.
okay .. never heard of that ;)
sgtm
metrics/metrics.go
Outdated
| c.mux.Lock() | ||
| defer c.mux.Unlock() | ||
| c.cachedControllerInfo = controllerInfo |
There was a problem hiding this comment.
Why is this cached here again? This will only help if runner.GetControllerInfo fails but did work some rounds before, right ?
There was a problem hiding this comment.
The os.Hostname() call can fail, which means we'll end up with no hostname when it does. At least for that call. The cache is to fall back on previous value. What I dislike is that I had to duplicate this in 2 places. I Think I'll add a retry strategy instead and remove the cache.
There was a problem hiding this comment.
I'd still like to know when something fails and react accordingly, than to ignore the error.
There was a problem hiding this comment.
Removed the caches and just added a retry in GetControllerInfo(). If it errs there, we just surface that error further along and log it.
gabriel-samfira
force-pushed
the
move-some-code-around
branch
from
965e154
to
f05e69a
Compare
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
gabriel-samfira
force-pushed
the
move-some-code-around
branch
from
f05e69a
to
8d17498
Compare
This change is just a couple of small fixes and moving some code around.