faucetsdn / faucetagent

gNMI agent for faucet configuration
4 stars 2 forks source link

option to set the address prometheus is on #25

Closed cglewis closed 5 years ago

lantz commented 5 years ago

@cglewis Could you explain the use case for this or the bug it fixes? The original spec was for the agent to run on the same machine as FAUCET (otherwise you may have to jump through hoops to make sure that the file system and pid space are shared, for example.)

I'm not necessarily opposed to it but it could cause confusion if FAUCET is running on another machine which doesn't share the same file system/pid space/OS kernel. Is it because you want to reuse server certificates or something? But that doesn't make sense either since we're not using https.

cglewis commented 5 years ago

So the issue I was having with it being localhost was when running Faucet in a Docker container either manually or using the docker-compose, the default docker-compose.yaml for Faucet doesn't expose 9302 so localhost is only within the scope of inside the container, not the host it's running on. I see your point about the pid space, though it wouldn't be an issue if using the FAUCET_CONFIG_STAT_RELOAD instead of HUP I think? And a shared filesystem with Docker volumes is either already being done or easy enough to do.

Basically I want to be able to run Faucet in a container and FaucetAgent in a different container on the same host, I saw this as a stepping stone to be able to get there.

lantz commented 5 years ago

I was afraid you'd say something like that; I'm not entirely enthusiastic about decoupling the agent from FAUCET or running it in a different container, VM, or physical machine, as it seems more likely to introduce unexpected behavior and error conditions (such as having more than one agent trying to control FAUCET at the same time, having an agent connected to the wrong FAUCET, sending HUP to the wrong FAUCET, writing the wrong config file, or introducing security issues) but we can do it and hope/recommend that people don't shoot themselves in the foot/head.

Do you want to add a test for this? Also should we be able to specify the entire URL (e.g. https?) A simple test could have FAUCET listen on different prometheus ports and verify that the agent can connect to that port.

Also are there any checks we can do to help users avoid some of the possible mistakes? Anything we should mention in the documentation?

gizmoguy commented 5 years ago

Sounds like something we might want to discuss on the dev call tomorrow?

lantz commented 5 years ago

After discussing it today, I think this is fine and we can move forward on it; if you want to add a simple test (and perhaps allow specifying the whole URL including https if desired?) then think we should merge it in.

cglewis commented 5 years ago

@lantz tests have been added. I'll have to update this after #24 is merged in though, as it will have conflicts.

cglewis commented 5 years ago

Ok, changed.