-
Notifications
You must be signed in to change notification settings - Fork 27
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
Run providers in separate container #194
Conversation
6e2eebe
to
520e91f
Compare
775f018
to
3ed679a
Compare
Signed-off-by: Emily McMullan <[email protected]>
3ed679a
to
37b895e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions and small nits but LGTM besides
|
||
func (a *analyzeCommand) RunProviders(ctx context.Context, networkName string, volName string, providers []string) (map[string]int, error) { | ||
providerPorts := map[string]int{} | ||
port, err := freeport.GetFreePort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is free port working on the host network but it probably needs to work on container network?
This is more of a question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'm trying to think of a way around this. The provider container needs the port as it starts, so we would not be able to first run the provider and then check if a port is in use for that container. And I think we moved away from hard-coding in the ports. My only other thought would be to catch the error and try to run the container again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that may make sense, I was hoping that you could point Freeport to a specific network, but catch err is probably the best approach for now
@@ -20,8 +20,9 @@ import ( | |||
type container struct { | |||
stdout []io.Writer | |||
stderr []io.Writer | |||
name string | |||
Name string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why exporting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is how the analyzer engine connects to the provider's network when the analyzer container is first created. I think the network connect
command requires the container to already be running, which I think may complicate things, since the analyzer container needs to start after the provider container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see makes sense
0cb1ab5
to
316e44e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small request, then this is good to go unless Shawn has something
Signed-off-by: Emily McMullan <[email protected]> update settings with provider images Signed-off-by: Emily McMullan <[email protected]>
316e44e
to
e2f483e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
Except for the Freeport part /LGTM |
Signed-off-by: Emily McMullan <[email protected]>
3cc9870
to
92a6d00
Compare
This will need to be updated for multiple provider containers once we accept multiple inputs
Closes #190