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

AdditionalVolumes to allow VolumeSource #487

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Jan 25, 2023

What this PR does:
Adds the abiliity to make AdditionalVolumes take VolumeSource as input instead of only PVCSpec.

Breaks the API by making PVCSpec a pointer to struct instead of struct..

And allows the modification of AdditionalVolumes in the webhook

Which issue(s) this PR fixes:
Fixes #467
Fixes #486

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@burmanm burmanm requested a review from a team as a code owner January 25, 2023 18:12
@Miles-Garnsey
Copy link
Member

Breaks the API by making PVCSpec a pointer to struct instead of struct..

This shouldn't be a problem for the yaml API right, because it just means that the PVCSpec becomes optional in the yaml? And a required -> optional change is always backwards compatible..?

I am a little more concerned about the impact it might have on the k8ssandra-operator side of things, since the move from a value -> pointer type will indeed break things (e.g. here). Do you have any suggestions on how to handle that?

I don't get the sense that we have many users accessing the APIs via the Go types, and given this is a beta API I don't think we have any obligation to maintain exact backwards compatibility even if we did (based on this). However, I think we should probably consider bumping the version to v1beta2 if we're going to make a breaking change, since this will signal the breaking change to users.

Finally, is there a reason that this field is better than accessing the podTemplateSpec directly? My understanding was that making these changes via PodTemplateSpec was becoming more accepted around the team.

@burmanm
Copy link
Contributor Author

burmanm commented Jan 27, 2023

This shouldn't be a problem for the yaml API right, because it just means that the PVCSpec becomes optional in the yaml? And a required -> optional change is always backwards compatible..?

It does not break YAML, that's true.

I am a little more concerned about the impact it might have on the k8ssandra-operator side of things, since the move from a value -> pointer type will indeed break things (e.g. here). Do you have any suggestions on how to handle that?

I have no idea why AdditionalVolumes is called "PVCs" in the K8ssandraCluster. That makes no sense. But why would it break that? The AdditionalVolumes struct hasn't changed type.

However, I think we should probably consider bumping the version to v1beta2 if we're going to make a breaking change, since this will signal the breaking change to users.

We haven't traditionally bumped to v1beta2 with other breaking changes either. That would require modifying webhook because that would change the YAML format also. And since YAML isn't breaking in this case, I don't think there's a reason to bump it to v1beta2. It only affects the Go API, but changing its package is not probably a very meaningful thing to do.

Finally, is there a reason that this field is better than accessing the podTemplateSpec directly? My understanding was that making these changes via PodTemplateSpec was becoming more accepted around the team.

cass-operator does not provide any PodTemplateSpec guarantees or backwards compatibility. We can and do break it (it's not passed as is and has never been) and have never recommended it. Configuring this type of Volume through PodTemplateSpec is cumbersome for users (requiring both volumes and volumesources). For YAML users it's often just a "hack" to get a missing feature.

@burmanm burmanm merged commit 43d6635 into k8ssandra:master Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants