openstack backend #170

sergiocazzolato · 2023-06-02T16:57:52Z

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).

corytodd · 2023-06-07T19:15:01Z

spread/openstack.go

+	}
+	server, err := p.computeClient.RunServer(opts)
+	if err != nil {
+		return nil, &FatalError{fmt.Errorf("Could not create instance", err)}


Should the format string include a format specifier here?

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

Oh okay, I see know thanks for explaining.

corytodd · 2023-06-07T19:22:00Z

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 · 2023-06-07T19:33:18Z

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 · 2023-06-09T20:45:47Z

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

mvo5

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.

README.md

spread/openstack.go

mvo5 · 2023-07-04T15:27:48Z

spread/openstack.go

+		return sameImage, fmt.Errorf("failed to retrieve images list: %s", errorTitle(err.Error()))
+	}
+
+	for _, i := range images {


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"

@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.

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 :)

spread/openstack.go

spread/project.go

mvo5

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.

spread/project.go

sergiocazzolato · 2023-07-17T17:43:22Z

@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.

…tack

mvo5

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.

README.md

spread/openstack.go

mvo5 · 2023-08-17T11:31:50Z

spread/openstack.go

+		return sameImage, fmt.Errorf("failed to retrieve images list: %s", errorTitle(err.Error()))
+	}
+
+	for _, i := range images {


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 :)

…again

…api error

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

ZeyadYasser

Thank you, I have some small questions.

spread/openstack.go

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

niemeyer

Thanks to everyone involved in getting this backend cooked. Let's see if we can get it merged in the near future.

A while ago I had already done an initial high-level pass on the logic with Michael, and still need to review it in more detail, but I'm not expecting major surprises there as my understanding is it's heavily based on the GCE backend. So in this pass I reviewed mainly the bits surrounding the actual backend logic. Once we get to some agreements on those I'll go in and do a more complete review on the backend details.

Please let me know how you'd like to proceed from here, otherwise.

niemeyer · 2024-02-26T14:53:27Z

.github/workflows/test.yaml

@@ -17,7 +17,7 @@ jobs:

      - name: Run tests
        run: |
-          spread google:
+          spread google:ubuntu-20.04-64: google:ubuntu-22.04-64-devstack:tests/openstack


What happened here? If this makes sense (unclear for now), it should be documented so it's more obvious what was disabled and what was enabled here.

Hi, thanks for the review,

In order to test openstack backend against a real openstack interface, we pre-configured a new image with devstack already installed. The main raeson was to speed up and simplify the test (so that image is only used to test openstack). Should I add this explanation in the workflow?

niemeyer · 2024-02-26T14:56:36Z

README.md

+```
+
+The Openstack backend gets all the information to authenticate from the
+environment variables. The following variables have to be set:


This does not look like a great practice, as it's sending real authentication data to every single test run. It also disagrees with what we do with the Google backend, and every other backend maybe?

I'm almost certainly not the first one to point this out, so what is the actual alternative practice inside the OpenStack community?

The most common mechanism to connect a client through the Openstack API is by sourcing a file with teh environment variables (like the one we get to use openstack client in canonistack). For example in PS5, in the environment I also see the openstack env vars in my environment (they are managed by vault tool), so I presume juju is using those to connect to openstack.

I agree with you this is not a good practice because this means we need to have an env var with the user and password. I'll research which workaround we could use.

I updated the documentation to explain better which env vars need to be defined. It is also supported to authenticate by using a key (similar to what we have in google). The key has to be stored in an env var to be loaded.

An improvement could be to load the vars from a file (as we hav in google) instead of the env. @niemeyer What do you think?

Finally I added a similar approach that the used in google.

niemeyer · 2024-02-26T15:00:49Z

README.md

+OS_PASSWORD
+OS_REGION_NAME
+OS_INTERFACE
+OS_IDENTITY_API_VERSION


In addition to the above, which of those variables have a direct equivalent in the Google backend setup?

In google we use the env var SPREAD_GOOGLE_KEY which has a link to the file with the following data:
"type"
"project_id"
"private_key_id"
"private_key"
"client_email"
"client_id"
"auth_uri"
"token_uri"
"auth_provider_x509_cert_url"
"client_x509_cert_url"

I updated the list of environment variables in the docs and I understand that the equivalent are:
project_id <-> OS_PROJECT_ID
auth_uri <-> OS_AUTH_URL
private_key_id <-> OS_ACCESS_KEY
private_key <-> OS_SECRET_KEY
client_id <-> ( OS_PROJECT_DOMAIN_NAME | OS_USER_DOMAIN_NAME )

niemeyer · 2024-02-26T15:03:59Z

spread.yaml

    - .spread-reuse.yaml
    - tests/.spread-reuse.yaml
    - $CACHE_DISABLED
+    - "*.snap"


Why the quotes only on this one? Also, where do the snap files come from?

niemeyer · 2024-02-26T15:08:31Z

spread/export_openstack_test.go

+	"context"
+	"time"
+
+	gooseClient "github.com/go-goose/goose/v5/client"


s/gooseClient/gooseclient/

Package names in Go are not typically cammel-cased.

niemeyer · 2024-02-26T15:36:24Z

tests/openstack/task.yaml

+    # Check the error in case the network does not exist
+    spread openstack:cirros-64-wrong-network: -v -reuse -resend &> task.out || true
+    grep 'cannot find valid network with name "noexist"' task.out
+    test -z "$(openstack server list)"


We have a system with very nice abstraction for composing test scenarios. Is there a good reason for us to choose to cook all of them as a shell script inside a single task instead of using that composition to at least bundle closely related ideas together?

Initially I though this but the problem I found is the time that devstack consumes to start (about 10 minutes), so if I create variants it will require more machines to get results. If makes sense I could move the scenarios to different variants and run them in parallel using more workers.

niemeyer · 2024-02-26T15:40:14Z

tests/openstack/task.yaml

+        fi
+        sleep 1
+    done
+    test -z "$(openstack server list)"


Is there a prior check that ensures that the list started empty in the first place? Also, please consider the comment above in this context.

Also, won't OpenStack show the any used servers as terminated instead of just showing an empty list?

Added the initial check to verify there is not any server running.

The command openstack server list doesn't show terminated servers, it just includes active ones.

niemeyer · 2024-02-26T15:41:23Z

tests/openstack/task.yaml

+    # The instance was created and the status has to be active to
+    # fail trying to access through ssh
+    spread openstack:cirros-64: -v -reuse -resend &> task.out || true
+    grep 'cannot find ready marker in console output for .*: timeout reached' task.out


This is not a great method to verify the contents, because on failure we'll get nothing, and won't know why we got nothing. I believe we have common practices for this in the Spread world. How do they look like?

niemeyer · 2024-02-26T15:51:50Z

tests/openstack/task.yaml

+
+    # trigger 1 instance and check it can be listed and the garbage collect works
+    test "0" = "$(spread -gc | grep -c "Checking openstack instance")"
+    spread openstack:cirros-64: &>/dev/null &


When is this stopped/checked for correctness?

niemeyer · 2024-02-26T15:52:20Z

tests/openstack/task.yaml

+        fi
+        sleep 1
+    done
+    openstack server show "$SERVER_ID" -f shell | MATCH 'status="ACTIVE"'


So all this test does is check that some server has shown up? Nothing else at all?

Updated order of openstack in list of backends Updated name of gooseClient -> gooseclient Fixed issues, it was not trying ssh connection when serial console is not available Updated spread.yaml noexist -> invalid

…cate

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

niemeyer · 2024-03-11T17:10:15Z

Folks, please schedule a call for us next week so we can have a general conversation about the approach for authentication and testing before we all spend too many cycles on different avenues.

Also include test to validate the key/secret authentication and tne env file

ZeyadYasser

Thank you, small comments

README.md

spread.yaml

spread/openstack.go

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 · 2024-03-25T21:55:57Z

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 added 3 commits April 14, 2023 12:38

New openstack backend

b4dfab2

First change for the openstack backend implementation

99a6884

Changes done to support openstack bakend after testing in canonistack

ccd286b

corytodd reviewed Jun 7, 2023

View reviewed changes

adding network and security groups to systems

81ec212

sergiocazzolato changed the title ~~tests: new openstack backend~~ openstack backend Jun 14, 2023

mvo5 reviewed Jul 4, 2023

View reviewed changes

mvo5 added 2 commits July 5, 2023 14:38

openstack: tweak function returns and messages

6adfc24

spread: rever change in {google,humbox,lindode}.go

b08c9ca

mvo5 force-pushed the migrate-openstack-backend branch from 04aa498 to b08c9ca Compare July 5, 2023 12:41

openstack: two more tweak about error message

e0052c3

This comment was marked as resolved.

Sign in to view

mvo5 reviewed Jul 5, 2023

View reviewed changes

spread/project.go Outdated Show resolved Hide resolved

mvo5 reviewed Jul 5, 2023

View reviewed changes

spread/project.go Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

mvo5 reviewed Jul 12, 2023

View reviewed changes

spread/project.go Outdated Show resolved Hide resolved

spread/project.go Outdated Show resolved Hide resolved

sergiocazzolato added 2 commits July 19, 2023 16:18

support networks and rename groups intead of security-groups in opens…

c82e63f

…tack

Add new test to validate openstack backend

b741ae6

sergiocazzolato requested a review from mvo5 July 22, 2023 03:00

sergiocazzolato and others added 4 commits July 29, 2023 16:25

Add new checks for errors and improve error management and messages

ba946a3

Test spread -gc

5e22375

openstack: add unit test for "openstackName()"

5c7f2bb

openstack: add unit tests for findImage()

0e4f033

mvo5 reviewed Aug 17, 2023

View reviewed changes

spread: improve openstackName() test and make it similar to "google" …

8fbae16

…again

mvo5 force-pushed the migrate-openstack-backend branch from 9053022 to 8fbae16 Compare August 17, 2023 16:28

sergiocazzolato added 4 commits December 4, 2023 18:41

support check serial console

0eb485d

Using get console action instead of get serial

7578146

Don't try waiting ssh when the serial output failed because of timeout

391f32e

Update errors to make sure we retry ssh connection on serial console …

e93ff91

…api error

sergiocazzolato force-pushed the migrate-openstack-backend branch from 1df6d27 to 6d6ead8 Compare December 6, 2023 15:31

adjust timeouts and messages getting error retrieving serial console

8a2c853

sergiocazzolato force-pushed the migrate-openstack-backend branch from 6d6ead8 to 8a2c853 Compare December 6, 2023 16:19

sergiocazzolato requested review from mvo5, ZeyadYasser and corytodd February 19, 2024 13:09

spread/openstack: refactor openstack backend

5cb6810

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

ZeyadYasser reviewed Feb 20, 2024

View reviewed changes

spread/openstack.go Show resolved Hide resolved

spread/openstack.go Show resolved Hide resolved

spread/openstack.go Outdated Show resolved Hide resolved

spread/tests/openstack: update test for serial output method

b18bfde

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

niemeyer requested changes Feb 26, 2024

View reviewed changes

sergiocazzolato and others added 5 commits February 28, 2024 11:35

Addressed comments

15c7274

Updated order of openstack in list of backends Updated name of gooseClient -> gooseclient Fixed issues, it was not trying ssh connection when serial console is not available Updated spread.yaml noexist -> invalid

Improve the README to explain which env vars are required to authenti…

da9a266

…cate

spread/openstack: remove exported types in unit tests

ec72ad6

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

spread/openstack: fix fallback logic

d2d54a3

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

Allow to define a .env file with the env vars required to autheticate

0484ab2

sergiocazzolato added 2 commits March 12, 2024 22:31

Allow authentication with key and secret

5f673c8

Also include test to validate the key/secret authentication and tne env file

add more details in the openstack test to explain -gc

755dc0e

ZeyadYasser removed the Ready label Mar 20, 2024

ZeyadYasser reviewed Mar 20, 2024

View reviewed changes

ZeyadYasser added 4 commits March 25, 2024 23:37

Fix README formatting

86b398d

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

openstack: comment out existing ssh configs that conflicts

701600c

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

openstack: rename goose{C,c}lient package

027ba48

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

openstack: drop spread tests

58c7fad

devstack has many issues and cannot fully replicate a normal openstack cluster for testing. Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

Fix cloud-init config to make openstack work again

7f84982

openstack backend #170

Are you sure you want to change the base?

openstack backend #170

Conversation

sergiocazzolato commented Jun 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corytodd commented Jun 7, 2023

sergiocazzolato commented Jun 7, 2023

corytodd commented Jun 9, 2023

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

mvo5 left a comment

Choose a reason for hiding this comment

sergiocazzolato commented Jul 17, 2023

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser left a comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiocazzolato Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Mar 11, 2024

ZeyadYasser left a comment

Choose a reason for hiding this comment

ZeyadYasser commented Mar 25, 2024

sergiocazzolato Feb 27, 2024 •

edited

Loading