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.

Sign up for GitHub

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

Jump to bottom

🐛 CAPD: delete container after failed start to work around port allocation issues #9125

Merged
merged 1 commit into from
Aug 7, 2023
Merged

🐛 CAPD: delete container after failed start to work around port allocation issues #9125

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion test/infrastructure/container/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,13 @@ func (d *dockerRuntime) RunContainer(ctx context.Context, runConfig *RunContaine

// Actually start the container
if err := d.dockerClient.ContainerStart(ctx, resp.ID, types.ContainerStartOptions{}); err != nil {
return errors.Wrapf(err, "error starting container %q", runConfig.Name)
err := errors.Wrapf(err, "error starting container %q", runConfig.Name)
// Delete the container and retry later on. This helps getting around the race
// condition where of hitting "port is already allocated" issues.
if innerErr := d.dockerClient.ContainerRemove(ctx, resp.ID, types.ContainerRemoveOptions{Force: true, RemoveVolumes: true}); innerErr != nil {
return errors.Wrapf(innerErr, "error removing container after failed start: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that mixes the errors in a way that is not easily readable. I think ideally we would use kerrors aggregate (and usually do)

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case it should result in:

{InnerErr}: error removing container after failed start: {err}

Ack, kerrors aggregate would have been an option I didn't think of.

Was also thinking about using go's errors.Join.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is not how error wrapping works. As far as I know it appends the innerErr at the end

Copy link
Member

Choose a reason for hiding this comment

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

(just like the normal fmt.Errorf("adfasfd test: %v", err) would)

Copy link
Member

Choose a reason for hiding this comment

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

Usually we use something like this in CAPI:

reterr = kerrors.NewAggregate([]error{reterr, errors.New("failed to unlock the kubeadm init lock")})

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah so currently it is:

error removing container after failed start: {err}: {innerErr}

I will follow up and use kerrors instead 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thx! I know it's a nit in a test provider, just noticed and was thinking about future me trying to parse the error :)

}
return err
}

if output != nil {
Expand Down