Skip to content

Fix v1alpha6 -> v1alpha7 idempotence #1626

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 11, 2023
Merged

Fix v1alpha6 -> v1alpha7 idempotence #1626

merged 2 commits into from
Sep 11, 2023

Conversation

@mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Jul 31, 2023 •
edited
Loading

This fixes a class of bug where we would incorrectly attempt to restore data during down-conversion of an object which had been modified during up-conversion. We fix it by storing a hash alongside the restore data so we know when the object has been modified.

Fixes #1606

/hold

@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/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 31, 2023
@k8s-ci-robot
Copy link
Contributor

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

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

@netlify
Copy link

netlify bot commented Jul 31, 2023 •
edited
Loading

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

Name Link
🔨 Latest commit 9f1f091
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64fb536e50610a00082af58d
😎 Deploy Preview https://deploy-preview-1626--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 31, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 31, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 31, 2023
@mdbooth mdbooth force-pushed the issue1606 branch 2 times, most recently from 919db73 to 456c8e7 Compare August 2, 2023 15:37
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 6, 2023
@mdbooth mdbooth marked this pull request as ready for review September 6, 2023 09:11
@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 Sep 6, 2023
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 6, 2023

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

@k8s-ci-robot
Copy link
Contributor

@mdbooth: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-openstack-build
  • /test pull-cluster-api-provider-openstack-e2e-test
  • /test pull-cluster-api-provider-openstack-test

The following commands are available to trigger optional jobs:

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

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-openstack-build
  • pull-cluster-api-provider-openstack-e2e-test
  • pull-cluster-api-provider-openstack-test

In response to this:

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

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.

@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 6, 2023

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

@mdbooth mdbooth mentioned this pull request Sep 6, 2023
Open
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 6, 2023

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

1 similar comment
@lentzi90
Copy link
Contributor

lentzi90 commented Sep 6, 2023

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

@mdbooth mdbooth mentioned this pull request Sep 7, 2023
Merged
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 7, 2023

I'm hoping the full test failure will be fixed by #1659

@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

Now trying #1661

lentzi90
lentzi90 reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

This looks alright to me, let's see if we can get the e2e-full test happy with it

lentzi90
lentzi90 reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2023
@mdbooth
Fix down-conversion after modification in up-conversion
d70962e 
This fixes a class of bug where we would incorrectly attempt to restore
data during down-conversion of an object which had been modified during
up-conversion. We fix it by storing a hash alongside the restore data so
we know when the object has been modified.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2023
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

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

@mdbooth
Add a network definition to v1alpha6 e2e templates
9f1f091 
This ensures an API transformation is required when upgrading.
@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

I've just pushed an extra commit which adds an explicit network to the v1alpha6 OpenStackMachineTemplates, and therefore means the upgrade test has to do an actual API transformation.

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

@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

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

@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

Converted correctly with the expected conversion data, e.g.: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/1626/pull-cluster-api-provider-openstack-e2e-full-test/1700192834843316224/artifacts/clusters/clusterctl-upgrade-hv5q5k/resources/clusterctl-upgrade/OpenStackMachineTemplate/clusterctl-upgrade-38hkl1-control-plane.yaml

@mdbooth
Copy link
Contributor Author

mdbooth commented Sep 8, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2023
lentzi90
lentzi90 reviewed Sep 11, 2023
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1d92762 into kubernetes-sigs:main Sep 11, 2023
@mdbooth mdbooth deleted the issue1606 branch September 11, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@jichenjc jichenjc Awaiting requested review from jichenjc

@tobiasgiese tobiasgiese Awaiting requested review from tobiasgiese

Assignees

@lentzi90 lentzi90

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Runtime error with v1alpha6 conversion on v0.8.0-alpha.0

3 participants

@mdbooth @k8s-ci-robot @lentzi90