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

A custom slimmed down version of VolumeSource #570

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

dprince
Copy link
Contributor

@dprince dprince commented Oct 14, 2024

Slim down VolumeSource by removing deprecated and "removed" fields from the struct. This will slim down our nested use of ExtraMounts.

Using this version of the struct shaves 38k off the cinder-operator bundle size. Using this across the board would have a significant effect on reducing the OpenStackControlplane CRD size and little if any effect on utility of the nested ExtraMount struct.

@dprince
Copy link
Contributor Author

dprince commented Oct 14, 2024

A preview of the effect on cinder-operator is here: dprince/cinder-operator@95e834d

@dprince dprince requested review from fmount and olliewalsh October 14, 2024 21:49
@dprince
Copy link
Contributor Author

dprince commented Oct 14, 2024

@olliewalsh This is similar to the StatefulSet idea you mentioned from Rabbit.

@dprince
Copy link
Contributor Author

dprince commented Oct 14, 2024

I removed zz_generated.deepcopy.go as the Makefile here doesn't seem to regenerate it. (just stale). Did something get broken recently?

Copy link
Contributor

@bshephar bshephar left a comment

Choose a reason for hiding this comment

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

We might want to leave additional things to allow the most customer possibilities. But just floating the idea of removing a few more things if they aren't explicitly supported by OpenShift.

modules/storage/storage.go Outdated Show resolved Hide resolved
modules/storage/storage.go Outdated Show resolved Hide resolved
@dprince
Copy link
Contributor Author

dprince commented Oct 15, 2024

@bshephar the more we can agree to remove the better.

modules/storage/storage.go Outdated Show resolved Hide resolved
@fmount
Copy link
Contributor

fmount commented Oct 15, 2024

I removed zz_generated.deepcopy.go as the Makefile here doesn't seem to regenerate it. (just stale). Did something get broken recently?

Not sure, but [1] should solve your problem here.

[1] https://paste.opendev.org/show/bUqTCs84vk01M2PefMn0/

@fmount
Copy link
Contributor

fmount commented Oct 15, 2024

@dprince an additional change we need to consider across all the operators that implement extraMounts is the way we access corev1.Volumes.

We usually append( ... a corev1.Volume to an array of the same type, and we need to patch functions like [1] to work with the Volumes struct defined in this module (but Make will tell you more about this comment).

With the goal in mind of reducing the bundle size, I think we're going in the right direction, and I can follow up w/ a glance-operator change (because it's simple) that is based on this patch.
If everyone agrees on that we can then prepare changes for all the operators implementing this new version.

[1] https://github.com/openstack-k8s-operators/cinder-operator/blob/main/pkg/cindervolume/volumes.go#L30

@dprince
Copy link
Contributor Author

dprince commented Oct 15, 2024

I removed zz_generated.deepcopy.go as the Makefile here doesn't seem to regenerate it. (just stale). Did something get broken recently?

Not sure, but [1] should solve your problem here.

[1] https://paste.opendev.org/show/bUqTCs84vk01M2PefMn0/

I'm not sure how the storage modules deepcopy was ever maintained. It is missing the needed kubebuilder annotations to do so

@dprince
Copy link
Contributor Author

dprince commented Oct 15, 2024

@dprince an additional change we need to consider across all the operators that implement extraMounts is the way we access corev1.Volumes.

We usually append( ... a corev1.Volume to an array of the same type, and we need to patch functions like [1] to work with the Volumes struct defined in this module (but Make will tell you more about this comment).

With the goal in mind of reducing the bundle size, I think we're going in the right direction, and I can follow up w/ a glance-operator change (because it's simple) that is based on this patch. If everyone agrees on that we can then prepare changes for all the operators implementing this new version.

[1] https://github.com/openstack-k8s-operators/cinder-operator/blob/main/pkg/cindervolume/volumes.go#L30

I updated my cinder PR. Wanted to get general feedback on this before going too far but I think it should work out fine.

@dprince dprince force-pushed the custom_vol_source branch 2 times, most recently from e89d57e to e3abf73 Compare October 16, 2024 01:12
modules/storage/storage.go Outdated Show resolved Hide resolved
Slim down VolumeSource by removing deprecated and "removed"
fields from the struct. This will slim down our nested
use of ExtraMounts

Also, adds ConvertVolumeSource function to the storage module
which converts from VolumeSource to a corev1.VolumeSource
Probably not likely we'll be using these:

PhotonPersistentDisk
Projected
Ephemeral
Copy link
Contributor

@stuggi stuggi left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@fmount fmount left a comment

Choose a reason for hiding this comment

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

/lgtm

@dprince dprince merged commit 30baa23 into openstack-k8s-operators:main Oct 25, 2024
2 checks passed
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.

5 participants