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

Added an extra verbosity level to paasta_metastatus #155

Closed
wants to merge 14 commits into from

Conversation

mjksmith
Copy link
Contributor

Uses Krall's mesos slave analysis script to display free CPU and RAM on each host. Called by using paasta metastatus -vv (or --verbose --verbose).

dest="verbose",
default=False,
default=0,
help="Print out more output regarding the state of the cluster",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you adjust the help output to hint that there can be more?

dest="verbose",
default=False,
help="Print out more output regarding the state of the cluster",
default=0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this to the itest to ensure we are invoking this new code path on a real box?

@solarkennedy
Copy link
Contributor

Just for record keeping, this PR is waiting for itest + review feedback fixes.

@Rob-Johnson
Copy link
Contributor

I don't really like the logic for 'extra' checks being printed in main(), here's my suggestion for how to implement what you need:

Add a boolean parameter like verbose_checks to get_mesos_status. Decide if this should be set to True or False on the value ofverbose in main.

Add a check named assert_slave_stats to the list of checks performed by get_mesos_state.

def assert_slave_stats(mesos_state):
    header_string = "%12s %36s %9s' % ('Hostname', 'CPU free', 'RAM free')"
    slave_outputs = ['%40s %8.2f %9.2f' % (slave['hostname'], slave['free_resources']['cpus'], slave['free_resources'],slave['mem']) 
                     for slave in get_extra_mesos_slave_data(mesos_state)]
    check_output = [header_string].extend(slave_outputs)
    return ("\n".join(check_output),True)

N.B the second value of the tuple returned by the assert_* functions indicates if this check is 'healthy'. If there is anything that this check shows that is worth paging on (I don't think there is), then we can add logic to decide if we should return True/False accordingly.

modify get_mesos_state to perform this check based on the verbose_checks parameter.

get_mesos_state will end up looking something like:

def get_mesos_status(mesos_state, run_verbose_checks=False):
    """Gathers information about the mesos cluster.
       :return: tuple of a string containing the status and a bool representing if it is ok or not
    """

    cluster_results = run_healthchecks_with_param(mesos_state, [assert_quorum_size, assert_no_duplicate_frameworks])

    metrics = get_mesos_stats()
    check_functions =  [
        assert_cpu_health,
        assert_memory_health,
        assert_tasks_running,
        assert_slave_health
    ]

    if run_verbose_checks:
       check_functions.append(assert_slave_stats)
    metrics_results = run_healthchecks_with_param(metrics, check_functions)

    return cluster_results + metrics_results

By doing this, we move the logic for 'whats verbose' away from main, and the 'verbose' checks become regular healthchecks

I'm happy to pair with you on this if you like, too.

@@ -286,11 +285,36 @@ def print_results_for_healthchecks(summary, ok, results, verbose):
print_with_indent(line, 2)


def get_extra_mesos_slave_data(mesos_state):
slaves = dict((slave['id'], slave) for slave in mesos_state['slaves'])
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need the whole slave dict as the value here? how about:

dict((slave['id'], {}) for slave in mesos_state['slaves'])

that way, in the below logic, you're won't be modifying the slave dict.

@mjksmith mjksmith force-pushed the PAASTA-1883 branch 2 times, most recently from 1ca9bed to d5fd644 Compare January 14, 2016 00:43
@then(u'paasta_metastatus exits with return code "{expected_return_code}" and output "{expected_output}"')
def check_metastatus_return_code(context, expected_return_code, expected_output):
@then(u'paasta_metastatus {flags} exits with return code "{expected_return_code}" and output "{expected_output}"')
def check_metastatus_return_code(context, flags, expected_return_code, expected_output):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check what happens when flags is empty?

Then paasta_metastatus exits with..

Will flags just be an empty string?

@Rob-Johnson
Copy link
Contributor

fix n ship - great stuff 👍

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