Skip to content

feat(nfs-server): provision nfs server resource in user provided namespace#58

Merged
kmova merged 12 commits intoopenebs-archive:developfrom
mynktl:nfs-server-ns
Jul 8, 2021
Merged

feat(nfs-server): provision nfs server resource in user provided namespace#58
kmova merged 12 commits intoopenebs-archive:developfrom
mynktl:nfs-server-ns

Conversation

@mynktl
Copy link
Copy Markdown
Contributor

@mynktl mynktl commented Jul 5, 2021

Why is this PR required? What issue does it fix?:
As of now, nfs-provisioner creates nfs-server resources in default 'openebs' namespace. This resource can be isolated from operator namespace.

What this PR does?:
This PR fixes the above issue. With these PR changes, nfs-provisioner will create the nfs-server resources in the namespace mentioned in OPENEBS_IO_NFS_SERVER_NS env variable in nfs-provisioner deployment. If OPENEBS_IO_NFS_SERVER_NS is not mentioned or is having empty value then nfs-server resources will be created in the operator namespace which is openebs.

Does this PR require any upgrade changes?:

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

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:

mayank added 5 commits July 5, 2021 11:50
  nfs-provisioner deployment can be configured with env NFSServerNamespace
  with namespace value to create nfs-server deployment in a provided namespace.

  Note: This requires nfs-provisioner should have permission to create/delete deployment,
  service, PVC resource in the provided namespace.

Signed-off-by: mayank <mayank.patel@mayadata.io>
Signed-off-by: mayank <mayank.patel@mayadata.io>
Signed-off-by: mayank <mayank.patel@mayadata.io>
Signed-off-by: mayank <mayank.patel@mayadata.io>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #58 (f4eca84) into develop (0e0a870) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #58   +/-   ##
========================================
  Coverage    48.37%   48.37%           
========================================
  Files           18       18           
  Lines         1598     1598           
========================================
  Hits           773      773           
  Misses         781      781           
  Partials        44       44           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e0a870...f4eca84. Read the comment docs.

@mynktl mynktl requested review from kmova and mittachaitu July 5, 2021 15:26
value: "openebs-operator-nfs"
# OPENEBS_IO_NFS_SERVER_NS defines the namespace for nfs-server deployment
#- name: OPENEBS_IO_NFS_SERVER_NS
# value: "default"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is the default - default namespace or the namespace where openebs is installed. Probably need to change this "openebs" for better clarity.

}

func getNfsServerNamespace() string {
return menv.Get(NFSServerNamespace)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This probably should handle the case where NFSServerNamespace is not present in the YAML and return the same value configured to getOpenEBSNamespace if empty.

return nil, fmt.Errorf("Cannot start Provisioner: failed to get namespace")
}

nfsServerNs := getNfsServerNamespace()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you can move this code into the menv file as per the previous comment

mayank added 3 commits July 6, 2021 10:34
Signed-off-by: mayank <mayank.patel@mayadata.io>
Signed-off-by: mayank <mayank.patel@mayadata.io>
@mynktl mynktl requested a review from kmova July 6, 2021 05:11
mayank added 2 commits July 6, 2021 10:56
Signed-off-by: mayank <mayank.patel@mayadata.io>
Signed-off-by: mayank <mayank.patel@mayadata.io>
Copy link
Copy Markdown
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

provided few minor comments

Signed-off-by: mayank <mayank.patel@mayadata.io>
@mynktl mynktl requested a review from mittachaitu July 7, 2021 11:43
@mynktl
Copy link
Copy Markdown
Contributor Author

mynktl commented Jul 7, 2021

Hi @kmova @mittachaitu
I've updated the PR as per review comments. PTAL.
Thanks

Copy link
Copy Markdown
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

changes are good

Signed-off-by: mayank <mayank.patel@mayadata.io>
@mynktl mynktl requested a review from kmova July 8, 2021 06:04
@kmova kmova merged commit e0a9e18 into openebs-archive:develop Jul 8, 2021
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