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

Enhance the code to support to use as container engine podman for e2e #473

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

cmoulliard
Copy link
Contributor

@cmoulliard cmoulliard requested a review from a team as a code owner January 6, 2025 17:35
@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard cmoulliard changed the title Enhanced the code to support to use as container engine podman for e2e Enhance the code to support to use as container engine podman for e2e Jan 6, 2025
@cmoulliard cmoulliard requested review from nimakaviani, nabuskey and punkwalker and removed request for nimakaviani January 6, 2025 18:09
Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

If we actually run this in GHA, we are doubling the time required to run all tests. We just need to be mindful when enabling it in the workflows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we create a new package just for podman instead of mixing docker and podman? Commands are very similar so you can abstract and re-use them in podman. Every time there's a slight differences between docker and podman, we have to do things like this if os.Getenv("CONTAINER_ENGINE") == "podman" {. I'd rather branch earlier.

@cmoulliard
Copy link
Contributor Author

If we actually run this in GHA, we are doubling the time required to run all tests.

Why do you want to do that ? This change only allows to run the tests using as engine docker or podman and should not require to run both engines.

@cmoulliard cmoulliard removed the request for review from punkwalker January 7, 2025 12:46
@cmoulliard cmoulliard marked this pull request as draft January 7, 2025 12:47
@cmoulliard cmoulliard marked this pull request as ready for review January 7, 2025 15:16
@cmoulliard
Copy link
Contributor Author

/e2e

Signed-off-by: cmoulliard <[email protected]>
@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard cmoulliard requested a review from nabuskey January 7, 2025 16:33
@cmoulliard
Copy link
Contributor Author

e2e tests succeeded using either podman or docker

@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard
Copy link
Contributor Author

/e2e

@cmoulliard
Copy link
Contributor Author

What do you think if we do this small improvement ?

// Interface containing commands to be executed (aka exec.command()). Until now we just need idpbuilder executable but additional methods could be added
type CommandExecutor interface {
	RunIdp(ctx context.Context, cmd string, timeout time.Duration) ([]byte, error)
}

// Container engine and methods able to return the engine cli to be used or to run a docker podman command
type Engine interface {
	RunCommand(ctx context.Context, cmd string, timeout time.Duration) ([]byte, error)
	GetClient() string
}

// Type name to be used to access the different methods of the interface
type ToolBox struct {
	Client string
}

// Implementation of the method RunIdp of the interface: CommandExecutor
func (t *ToolBox) RunIdp(ctx context.Context, cmd string, timeout time.Duration) ([]byte, error) {
	return shared.RunIdp(ctx, cmd, timeout)
}

// Implementation of the method Getclient of the interface: Engine
func (t *ToolBox) GetClient() string {
	return t.Client
}

// Implementation of the method RunCommand of the interface: Engine
func (t *ToolBox) RunCommand(ctx context.Context, command string, timeout time.Duration) ([]byte, error) {
	cmdCtx, cancel := context.WithTimeout(ctx, timeout)
	defer cancel()

	cmds := strings.Split(command, " ")
	if len(cmds) == 0 {
		return nil, fmt.Errorf("supply at least one command")
	}

	binary := cmds[0]
	args := make([]string, 0, len(cmds)-1)
	if len(cmds) > 1 {
		args = append(args, cmds[1:]...)
	}

	if cmds[1] == "login" || cmds[1] == "push" || cmds[1] == "pull" {
		args = append(args, "--tls-verify=false")
	}

	c := exec.CommandContext(cmdCtx, binary, args...)

	// DOCKER_HOST = unix:///var/run/docker.sock is needed for podman running in rootless mode
	c.Env = append(os.Environ(), "DOCKER_HOST="+os.Getenv("DOCKER_HOST"))

	b, err := c.CombinedOutput()
	if err != nil {
		return nil, fmt.Errorf("error while running %s: %s, %s", command, err, b)
	}

	return b, nil
}

// dummy main code
func main() {
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()

        // create an intance of the toolbox
	toolBox := &ToolBox{ client: "podman"}

	// Execute the idpbuilder create command
	output, err := toolBox.RunIdp(ctx, "create", 3*time.Second)
	...

@nabuskey

…mplementing the interface exist

Signed-off-by: cmoulliard <[email protected]>
Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Do we need to worry about running e2e tests in parallel? By default tests run parallel across multiple packages.

We might end up in a situation where you are running podman and docker tests at the same time using the same port. This will result in failures.

Comment on lines +11 to +12
RunIdpCommand(ctx context.Context, cmd string, timeout time.Duration) ([]byte, error)
RunCommand(ctx context.Context, cmd string, timeout time.Duration) ([]byte, error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Signature is exactly the same. Why can't we use RunCommand? If you need env vars to be set, you can update the function to take a map that defines env vars.

If we do that, we don't need this interface because we can just use the function as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. By the way there is a difference as RunCommand is used to rung the container engine cmd (podman pusll, podman push, etc) why RunIdpCommand is used to run IDP command: idp create, etc

Note: See my comment where I proposed to move RunIdpCommand

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, introducing interfaces is not necessary for two engines. Makes it a bit more difficult to read for beginners. Function calls are sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't use an interface then how would you like that we accommodate within the e2e go file and this function

func TestCreateCluster(t *testing.T, containerEngine container.Engine) {
...

the access to the method runIdpCommand or runCommand no matter which engine is used: podman or docker

...
	t.Log("running idpbuilder create")
	b, err := containerEngine.RunIdpCommand(ctx, fmt.Sprintf("%s create --recreate", IdpbuilderBinaryLocation), 0)
	assert.NoError(t, err, fmt.Sprintf("error while running create: %s, %s", err, b))

tests/e2e/shared/shared.go Outdated Show resolved Hide resolved
@cmoulliard
Copy link
Contributor Author

We might end up in a situation where you are running podman and docker tests at the same time using the same port. This will result in failures.

This is not something that we are doing now. We can address such a need in a separate future PR

@nabuskey
Copy link
Collaborator

nabuskey commented Jan 8, 2025

This is not something that we are doing now. We can address such a need in a separate future PR

You are right that we are not doing this right now because we only have one package. With this change we have two packages. By default, tests in different packages run in parallel. If we have two packages that need to bind to the same port, it's not good. So all I am saying is we need to make sure we test this well and make sure we don't break E2E.

@cmoulliard
Copy link
Contributor Author

We might end up in a situation where you are running podman and docker tests at the same time using the same port. This will result in failures.

What do you then suggest that we do as configuring a machine able to run podman and docker engines at the same time for // tests is quite difficult and will never happen. until ow on github workflow as podman cannot yet run there ?

@nabuskey
Copy link
Collaborator

nabuskey commented Jan 9, 2025

We might end up in a situation where you are running podman and docker tests at the same time using the same port. This will result in failures.

What do you then suggest that we do as configuring a machine able to run podman and docker engines at the same time for // tests is quite difficult and will never happen. until ow on github workflow as podman cannot yet run there ?

The GHA image we use come with both podman and docker. They both work out of the box, as far as I know. I have docker and podman installed on my workstation and they work fine too.

https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md

What we need to do is make sure we don't run these tests in parallel to avoid host port contentions. Looking at the Makefile, we might be ok but we need to test.

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.

2 participants