⚠️ Allow explicitly empty volume AZ

mdbooth commented 5 months ago

Replaces an AvailabilityZone string for volumes with a VolumeAvailabilityZone struct which allows more flexibility in defaulting behaviour. Specifically it enables us to express both the current default behaviour where we take the volume AZ from the Machine, and a new default behaviour where to don't specify a volume AZ at all.

In making this change to both RootVolume and AdditionalBlockDevices we use common code for both APIs. This has the result of updating RootVolume to be consistent with a volume specified via AdditionalBlockDevices.

Fixes #1649

/hold

k8s-ci-robot commented 5 months ago

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

netlify[bot] commented 5 months ago

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
Latest commit	9aa449c186b9cf1c5d2f4cb12bda0c622fe7e598
Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/661818600df7c600087335c8
Deploy Preview	https://deploy-preview-2008--kubernetes-sigs-cluster-api-openstack.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdbooth commented 5 months ago

That test failure confirms that the new default behaviour works :) I've updated the e2e test to specify the from: Machine behaviour.

mdbooth commented 5 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 5 months ago

Running the e2e tests again because there's some non-determinism in there: if we weren't correctly setting the target AZ, they might land in the right place anyway by accident.

/test pull-cluster-api-provider-openstack-test

mdbooth commented 5 months ago

The merge conflict is only in instance_test.go. The e2e-full-test passed before it was detected, so I won't bother running it again.

mdbooth commented 5 months ago

/hold cancel

mdbooth commented 5 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

huxcrux commented 5 months ago

/lgtm

Awesome that size on discs is specified in the same way for root and extra disks :)

mdbooth commented 5 months ago

/approve

k8s-ci-robot commented 5 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/OWNERS)~~ [mdbooth] 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 commented 5 months ago

@mdbooth: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-openstack-e2e-test	9aa449c186b9cf1c5d2f4cb12bda0c622fe7e598	link	unknown	`/test pull-cluster-api-provider-openstack-e2e-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

mdbooth commented 5 months ago

@mdbooth: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command pull-cluster-api-provider-openstack-e2e-test 9aa449c link unknown /test pull-cluster-api-provider-openstack-e2e-test Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

@lentzi90 This is worrying. This failure was a flake, and I'm reasonably confident we haven't broken it. However... I'm surprised it would merge a PR with a test failure. I'm assuming this additional test run is against the proposed merge commit, so I get why it runs. But why run it if it's going to merge anyway on failure?

lentzi90 commented 5 months ago

Oh yes this looks bad. I have no explanation for it. Could there be some higher level config in test-infra that runs it as a post-submit? I remember we removed our post-submits from here, but perhaps there is some config telling prow to rerun after merge? :thinking: The job is definitely marked as required so I don't see how it could merge except by some bug in prow

liggitt commented 5 months ago

https://prow.k8s.io/pr-history/?org=kubernetes-sigs&repo=cluster-api-provider-openstack&pr=2008 shows a pass of that test at that commit.

It got tested and merged in a batch merge

kubernetes-sigs / cluster-api-provider-openstack