Skip to content
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

Store agent ID in config file #1888

Merged
merged 7 commits into from
Jul 2, 2023

Conversation

ttomasini
Copy link
Contributor

@ttomasini ttomasini commented Jun 26, 2023

This PR solves the following TODO in /cmd/agent/agent.go:

agentID := int64(-1) // TODO: store agent id in a file

It ensures that the agent ID is persisted in a file and can then, for example, also be bind-mounted in a Docker container. The new WOODPECKER_AGENT_ID_CONFIG_PATH WOODPECKER_AGENT_ID_FILE environment variable makes it possible to configure the file path.

@ttomasini ttomasini force-pushed the improvement/agent_id_management branch from 7e5ad3d to 0d22a51 Compare June 26, 2023 22:02
@6543 6543 added agent enhancement improve existing features labels Jun 26, 2023
@6543 6543 added this to the 1.0.0 milestone Jun 26, 2023
.gitignore Outdated Show resolved Hide resolved
cmd/agent/agent.go Outdated Show resolved Hide resolved
cmd/agent/flags.go Outdated Show resolved Hide resolved
@ttomasini
Copy link
Contributor Author

ttomasini commented Jun 26, 2023

  • add new env variable to documentation: docs/docs/30-administration/15-agent-config.md

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Jun 27, 2023

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-1888.surge.sh

@ttomasini ttomasini force-pushed the improvement/agent_id_management branch 4 times, most recently from dcad119 to f74e496 Compare June 27, 2023 18:52
@ttomasini ttomasini requested a review from 6543 June 27, 2023 18:56
@ttomasini ttomasini force-pushed the improvement/agent_id_management branch 2 times, most recently from c7651b0 to 09ecdeb Compare June 28, 2023 13:25
@ttomasini ttomasini requested a review from anbraten June 28, 2023 16:19
@zc-devs
Copy link
Contributor

zc-devs commented Jun 28, 2023

Do we really need a config file? Why? Why a file per option? May be one (few) yaml config(s) is better?

My proposals:

  1. Introduce an env var WOODPECKER_AGENT_ID. It is more convenient to configure via env vars, at least in Kubernetes. It is more consistent with current configuration also.
  2. Use UUID instead of Int. Perhaps hostname is more readable.
  3. Drop config. If not, use one yaml config. Or leave as is, but consider to rename file var to WOODPECKER_AGENT_ID_FILE.

If we want to automatically generate an ID by agent, then

  1. Try to load the ID from WOODPECKER_AGENT_ID.
  2. If doesn't exist, generate new UUID and store it in the file. In this case the separate file could be better.

@zc-devs
Copy link
Contributor

zc-devs commented Jun 28, 2023

Perhaps hostname is more readable.

There is Name (name of agent) field. So hostname suits better here, I think...

@ttomasini
Copy link
Contributor Author

ttomasini commented Jun 28, 2023

Do we really need a config file? Why? Why a file per option? May be one (few) yaml config(s) is better?

My proposals:

  1. Introduce an env var WOODPECKER_AGENT_ID. It is more convenient to configure via env vars, at least in Kubernetes. It is more consistent with current configuration also.
  2. Use UUID instead of Int. Perhaps hostname is more readable.
  3. Drop config. If not, use one yaml config. Or leave as is, but consider to rename file var to WOODPECKER_AGENT_ID_FILE.

If we want to automatically generate an ID by agent, then

  1. Try to load the ID from WOODPECKER_AGENT_ID.
  2. If doesn't exist, generate new UUID and store it in the file. In this case the separate file could be better.

Hey @zc-devs

I fully agree with most of your points. The main reason I chose this implementation is for backwards compatibility.

Currently the server generates the agent ID for the agent and so far the server can only handle int64 values. It was important to me to improve the current situation with as little additional complexity as possible. This includes the choice of a simple configuration format. Especially since the 1.0.0 release is not far away, I didn't want to introduce any new complex changes here.

But I think, especially for the next new and breaking release, everything you suggest (especially the use of UUIDs) is highly preferable over the current implementation.

@anbraten
Copy link
Member

WOODPECKER_AGENT_ID is not necessary as you could already set the specific agent-token instead. The id file would be only interesting for agents using the system-token. The issue is that the system-token normally always creates a new agent in the DB and returns the id to the agent. This way the agent could store it to a file and at a later point access the server again using the system-token & agent-id combination. If using a specific agent-token the id can be directly identified by the server as the token would be unique.

@zc-devs
Copy link
Contributor

zc-devs commented Jun 29, 2023

Currently the server generates the agent ID for the agent

Ohh... That's sad...

Then env var should be renamed in consistent with current state/docs:

As it may be difficult to use docker secrets for environment variables woodpecker allows to read sensible data from files by providing a *_FILE option of all sensible configuration variables (WOODPECKER_AGENT_SECRET_FILE, WOODPECKER_DATABASE_DATASOURCE_FILE, WOODPECKER_ENCRYPTION_KEY_FILE ...)

@zc-devs
Copy link
Contributor

zc-devs commented Jun 29, 2023

@anbraten thanks for great explanation. Things became much more clear. I'll add agent registration process in docs later, if no one do it ahead.

The id file would be only interesting for agents using the system-token. The issue is that the system-token normally always creates a new agent in the DB and returns the id to the agent. This way the agent could store it to a file and at a later point access the server again using the system-token & agent-id combination.

Yeah, I see
Screenshot 2023-06-29 121043

WOODPECKER_AGENT_ID is not necessary as you could already set the specific agent-token instead. If using a specific agent-token the id can be directly identified by the server as the token would be unique.

That is good news. I'll try it.

In #1648 I forgot to add WOODPECKER_AGENT_SECRET (system token) in the server configuration. So, the agent could not register on the server. When I added specific agent token manually, the agent registered successfully.

Eventually I realized mistake and added system token in the server configuration. It works also - I have duplicates now :)

@6543
Copy link
Member

6543 commented Jun 30, 2023

please update branch - so ci runs again as expected :)

@ttomasini ttomasini force-pushed the improvement/agent_id_management branch 2 times, most recently from 2c53b41 to 590e584 Compare June 30, 2023 12:20
@ttomasini
Copy link
Contributor Author

ttomasini commented Jun 30, 2023

please update branch - so ci runs again as expected :)

go: downloading github.com/emicklei/go-restful/v3 v3.9.0
github.com/woodpecker-ci/woodpecker/cmd/server imports
	github.com/gin-gonic/gin imports
	github.com/gin-gonic/gin/binding imports
	github.com/go-playground/validator/v10 imports
	github.com/gabriel-vasile/mimetype: github.com/gabriel-vasile/[email protected]: reading https://proxy.golang.org/github.com/gabriel-vasile/mimetype/@v/v1.4.2.zip: 403 Forbidden

@ttomasini ttomasini force-pushed the improvement/agent_id_management branch 2 times, most recently from 77eb72b to 38da124 Compare July 1, 2023 08:18
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2023

Codecov Report

Patch coverage: 74.07% and project coverage change: +0.09 🎉

Comparison is base (3d435a9) 39.25% compared to head (77eb72b) 39.34%.

❗ Current head 77eb72b differs from pull request most recent head 92d1766. Consider uploading reports for the commit 92d1766 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1888      +/-   ##
==========================================
+ Coverage   39.25%   39.34%   +0.09%     
==========================================
  Files         177      177              
  Lines       10713    10739      +26     
==========================================
+ Hits         4205     4225      +20     
- Misses       6215     6220       +5     
- Partials      293      294       +1     
Impacted Files Coverage Δ
cmd/agent/agent.go 13.00% <74.07%> (+8.43%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ttomasini ttomasini force-pushed the improvement/agent_id_management branch from 38da124 to 92d1766 Compare July 1, 2023 20:16
@ttomasini
Copy link
Contributor Author

ttomasini commented Jul 2, 2023

I am not sure what is happening here but it looks like the pipeline has gone through successfully but github is still waiting for feedback:
https://ci.woodpecker-ci.org/repos/3780/pipeline/6298/1
image

It is similar but not equal to #1895, any ideas?

@qwerty287
Copy link
Contributor

It's fixed already, the pipeline must be restarted. Did that, see https://ci.woodpecker-ci.org/repos/3780/pipeline/6322

@anbraten anbraten merged commit eb5c48a into woodpecker-ci:master Jul 2, 2023
@6543 6543 mentioned this pull request Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants