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

Add pre-created fip testcase #61

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

rlobillo
Copy link
Contributor

Covers partially the epic https://issues.redhat.com/browse/OSASINFRA-2753

Test that creates a FIP on openstack and uses it for creating an UDP svc type:LoadBalancer that sets on its spec the pre-created FIP.

This test will be skipped for Kuryr NetworkType and will run with both amphora and OVN provider.

Copy link
Contributor

@dulek dulek 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 please take a look at Maysa's remarks.

test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
test/extended/openstack/loadbalancers.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2022
@rlobillo rlobillo force-pushed the precreated_fip branch 2 times, most recently from 3837ebe to a836a15 Compare December 9, 2022 14:18
@rlobillo
Copy link
Contributor Author

rlobillo commented Dec 9, 2022

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 15, 2022

@rlobillo: Overrode contexts on behalf of rlobillo: ci/prow/test-kuryr

In response to this:

@MaysaMacedo @dulek last UDP LB test to merge in master for this round. Please take a look.

/override ci/prow/test-kuryr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2022
@rlobillo rlobillo force-pushed the precreated_fip branch 2 times, most recently from b76fdec to 743ce40 Compare December 16, 2022 07:51
@rlobillo
Copy link
Contributor Author

/override ci/prow/test-kuryr

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2022

@rlobillo: Overrode contexts on behalf of rlobillo: ci/prow/test-kuryr

In response to this:

/override ci/prow/test-kuryr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

return network.ID, nil
}
} else {
return network.ID, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You still wants to return if subnet is not accessible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in fact that is the typical case: The external network is created by the openstack admin with --no-share true so the tenant cannot inspect the external subnets. Please refer to kubernetes/cloud-provider-openstack#2014

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright!

@@ -419,3 +478,56 @@ func createTestDeployment(depName string, labels map[string]string, replicas int
}}
return testDeployment
}

// GetFloatingNetworkID returns a floating network ID.
func GetFloatingNetworkID(client *gophercloud.ServiceClient) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this mimics how the cloud provider gets the ID of the floating IP network? What happens if it's forced to be something else in the configuration? IMO test should check the config too. Or you should pass your own network as an annotation when creating the service, but then it's probably not guaranteed the connectivity will always work if you choose something inappropriate for the setup.

Copy link
Contributor Author

@rlobillo rlobillo Dec 16, 2022

Choose a reason for hiding this comment

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

Exactly, these functions are highly inspired by https://github.com/openshift/cloud-provider-openstack/blob/master/pkg/util/openstack/network.go#L53

The intention of this test is to validate that user can set a previously created FIP on the svc definition, so CCM does not need to create it at all, and it just makes use of it.

Therefore, what is defined on the cloud-provider-config should be irrelevant in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rlobillo: What if there's only one "correct" FIP subnet that can be accessed from where the tests are run and it is set in the config? If autodetection takes incorrect one the connectivity check will fail. I'm just trying to find the corner cases so that when some job with a specific configuration will be run we won't need to debug why this test fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dulek: OK! the test will use the subnet on cloud-provider-config in case it's configured and, if not, auto-detect. I'm on it.

Covers partially the epic https://issues.redhat.com/browse/OSASINFRA-2753

Test that creates a FIP on openstack and use it for creating an UDP svc
type:LoadBalancer that sets on its spec the pre-created FIP.

This test will be skipped for Kuryr NetworkType and will run with both
amphora and OVN provider.
@dulek
Copy link
Contributor

dulek commented Dec 22, 2022

/lgtm
/approve
/hold wait for CI

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2022

@rlobillo: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dulek
Copy link
Contributor

dulek commented Dec 22, 2022

/hold cancel

Looking good!

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2022
@openshift-merge-robot openshift-merge-robot merged commit faec6d4 into openshift:main Dec 22, 2022
@rlobillo
Copy link
Contributor Author

/cherry-pick release-4.12

@openshift-cherrypick-robot

@rlobillo: new pull request created: #70

In response to this:

/cherry-pick release-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants