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

Extend volumes #319

Closed
wants to merge 2 commits into from
Closed

Extend volumes #319

wants to merge 2 commits into from

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Jan 20, 2021

What does this PR do?

Implements extensions to Volume components according to proposal in #310. Not a final design; I'm hoping for more discussion before merge. I've added this topic to the upcoming call to discuss more widely.

What issues does this PR fix or reference?

Closes #310

Is your PR tested? Consider putting some instruction how to test your changes

N/A?

Add fields to the Volume Component type to allow specifying:
* Whether persistent storage should be used for this Volume (enabling
provisioning of emptyDir volumes)
* An external volume that should exist outside of the lifecycle of
container components; allows specifying an existing
PersistentVolumeClaim, ConfigMap, or Secret to be attached to the
container component.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

I'm OK with the made proposal


const (
// StorageVolumeType specifies persistent storage, e.g. a PersistentVolumeClaim
StorageVolumeType ExistingVolumeType = "storage"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what is drawback if we name it with k8s term (as we do for other types), e.g. pvc, or persistentvolumeclaim (I know it's too long).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PVC probably makes sense to make clear what we're planning to do with the information provided. I chose "storage" as it's more generic, but it may complicate things if we need to extend this in the future.

Copy link
Member

Choose a reason for hiding this comment

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

What about persistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied this change in #325

// delete.
// +optional
// +kubebuilder:default=true
Persistent bool `json:"persistent,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Can it be used together with External? Should we deny it on the implementation level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. One thing we discussed is that semantically it only makes sense to have either [persistent, size] or [external] but not both.

I also need to remove the readonly parameter for now, it doesn't make sense in its current spot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can revisit this topic once #324 is merged and if we decide to go ahead on #325.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, sleshchenko
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found 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

@amisevsk amisevsk mentioned this pull request Jan 26, 2021
@amisevsk
Copy link
Contributor Author

amisevsk commented Jan 26, 2021

I'm closing this PR in favor of #324 and #325 which represent the same changes but as separate PRs. I think #324 (ephemeral/non-persistent volumes) makes sense to merge but #325 (external volumes) deserves more discussion, as pointed out by @l0rd.

@amisevsk amisevsk closed this Jan 26, 2021
@amisevsk amisevsk deleted the extend-volumes branch January 26, 2021 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending Volume components to support more features
4 participants