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

Import Cluster: Changing from 2 options checkbox to 3 options radio button #788

Closed
julienlim opened this issue Feb 1, 2018 · 9 comments
Closed
Assignees

Comments

@julienlim
Copy link
Member

This enhancement and bug fix is spawned from Volume profile enabled/not-started info in tendrl #551, which has to do with confusion with enabling / disabling volume profiling during Import Cluster.

Related bugs that can provide further background information:

In addition to the 2 options of Enable Volume Profiling and Disable Volume Profiling, we need to allow for a 3rd option of Leave existing Volume Profiling configuration as-is (which may be a mix of volume profiling enabled and disabled within a given cluster).

Updated mockup for Import Cluster showing these 3 options in a radio button may be found at: Import Cluster Mockup.

Note: The expected behavior of the 3 options are as follow:

  • Enable volume profiling - all volumes have volume profiling enabled
  • Disable volume profiling - all volumes have volume profiling disabled
  • Keep current configuration (of volume profiling) - leave existing volume profiling on the volumes as-is

See Volume profile enabled/not-started info in tendrl #551 for further details.

@gnehapk @a2batic @cloudbehl @mcarrano

@julienlim julienlim changed the title Import Cluster Changing from 2 options checkbox to 3 options radio button Import Cluster: Changing from 2 options checkbox to 3 options radio button Feb 1, 2018
@gnehapk
Copy link
Member

gnehapk commented Feb 5, 2018

Depends on Tendrl/api#350

@shtripat
Copy link
Member

shtripat commented Feb 6, 2018

Depends on Tendrl/gluster-integration#553

@shtripat
Copy link
Member

shtripat commented Feb 15, 2018

Based on @julienlim 's suggestion to have 3 options while import cluster flow for volume profiling enable/disable/leave as-is, I would suggest below changes in backend data model

Have two fields namely volume_profiling_flag and volume_profiling_state in Cluster object. The option selected while import cluster flow, would be maintained under volume_profiling_flag of cluster object which will have possible values of enable/disable/leave-as-is. Now as part of cluster data sync by GI, the value of volume_profiling_state at cluster level would be set based on the current state of volume reporting from CLI. The logic could be something like

if cluster.volume_profiling_flag == "enable":
    Enable profiling for all the volumes underlying
    cluster.volume_profiling_state = 'enabled'
if cluster.volume_profiling_flag == "disable":
    Disable profiling for all the volumes underlying
    cluster.volume_profiling_state = "disabled"
if cluster.volume_profiling_flag == "leave-as-is":
    Check the profiling state of underlying volumes and based on that
    if profiling enabled for all the volumes:
        cluster.volume_profiling_state = 'enabled'
    if profiling not enabled for any of the volumes:
        cluster.volume_profiling_state = 'disabled'
    if profiling enabled for few volumes:
        cluster.volume_profiling_state = 'mixed'
    Mark the volume.profiling_enabled field based on profiling enabled for underlying volume
cluster.save

UI should use the field volume_profiling_state for cluster and profiling_enabled for volume to show details in views.

@r0h4n @nthomas-redhat @julienlim @GowthamShanmugam comments?? suggestions??

@shtripat
Copy link
Member

For backward compatibility, with older versions of glusterfs, as get-state doesnt report the profiling enabled information for volumes, all the volume.profiling_enabled would be set as None. In such a scenarios if user selects leave-as-is while import cluster for profiling, the value of volume_profiling_state for cluster could be marked as unkown as in reality its not known to us in this case.

@julienlim
Copy link
Member Author

@shtripat @gnehapk @a2batic @nthomas-redhat @GowthamShanmugam @r0h4n @Tendrl/tendrl-qe

Ack. Looks good to me, and +1 on how to deal with older version of glusterfs that don't have the get-state for volume profiling support.

@gnehapk
Copy link
Member

gnehapk commented Feb 28, 2018

@julienlim @mcarrano @shtripat @nthomas-redhat @r0h4n How "Enable/Disable Profiling" action should work for cluster's mixed volume profiling status?

mixed-vol-profile

@julienlim
Copy link
Member Author

@julienlim @mcarrano @shtripat @nthomas-redhat @r0h4n @gnehapk

Good question @gnehapk.

At the cluster level (after import), if supported by get-state, when enable profiling performed, volume profiling should be enabled on all volumes in the cluster even if it's mixed.

If disable profiling performed, then volume profiling should be disabled on all volumes in the cluster even if it's mixed.

If mixed and user wants to retain the existing mixed profiling, the user should not enable/disable the profiling. I think we may need to provide some kind "i" (infotip) or document this somehow.

Thoughts?

@shtripat
Copy link
Member

shtripat commented Mar 1, 2018

Sorry fir just a thumps up above.
So I am in agreement with @julienlim here.

If after import the cluster level value if mixed we should have both enable and disable options available at cluster level. Back-end would take care of enabling/disabling accordingly as part sync for all the volumes of the cluster.

@gnehapk
Copy link
Member

gnehapk commented Mar 8, 2018

@julienlim Fixed with the milestone 3 release. Hence, closing it. Please open, if you see any issue,

@gnehapk gnehapk closed this as completed Mar 8, 2018
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

No branches or pull requests

4 participants