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

Implement Openstack cloudmock, add integration test #9722

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Aug 10, 2020

This implements basic CRUD logic for all of the openstack resources, a followup to #9691

Most of the resource types are pretty straight forward except:

  • zones and recordsets live under the same /zones path prefix
  • loadbalancer responses have their pools and listener IDs embedded in them
  • routers and routerinterfaces live under the same /routers path prefix
  • the security group rule's remote_group_id query param behaves differently than other filters - if it isn't provided that indicates the rules should not have a remote_group_id field, rather than not filtering on the field at all.

Next steps:

  • server creation just uses a hardcoded IP address. I can update this to use sequential IPs in their subnet in a followup PR
  • loadbalancers and pools dont have their /members subresource yet.
  • I believe loadbalancer creation creates a VIP automatically but I need to look into the docs more
  • I'll need to update this for octavia support
  • We'll want a "complex_openstack" integration test that has a more complete spec.cloudConfig.openstack

This is a large PR but most of the file changes are all independent so the "context depth" for reviewing this shouldn't be too bad, but if you want me to break up the first commit further I can do that.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 10, 2020
@rifelpet
Copy link
Member Author

/test all

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 10, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the area/provider/openstack Issues or PRs related to openstack provider label Aug 10, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2020
@rifelpet rifelpet force-pushed the openstack-cloudmock-part4 branch from 8ecce2d to a01647d Compare August 10, 2020 20:00
This is the bulk of the changes necessary to support openstack integration tests.

As mentioned in cloudmock/openstack/README.md, this involved a lot of wireshark to understand
how the gophercloud clients builds the requests and expects the responses.
This will create / update / update / delete an openstack cluster using cloudmock, ensuring there are no lingering changes reported or orphaned resources
@rifelpet rifelpet force-pushed the openstack-cloudmock-part4 branch from a01647d to 6991655 Compare August 10, 2020 20:23
@rifelpet
Copy link
Member Author

/test all

@rifelpet
Copy link
Member Author

/test pull-kops-verify-staticcheck

@rifelpet rifelpet marked this pull request as ready for review August 10, 2020 20:57
@rifelpet rifelpet changed the title WIP Implement Openstack cloudmock, add integration test mplement Openstack cloudmock, add integration test Aug 10, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2020
@rifelpet rifelpet changed the title mplement Openstack cloudmock, add integration test Implement Openstack cloudmock, add integration test Aug 10, 2020
@rifelpet
Copy link
Member Author

/cc @olemarkus

@olemarkus
Copy link
Member

The test and the clusterspec looks good. I didn't go through all the mock implementations in detail though. I suspect tweaks are needed anyway once we try to add a more complex spec.

There is no way to go in and verify that e.g dns servers was correctly set on a subnet after the tests has run right? One would need something like Heat or Terraform support to validate that? Which doesn't even guarantee that the direct output works as expected.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2020
@k8s-ci-robot k8s-ci-robot merged commit 40d9dbc into kubernetes:master Aug 11, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 11, 2020
@rifelpet
Copy link
Member Author

rifelpet commented Aug 11, 2020

Right, this wont verify that values are set properly. Terraform support would take us a step further since it has input validation, but neither would catch issues like "the wrong IP was used" or "there's a typo in userdata". Those could only be caught by having e2e jobs which would definitely be my preferred route but we've discussed that a few times :) hopefully we can get the infra in place for that sometime.

In the mean time, this integration test will catch issues like:

  • nil pointer dereference in openstack models or openstacktasks
  • openstacktasks dependency cycles
  • inaccurate openstacktasks Find logic
  • missing resource types in pkg/resources/openstack

@justinaslelys
Copy link

This implements basic CRUD logic for all of the openstack resources, a followup to #9698

Hello @rifelpet, are you sure you've mentioned correct issue here?

@rifelpet
Copy link
Member Author

@xoxbet oops you're right, that was supposed to be #9691. Thanks for catching that

@rifelpet rifelpet deleted the openstack-cloudmock-part4 branch October 29, 2020 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants