feat: allow config server.connections config to poll multiple Forgejo servers simultaneously #1380
No reviewers
aahlenst
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1380
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "mfenniak/forgejo-runner:multi-connection"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds support for multiple server connections via config file.
Example:
Both
labelsandfetch_intervalcan optionally be provided on a connection. If absent, they'll default torunner.labelsandrunner.fetch_interval(with a default of 2s, and a minimum of 30s for codeberg.org).This is a fully backwards compatible change. If no config changes are made, then the contents of the runner registration file (
.runner) file will be used to populate one server connection. However, if multiple servers are desired, then.runnermust be removed and migrated to the new config file section -- bothserver.connectionsand.runnercannot co-exist.Future items that are planned for this effort are documented in forgejo/forgejo-actions-feature-requests#88 (comment), and include new registration tools in Forgejo to make this capability easier to use.
server.connectionsconfig to poll multiple Forgejo servers simultaneouslyWIP: feat: useto WIP: feat: useserver.connectionsconfig to poll multiple Forgejo servers simultaneouslyserver.connectionsconfig to poll multiple Forgejo servers simultaneously [skip ci]2ee87865a17b517ccfd17b517ccfd1to530056bd10530056bd10to99aee6b52e99aee6b52eto0518962ced0518962cedto9fe105895b9fe105895bto7fc2e94890WIP: feat: useto WIP: feat: useserver.connectionsconfig to poll multiple Forgejo servers simultaneously [skip ci]server.connectionsconfig to poll multiple Forgejo servers simultaneously7fc2e9489033f06a578933f06a57898469762532846976253233ae17db9333ae17db932e7f3d8c26WIP: feat: useto WIP: feat: allow configserver.connectionsconfig to poll multiple Forgejo servers simultaneouslyserver.connectionsconfig to poll multiple Forgejo servers simultaneouslyWIP: feat: allow configto feat: allow configserver.connectionsconfig to poll multiple Forgejo servers simultaneouslyserver.connectionsconfig to poll multiple Forgejo servers simultaneously@aahlenst I've done my best to make this change reviewable by-commit, but pulling apart logical commits has been a bit difficult. Hopefully it's manageable as the overall size isn't too large.
I gave it a whirl and it works great.
@ -256,4 +244,0 @@// if declared successfully, override the labels in the.runner file with valid labels in the config file (if specified)runner.Update(ctx, ls)reg.Labels = ls.ToStrings()if err := config.SaveRegistration(cfg.Runner.File, reg); err != nil {To confirm that I got it right:
In
config.go,parsedDefaultLabelsare now applied to every connection before handing out the newly created connection. That means that every connection in.runnernow receives labels even if there are no labels in.runner.That's terrific and fixes multiple bugs.
That's right. 👍
@ -85,0 +78,4 @@runners := make([]run.RunnerInterface, 0, len(cfg.Server.Connections))ephemeralRunners := make([]bool, 0, len(cfg.Server.Connections))for name, conn := range cfg.Server.Connections {client := createClient(cfg, conn)Info: shadows imported package
client.@ -250,0 +250,4 @@} else {for _, conn := range config.Server.Connections {if conn.FetchInterval == 0 {conn.FetchInterval = s.FetchIntervalWhat about enforcing the Codeberg limit here?
config.New(config.FromFile(...), config.FromRegistration)-- at this point in the code we're in theconfig.FromFile(...)option, and so applying the Codeberg limit here won't apply to the connection imported from the.runnerregistration file.@ -483,0 +503,4 @@if conn.FetchInterval == 0 {conn.FetchInterval = 2 * time.Second}if conn.URL.Host == "codeberg.org" && conn.FetchInterval < 30*time.Second {conn.URL.Hostname()?Hostname()removes an optional port. I'd also compare towww.codeberg.org.Updated to
strings.HasSuffix(conn.URL.Hostname(), "codeberg.org"). 👍@ -23,4 +24,0 @@Token string `json:"token"`Address string `json:"address"`Labels []string `json:"labels"`Ephemeral bool `json:"-"`You were able to optimize that one away? That's great.
Yeah, it led to a four-return-value function in
createRunnerwhich I don't love, but it's easy to fix in a future refactor.@ -64,0 +94,4 @@require.Len(t, c.Labels, 1)assert.Equal(t, c.Labels[0].Name, "ubuntu-latest")assert.Equal(t, c.Labels[0].Schema, "docker")assert.Equal(t, c.Labels[0].Arg, "//code.forgejo.org/oci/node:20-bookworm")Should we add
String()toLabel? Doesn't have to happen as part of this PR. I can do it if you want me to.That'd be nice-to-have in the future.
2e7f3d8c26c456ed039ecascading-pr updated at actions/setup-forgejo#889
Hm. While looking into the end-to-end testing failure (https://code.forgejo.org/forgejo/end-to-end/actions/runs/4938/jobs/6/attempt/1), I discovered one bug before I got to reproducing the test. Previously labels from
.runnerwould be overridden by labels from the config file'srunner.labels, but now they get set into the server connection and therefore look just like they're specified on the connection.I'm looking into a solution for this before this can move forward.
cascading-pr updated at actions/setup-forgejo#889
That may have been the cause of the end-to-end test failure as well, as I can pass the failed test in a local environment now -- will see if it passes in CI as well.
914b403c29is a bit ugly, marking the.runnerlabels as some that can be overridden... but as it's all internal to the config package I don't hate it completely. In the long term as we get rid of.runner, the behaviour becomes clearer and easier to understand.cascading-pr updated at actions/setup-forgejo#889
Another patch for an end-to-end test failure:
10c517717fIt's probably more of a testing artifact than anything else, but I figured it's best to maintain as much backward compatibility as possible.create-runner-filetest was failing because a config file'srunner.labelsis used to declare the runner during this operation, which used to cause a second save of the.runnerfile with the declared labels and cause the config labels to end up in the.runnerfile. The test is validating that those labels end up in.runner(even though, in the presence of a config file, the value here has no impact). I've replicated the same behaviour by initializing the registration with those labels now.end-to-end tests passed, although "actions (11.0)" failed on first run but succeeded on a retry. https://code.forgejo.org/forgejo/end-to-end/actions/runs/4940/jobs/6/attempt/2
I think removing
SaveRegistrationoperations fromdaemonmay cause some of the end-to-end tests to have a little flakiness as a leftover.runnerfile from one test could impact another, depending on what the tests do. But I think this is some laziness in cleanup between end-to-end tests that can be revisited if it proves flaky -- the core change here makes sense and makes the overall operational behaviour more predictable.When's this making it into a release?
@OffsetMonkey538 soon-ish. That's the best I can say. However, you can start testing it today, for example, by building it yourself or fetching the binary from the CI run. Feedback is always appreciated.
Are commits not automatically released as container images? I use docker for running my runners
@OffsetMonkey538 You should be able to build a container image yourself: run
docker build -t forgejo-runner-experimental:latest -f Dockerfile .in the root of this repository.I'm planning to release this as soon as #1383 is complete, which is functionally ready but just resolving a minor difference of technical opinion. 🙂
create-runner-filebehaviour change #1402