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

feat (provisioner-localpv): Merge CAS config from PersistentVolumeClaim #144

Closed
wants to merge 1 commit into from

Conversation

nobiit
Copy link
Contributor

@nobiit nobiit commented Sep 25, 2022

Pull Request template

Why is this PR required? What issue does it fix?:
Allows users to filter the type of storage they want right from the PVC. Instead of having to create multiple StorageClass types for infrequent volumes

What this PR does?:
Merge CAS from PersistentVolumeClaim and StorageClass

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

  • PVC has annotations attribute cas.openebs.io/config and it is included in BlockDeviceClaim

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@nobiit nobiit changed the title (feat) Merge CAS config from PersistentVolumeClaim (feat) provisioner-localpv: Merge CAS config from PersistentVolumeClaim Sep 25, 2022
@nobiit nobiit changed the title (feat) provisioner-localpv: Merge CAS config from PersistentVolumeClaim feat (provisioner-localpv): Merge CAS config from PersistentVolumeClaim Sep 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #144 (fd98f1a) into develop (e797585) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #144      +/-   ##
===========================================
- Coverage    38.00%   37.91%   -0.10%     
===========================================
  Files           36       36              
  Lines         3365     3373       +8     
===========================================
  Hits          1279     1279              
- Misses        2004     2012       +8     
  Partials        82       82              
Impacted Files Coverage Δ
cmd/provisioner-localpv/app/config.go 16.31% <0.00%> (-0.99%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

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

@nobiit -- Thank you for your contribution! I have few comments on your contribution.
Volume-level configuration should take precedence over StorageClass configuration, if the same configuration keys exist for both. PVC config should be merged into the PV config before the StorageClass config is merged.

Also, the key values from the first argument in the cast.MergeConfig() takes precedence over the ones in the second.
PTAL here https://github.com/openebs/dynamic-nfs-provisioner/blob/develop/provisioner/config.go#L118.

@niladrih
Copy link
Member

@nobiit -- The change in the above comment, is not required. Let's go with what you've implemented already. On second thought, the config in the SC should take precedence. The SC config is something a cluster admin may want to enforce.

Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

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

This change is required, otherwise LGTM.

if len(strings.TrimSpace(pvcCASConfigStr)) != 0 {
pvcCASConfig, err := cast.UnMarshallToConfig(pvcCASConfigStr)
if err == nil {
pvConfig = cast.MergeConfig(pvcCASConfig, pvConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pvConfig = cast.MergeConfig(pvcCASConfig, pvConfig)
// Config keys which already exist (SC config),
// will be skipped
// i.e. SC config will have precedence over PVC config,
// if both have the same keys
pvConfig = cast.MergeConfig(pvConfig, pvcCASConfig)

@niladrih
Copy link
Member

niladrih commented Jul 1, 2024

Closing this PR as these changes have been included in #190.

@niladrih niladrih closed this Jul 1, 2024
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.

3 participants