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.

Sign up for GitHub

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

Jump to bottom

mgr/dashboard: add smb service management support #57134

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

mgr/dashboard: add smb service management support #57134

wants to merge 1 commit into from
+252 −2

Conversation

Pegonzal
Copy link
Contributor

@Pegonzal Pegonzal commented Apr 29, 2024 •
edited

image

screen-capture.45.webm

Fixes: https://tracker.ceph.com/issues/65681

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@Pegonzal Pegonzal requested a review from a team as a code owner April 29, 2024 08:49
@Pegonzal Pegonzal requested review from cloudbehl and avanthakkar and removed request for a team April 29, 2024 08:49
ivoalmeida
ivoalmeida approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@ivoalmeida ivoalmeida left a comment

Choose a reason for hiding this comment

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

lgtm

avanthakkar
avanthakkar reviewed Apr 29, 2024
View reviewed changes
Copy link
Contributor

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

LGTM functionally!
Can you please also add a unit test for smb service creation

@cloudbehl
Copy link
Contributor

@Pegonzal Thanks for PR.

  1. Can we bring required items on top.
  2. Can you add example placeholders for all items.
  3. Config uri => Config URI
  4. ceph users could be selectable rather than entering manually.
  5. Features we have only one(domain), can that be a list and can domain feature be enabled by default? @phlogistonjohn can you help suggest, if its a ip to have otherwise we can remove the field?

@phlogistonjohn can you help, what things from your perspective could be made easier for user by automating or having preselected values?

@phlogistonjohn
Copy link
Contributor

Hi @cloudbehl this feature is really meant to support the (recently merged) smb mgr module and is not really meant to end users to interact with directly. @Pegonzal needs to make the existing service spec support complete but should only be done minimally IMO. I wouldn't go too far in making it "friendly" just do enough to be complete. If you are an "expert" in the samba-containers project you could use this service spec directly but that's a vanishingly small number of people.

If you want, features can be a multi-select or group of checkboxes (or whatever your framework calls it). The enumeration for 'features' is currently either domain or nothing but future versions will likely add metrics and cluster (names not set in stone). But it'll always be a list of "enums". The default should be the equivalent of an "empty list". The features list tells cephadm what specific grouping of containers will be needed.

The smb mgr module PR also had to add an additional field. user_sources (link) that works similarly to join_sources - it takes a list of URI values that sambacc will use to download user configuration from.

@Pegonzal
Copy link
Contributor Author

Can you please also add a unit test for smb service creation

Added it

Can we bring required items on top.
Can you add example placeholders for all items.
Config uri => Config URI

All addressed

ceph users could be selectable rather than entering manually.

Created a tracker to add this new feature in the forms that might need it https://tracker.ceph.com/issues/66032

Also added the user_sources field and changed the features field to use checkboxes, so the user can select any if needed.

@Pegonzal
Copy link
Contributor Author

jenkins test api

@Pegonzal
Copy link
Contributor Author

jenkins test dashboard

@Pegonzal
mgr/dashboard: add smb service management support
7b34af8 
Fixes: https://tracker.ceph.com/issues/65681
Signed-off-by: Pedro Gonzalez Gomez <pegonzal@redhat.com>
@Pegonzal
Copy link
Contributor Author

jenkins test dashboard

cloudbehl
cloudbehl approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

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

minor comments, mostly looks good.

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html
placeholder="rados://.smb/foo/scc.toml"
i18n-placeholder>
<span class="invalid-feedback"
*ngIf="serviceForm.showError('config_uri', frm, 'required')"
Copy link
Contributor

Choose a reason for hiding this comment

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

good to have validation for url based on Supported URI schemes include http:, https:, rados:, and rados:mon-config-key:.

...gr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html
<label class="cd-col-form-label required"
for="cluster_id"
i18n>
Cluster id
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Cluster id
Cluster name

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets call it name, ID is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

or something like this: Cluster ID(Name)

@cloudbehl cloudbehl requested a review from nizamial09 May 22, 2024 06:59
nizamial09
nizamial09 approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

LGTM! can you add an e2e test in the cephadm dashboard e2e suite for the service creation? Since we don't expect users to use this interface to frequently there's a good chance that we can miss out if this is broken unintentionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloudbehl cloudbehl cloudbehl approved these changes

@ivoalmeida ivoalmeida ivoalmeida approved these changes

@avanthakkar avanthakkar avanthakkar approved these changes

@nizamial09 nizamial09 nizamial09 approved these changes

Assignees
No one assigned
Labels
dashboard pybind
Projects
Ceph-Dashboard
Status: Reviewer approved
Milestone
No milestone
6 participants
@Pegonzal @cloudbehl @phlogistonjohn @ivoalmeida @avanthakkar @nizamial09