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

Run providers in separate container #194

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

eemcmullan
Copy link
Collaborator

@eemcmullan eemcmullan commented Apr 15, 2024

This will need to be updated for multiple provider containers once we accept multiple inputs

Closes #190

@eemcmullan eemcmullan changed the title Run Java provider in separate container Run provider in separate container Apr 15, 2024
Dockerfile Outdated Show resolved Hide resolved
cmd/openrewrite.go Outdated Show resolved Hide resolved
@eemcmullan eemcmullan force-pushed the prov-container branch 3 times, most recently from 6e2eebe to 520e91f Compare April 18, 2024 15:47
@eemcmullan eemcmullan changed the title Run provider in separate container Run providers in separate container Apr 18, 2024
@eemcmullan eemcmullan force-pushed the prov-container branch 4 times, most recently from 775f018 to 3ed679a Compare April 19, 2024 18:35
@eemcmullan eemcmullan marked this pull request as ready for review April 19, 2024 18:35
cmd/analyze.go Outdated Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved
cmd/settings.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shawn-hurley shawn-hurley left a 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

cmd/analyze.go Outdated Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved

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()
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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

cmd/analyze.go Show resolved Hide resolved
cmd/analyze.go Outdated Show resolved Hide resolved
@@ -20,8 +20,9 @@ import (
type container struct {
stdout []io.Writer
stderr []io.Writer
name string
Name string
Copy link
Contributor

Choose a reason for hiding this comment

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

why exporting?

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see makes sense

Copy link
Contributor

@pranavgaikwad pranavgaikwad left a 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

Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: Emily McMullan <[email protected]>

update settings with provider images

Signed-off-by: Emily McMullan <[email protected]>
Copy link
Member

@aufi aufi left a comment

Choose a reason for hiding this comment

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

ack

@shawn-hurley
Copy link
Contributor

Except for the Freeport part /LGTM

cmd/analyze.go Outdated Show resolved Hide resolved
@eemcmullan eemcmullan merged commit 1484e83 into konveyor:main Apr 24, 2024
2 checks passed
@eemcmullan eemcmullan deleted the prov-container branch August 27, 2024 15:51
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.

Add support for running containers for each provider & the analyzer engine
4 participants