Skip to content

update API Provider to v1beta1#399

Closed
anish-mudaraddi wants to merge 9 commits intoazimuth-cloud:mainfrom
anish-mudaraddi:update-api-provider
Closed

update API Provider to v1beta1#399
anish-mudaraddi wants to merge 9 commits intoazimuth-cloud:mainfrom
anish-mudaraddi:update-api-provider

Conversation

@anish-mudaraddi
Copy link
Copy Markdown
Contributor

An attempt to fix issue #398

update api provider to use v1beta1 since its provided in newest cluster api provider version 0.10.*

update api provider to use v1beta1 since its provided in newest cluster api provider version 0.10.*
Copy link
Copy Markdown
Contributor

@scrungus scrungus left a comment

Choose a reason for hiding this comment

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

I think this will require some additional changes, for example removing identityRef.kind:



and addition of cloudName.

Could you go through this changelog and make any necessary changes?

made further changes for openstackmachinetemplate and openstackcluster to to be compatible with v1beta1 api provider version
@anish-mudaraddi
Copy link
Copy Markdown
Contributor Author

I've made the necessary changes - and I've tested making a cluster with the changes and its working for me!

@scrungus
Copy link
Copy Markdown
Contributor

@anish-mudaraddi the etcd test is failing, looks like we need this change as well.

Copy link
Copy Markdown
Contributor

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

This PR is a good start, but while we are moving to a new API version I'd like to take the opportunity to adopt some of the new functionality. Some examples would be:

  • Moving the external network specification to a proper filter but still respecting the old-style ID for backwards compatibility
  • Supporting multiple subnets but still respecting the old-style single subnet configuration for backwards compatibility
  • ...

I understand that you want this quickly to correct your issues with Argo CD, but it seems like a good opportunity to also do these things.

@mkjpryor
Copy link
Copy Markdown
Contributor

mkjpryor commented Sep 16, 2024

It is also important that all of these changes are implemented in a backwards-compatible way. This means that where fields have changed their name, e.g. diskSize to sizeGiB for root volumes, we want to respect both (for the time being at least).

@anish-mudaraddi
Copy link
Copy Markdown
Contributor Author

@mkjpryor no problem - I'll try to implement the functionality you requested!

v1beta1 changes the names of the following for the rootVolume field:
   - diskSize to sizeGiB
   - volumeType to type

it also changes the way availabilityZone name is provided

This commit ensures that the changes are backwards compatible and deprecates the changes in values.yaml
allow users to specify additional security groups using managedSecurityGroups. Allow backwards compatibility with previous way which set it as a boolean
allow users to specify if they want to disable external networking for the cluster
allow users to utilise filter methods available in v1beta1 for specifying external network - ensured backwards compatibility with externalNetworkiD
v1beta1 allows specifying 2 subnets for dual stack configuration. It also adds the managedSubnets field. This commit adds these and also maintains backwards compatibility
allows specifying serverGroupFilter instead of serverGroupId for machine templates
but maintaining backwards compatibility
@anish-mudaraddi
Copy link
Copy Markdown
Contributor Author

anish-mudaraddi commented Sep 24, 2024

I've implemented the following changes:

  • removal of identityRef kind Secret

  • moving cloudName to identityRef.cloudName

  • added field ServerGroupFilter - (an alias for ServerGroup)

    • ServerGroupID is still supported for backwards compatibility.
    • I used ServerGroupFilter instead of ServerGroup to match existing naming convention used for defining subnets and networks - subnetFilter and networkFilter - but I can change this
  • added field externalNetworkFilter - (similar to above)

  • added fields managedSubnets and subnets to match upstream

    • old fields nodeCidr and dnsNameservers still supported
  • add field sizeGiB to replace diskSize

    • supporting both for backwards compatibility
  • added field allowedCIDRs - still supporting old field allowedCidrs

  • changes to the way machineImage and machineImageId are parsed to be compatible with new changes

  • availabilityZone field in v1beta1 has changed to no longer accept name string

    • I updated this but also support defining the name the old way
  • added disableExternalNetwork field

Bits I left out

  • a field like machineimageFilter - I don't think we need it since we can define an image via id and name currently which I think is enough

  • floatingIPPoolRef functionality - something for a future PR - I think its out-of-scope for this PR

@anish-mudaraddi
Copy link
Copy Markdown
Contributor Author

@mkjpryor @scrungus just bumping this

@mkjpryor
Copy link
Copy Markdown
Contributor

mkjpryor commented Oct 8, 2024

@anish-mudaraddi Can you rebase this onto main please? Thanks!

@mkjpryor
Copy link
Copy Markdown
Contributor

mkjpryor commented Oct 8, 2024

@anish-mudaraddi

I'm finding this PR very hard to review.

Thinking on it some more, I think I would prefer to split this change into two PRs:

  1. Move to v1beta1 resources with no changes to the values.yaml
  2. Introduce some of the new features, in a backwards compatible way

@mkjpryor
Copy link
Copy Markdown
Contributor

mkjpryor commented Oct 8, 2024

@anish-mudaraddi

I think #423 is the minimal set of changes needed to move to v1beta1 resources while supporting the existing values format. Can you please test and comment on that PR?

# The size of the disk to use in gibibytes (GiB)
# If not given, the ephemeral root disk from the flavor is used
diskSize:
sizeGiB:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, I fear some of these are going to break a lot of users' templates without some help to migrate between the old and new values here. 🤔 @mkjpryor any ideas on how to help with that?

@mkjpryor
Copy link
Copy Markdown
Contributor

Superseded by #423 for now

@mkjpryor mkjpryor closed this Oct 16, 2024
@anish-mudaraddi anish-mudaraddi deleted the update-api-provider branch October 16, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants