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

Make listener and metrics ports configurable #1647

Merged
merged 9 commits into from
May 28, 2019

Conversation

jannfis
Copy link
Member

@jannfis jannfis commented May 26, 2019

Add the ability to specify the listener ports for API server and repo server processes as well as the listener ports for the metrics server of API server, repo server and application conroller. I found having this possibility most useful on my local dev machine - especially for executing the E2E tests - where other processes are already listening on some of the default ports used by ArgoCD.

This change:

  • Introduces --port parameter for argocd-server and argocd-repo-server
  • Introduces --metrics-port parameter for argocd-server, argocd-repo-server and argocd-application-controller

If none of the parameters are specified, previous default values are used, so this change should be non-breaking.

This option lets you specifiy the TCP port that argocd-server should use to listen for incoming connections.
Non-breaking: If not specified, the default value of 8080 will be used as ever since.
Non-breaking: Will use the standard ports as default if parameters are not specified.
@jannfis
Copy link
Member Author

jannfis commented May 26, 2019

Is CI still broken? I think the errors thrown at me are weird:

From "test" step:
ok github.com/argoproj/argo-cd/util/jwt 0.010s coverage: 26.7% of statements time="2019-05-26T19:49:21Z" level=error msg="ks show prodfailed: level=error msg=\"find objects: evaluating parameters for module \\\"/\\\" in environment \\\"prod\\\": Get https://raw.githubusercontent.com/kubernetes/kubernetes/v1.10.0/api/op enapi-spec/swagger.json: net/http: request canceled (Client.Timeout exceeded while awaiting headers)\"\n" --- FAIL: TestShow (10.18s)

From "build-e2e" step:
time="2019-05-26T19:52:14Z" level=info msg="ran command" duration=70.575188ms err="exit status 1" time="2019-05-26T19:52:14Z" level=info msg="running command" args="[app get argocd-e2e-kcvzc -o yaml --server 10.43.87.224:443 --auth-token eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE1NTg5MDAzMzMsImlzcyI6ImFyZ29jZCIsIm5iZiI6MTU1ODkwMDMzMywic 3ViIjoiYWRtaW4ifQ.SSHbm4psodAYqGI8c7k-ZITfkFZUmHGLROLOKdno9PI --insecure]" name=../../dist/argocd workDir= time="2019-05-26T19:52:14Z" level=info msg="0: time=\"2019-05-26T19:52:14Z\" level=fatal msg=\"rpc error: code = Unauthenticated desc = invalid session: signature is invalid\"" time="2019-05-26T19:52:14Z" level=info msg="1: " time="2019-05-26T19:52:14Z" level=info msg="ran command" duration=68.517748ms err="exit status 1" time="2019-05-26T19:52:14Z" level=fatal msg="exit status 1" FAIL github.com/argoproj/argo-cd/test/e2e 1.451s

On my local workstation, both make test and make test-e2e succeed.

@alexec alexec self-requested a review May 28, 2019 15:05
@alexec
Copy link
Contributor

alexec commented May 28, 2019

Reviewing.

@alexec
Copy link
Contributor

alexec commented May 28, 2019

That failed test is flakey.

@codecov-io
Copy link

codecov-io commented May 28, 2019

Codecov Report

Merging #1647 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
- Coverage   32.56%   32.52%   -0.04%     
==========================================
  Files          71       71              
  Lines       10814    11030     +216     
==========================================
+ Hits         3522     3588      +66     
- Misses       6793     6920     +127     
- Partials      499      522      +23
Impacted Files Coverage Δ
server/server.go 46.97% <100%> (+0.23%) ⬆️
controller/appcontroller.go 37.5% <100%> (+1.4%) ⬆️
server/rbacpolicy/rbacpolicy.go 66.66% <0%> (-13.03%) ⬇️
server/application/application.go 23.12% <0%> (-0.38%) ⬇️
server/application/forwarder_overwrite.go 100% <0%> (ø) ⬆️
server/project/project.go 62.04% <0%> (+8.04%) ⬆️
server/account/account.go 68.18% <0%> (+8.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 894b150...26e1eda. Read the comment docs.

@alexec
Copy link
Contributor

alexec commented May 28, 2019

This passes all tests locally.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good. @alexmt

cmd/argocd-repo-server/main.go Outdated Show resolved Hide resolved
cmd/argocd-server/commands/root.go Outdated Show resolved Hide resolved
server/server_test.go Outdated Show resolved Hide resolved
@alexmt alexmt self-requested a review May 28, 2019 16:52
@jannfis
Copy link
Member Author

jannfis commented May 28, 2019

@alexec Thanks for taking the time to review the changes! I changed --listen-port argument to be --port, I think you are right and this is more intuitive and also more consistent with other command line arguments "out there".

As for displaying default values in the usage summary, this is also a valid request. Fortunately, Cobra already does that for us when a default value for a given parameter was set, and it does not need to be written in the parameter's help text explicitly:

[eris@docker-build argo-cd]$ ./dist/argocd-server --help | egrep -- '--(|metrics-)port'
--metrics-port int Start metrics on given port (default 8083)
--port int Listen on given port (default 8080)
[eris@docker-build argo-cd]$ ./dist/argocd-repo-server --help | egrep -- '--(|metrics-)port'
--metrics-port int Start metrics server on given port (default 8084)
--port int Listen on given port for incoming connections (default 8081)
[eris@docker-build argo-cd]$ ./dist/argocd-application-controller --help | egrep -- '--(|metrics-)port'
--metrics-port int Start metrics server on given port (default 8082)

@alexec alexec merged commit 9f9a076 into argoproj:master May 28, 2019
@alexec alexec added this to the v1.1 milestone May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants