Multi vm affinity e2e #196
Conversation
…n number of available hosts. Added alternate e2e config file for 10.11.0.2.
… param, allowing override of the default test/e2e/config/cloudstack.yaml.
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
| ph.SetResourcestate("Enabled") | ||
| ph.SetState("Up") | ||
| listHostsResponse, err := client.Host.ListHosts(ph) | ||
| Expect(err).To(BeNil(), "error listing hosts") |
There was a problem hiding this comment.
do the contents of err get logged here? That'd be useful for troubleshooting.
There was a problem hiding this comment.
Yes:
error listing zones Expected <*errors.errorString | 0xc00154abe0>: { s: "CloudStack API error 401 (CSExceptionErrorCode: 0): unable to verify user credentials and/or request signature", } to be nil
test/e2e/affinity_group.go
Outdated
| @@ -79,6 +79,17 @@ func AffinityGroupSpec(ctx context.Context, inputGetter func() CommonSpecInput) | |||
| } | |||
|
|
|||
| func executeTest(ctx context.Context, input CommonSpecInput, namespace *corev1.Namespace, specName string, clusterResources *clusterctl.ApplyClusterTemplateAndWaitResult, affinityType string) []string { | |||
| csClient := CreateCloudStackClient(ctx, input.BootstrapClusterProxy.GetKubeconfigPath()) | |||
| zoneName := input.E2EConfig.GetVariable("CLOUDSTACK_ZONE_NAME") | |||
There was a problem hiding this comment.
is only one zone supported? Can users not pass in a config with multiple zones?
There was a problem hiding this comment.
Yes, as of today there's only a single-valued CLOUDSTACK_ZONE_NAME param defined in the config, and referenced in the cluster templates. None of the test scenarios/cluster templates so far define more than a single failure domain. Supporting multiple zones in multiple FDs would require some changes.
… secret format. Removed redundant int64 cast. Clarified error message if wrong number of zones for a name found.
test/e2e/affinity_group.go
Outdated
| if numHosts < 3 { | ||
| // If not enough ACS hosts to support 3 machines with anti-affinity, fall back to just allocating one machine. | ||
| // Test will only confirm that the single VM is in an anti-affinity group, but not that ACS anti-affinty works. | ||
| By("Fewer than three host in ACS env. Only verifying that a single VM gets placed in an AG, not that true affinity/anti-affinity is ultimately achieved.") |
There was a problem hiding this comment.
I think we should explicitly skip this test if there are <3 hosts. This way is very subtle and tests some plumbing code but not the full vertical feature. If we skip the test with a message explaining why, then the user knows they should address it in their environment.
There was a problem hiding this comment.
Done. That makes the most sense. Thanks for that Max.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jweite-amazon, maxdrib 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 |
k8s-ci-robot
added
size/M
Issue #, if available:
Description of changes:
Changes to E2E affinity tests to support true affinity testing if ACS env has three or more hosts in the zone being used for the test. If fewer than three hosts are available, falls back to the pre-existing one-VM test, which only verifies that the VM is placed in the appropriate affinity group.
Testing performed:
Executed test in single host and 3-host environments, observing it allocating the one or three VMs respectively for the CP and workers. Tests pass in both cases.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.