Skip to content

openstack backend#170

Merged
niemeyer merged 74 commits intocanonical:masterfrom
sergiocazzolato:migrate-openstack-backend
Apr 14, 2025
Merged

openstack backend#170
niemeyer merged 74 commits intocanonical:masterfrom
sergiocazzolato:migrate-openstack-backend

Conversation

@sergiocazzolato
Copy link
Copy Markdown
Contributor

This is the new openstack backend. Through this backend it is possible to run tests/tasks in openstack infrastructure.

The documenatation is also added explaning how to setup spread to use it.

For the openstack backend implementation the lib goose was used. This lib provides the clients needed to interact with the different openstack modules (nova. neutron, glance and, keystone).

Comment thread spread/openstack.go Outdated
}
server, err := p.computeClient.RunServer(opts)
if err != nil {
return nil, &FatalError{fmt.Errorf("Could not create instance", err)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should the format string include a format specifier here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the FatalError means it is not needed to retry and it is built from an error, I already updated the error in the backend to make sure we retry only when it makes sense

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh okay, I see know thanks for explaining.

@corytodd
Copy link
Copy Markdown

corytodd commented Jun 7, 2023

Awesome, we are able to allocate and discard on our openstack tenant. Do you think it's feasible to support adding security groups automatically? For example, we do not use a default allow-ssh policy on our network so in order for these tests to work I had to manually attach the policy after allocation but before the test ran. We have other ports that may need to be open depending on the test so having a way to set these dynamically would be useful for us.

@sergiocazzolato
Copy link
Copy Markdown
Contributor Author

Awesome, we are able to allocate and discard on our openstack tenant. Do you think it's feasible to support adding security groups automatically? For example, we do not use a default allow-ssh policy on our network so in order for these tests to work I had to manually attach the policy after allocation but before the test ran. We have other ports that may need to be open depending on the test so having a way to set these dynamically would be useful for us.

Thanks for taking a look. I'll add few extra features:
add extra storage
specify the network
specify the security group

@corytodd
Copy link
Copy Markdown

corytodd commented Jun 9, 2023

Specifying the network and security group work great! This passes our testing, I would support a +1 on getting this merged. Thanks!

@sergiocazzolato sergiocazzolato changed the title tests: new openstack backend openstack backend Jun 14, 2023
Copy link
Copy Markdown
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Sergio! I did a first review and have some suggestion inline. I will also sync with Gustavo to ask how he wants to see this moving forward.

Comment thread README.md Outdated
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
return sameImage, fmt.Errorf("failed to retrieve images list: %s", errorTitle(err.Error()))
}

for _, i := range images {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this code should be slightly more elaborate and follow googleProvider:image or linode:tempate(). linode is simpler and just does a prefix search but afaict all do more than just check for "contains"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mvo5 could you please elaborate a bit more this? In openstack the images dont have family or project associated as in gce, so because of that I used the contain.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I was mostly wondering what contraints there are about image names, I created a unit test for the code now so that we can explore various test cases and examples :)

Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go
Comment thread spread/openstack.go Outdated
@mvo5 mvo5 force-pushed the migrate-openstack-backend branch from 04aa498 to b08c9ca Compare July 5, 2023 12:41
@mvo5

This comment was marked as resolved.

Comment thread spread/project.go Outdated
Comment thread spread/project.go Outdated
mvo5

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Gustavo also asked that backends that do not (yet) support the options network/groups should error when they are specified.

A smoke spread test against a real system should be included and unit tests as far as possible without modifying non-openstack code.

The way images are selected/filtered also needs a review.

Comment thread spread/project.go Outdated
Comment thread spread/project.go Outdated
@sergiocazzolato
Copy link
Copy Markdown
Contributor Author

@mvo5 about the network list associated to a machine, the consideration here is that where there are more than 1 network associated to a machine, the ip used by spread to connect has to be provided by the first network. I'll include that in the README.

@sergiocazzolato sergiocazzolato requested a review from mvo5 July 22, 2023 03:00
Copy link
Copy Markdown
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks, I looekd a bit more and I really like the updated spread test! I also added a few comments and suggestions and pushed some small tweaks.

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md
Comment thread spread/openstack.go
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go Outdated
return sameImage, fmt.Errorf("failed to retrieve images list: %s", errorTitle(err.Error()))
}

for _, i := range images {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I was mostly wondering what contraints there are about image names, I created a unit test for the code now so that we can explore various test cases and examples :)

@mvo5 mvo5 force-pushed the migrate-openstack-backend branch from 9053022 to 8fbae16 Compare August 17, 2023 16:28
Also include test to validate the key/secret authentication and tne env
file
@ZeyadYasser ZeyadYasser removed the Ready label Mar 20, 2024
Copy link
Copy Markdown
Contributor

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

Thank you, small comments

Comment thread README.md Outdated
Comment thread spread.yaml Outdated
Comment thread spread.yaml
Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go
Comment thread spread/openstack.go
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
devstack has many issues and cannot fully replicate a normal
openstack cluster for testing.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser
Copy link
Copy Markdown
Contributor

I dropped the spread test for openstack due to issues and inconsistencies faced with devstack where it cannot replicate a normal openstack cluster.

A better alternative is to do something similar to google. After the openstack backend is merged, we add it as a backend in spread.yaml.

sergiocazzolato and others added 3 commits January 15, 2025 11:30
Now the volumes are cleaned manually to avoid issues when using
delete_on_termination=true
Garbage collection also is update to delete orphan volumes
Establish a set of attributes to encode credentials instead of reading
them directly from the environment. Only identity API version 3 is
currently supported.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Copy link
Copy Markdown

@dariuszd21 dariuszd21 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just minor questions

Comment thread spread/openstack.go Outdated
Comment thread spread/openstack.go
@cmatsuoka cmatsuoka force-pushed the migrate-openstack-backend branch 2 times, most recently from 5e21ff2 to 2e787a8 Compare March 11, 2025 13:12
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Comment thread spread/openstack.go Outdated
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka requested a review from niemeyer March 27, 2025 22:17
Copy link
Copy Markdown
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thank you very much to everyone involved in developing and polishing this backend, and thanks @cmatsuoka for polishing the final details for unblocking it. With your latest changes, the shared part of these changes seem to follow the existing patterns, and any remaining problems are going to be specific to OpenStack itself, so let's go ahead and get this in.

There's one final detail to be done shortly in a follow up, which is fixing the casing, per comment below. But this is fine to be done as a separate PR so we don't wait even longer than we already did.

Comment thread README.md
[LXD backend](#lxd)
[QEMU backend](#qemu)
[Google backend](#google)
[Openstack backend](#openstack)
Copy link
Copy Markdown
Contributor

@niemeyer niemeyer Apr 14, 2025

Choose a reason for hiding this comment

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

The casing of the project name is incorrect, both in documentation and in the code itself. The project seems to call itself as OpenStack instead of Openstack, so I would suggest we follow suit and do a s/Openstack/OpenStack/ everywhere. Let's please keep "openstack", though (not "openStack").

@niemeyer niemeyer merged commit f1035e9 into canonical:master Apr 14, 2025
1 check passed
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.

7 participants