t176 #3329

waltdisgrace · 2024-01-17T15:58:35Z

No description provided.

kgaillot

Good start

include/crm/common/output.h

lib/common/output_xml.c

lib/pacemaker/pcmk_output.c

kgaillot · 2024-01-17T17:09:09Z

Changed my mind on validate vs verify, updated comment :)

kgaillot

Nice, it's getting there

lib/pacemaker/pcmk_output.c

cts/cli/regression.crm_mon.exp

kgaillot · 2024-01-31T22:22:06Z

cts/cli/regression.daemons.exp

@@ -27,13 +27,13 @@
  <shortdesc lang="en">Pacemaker controller options</shortdesc>
  <parameters>
    <parameter name="dc-version">
-      <longdesc lang="en">Includes a hash which identifies the exact revision the code was built from. Used for diagnostic purposes.</longdesc>
+      <longdesc lang="en">Includes a hash which identifies the exact changeset the code was built from. Used for diagnostic purposes.</longdesc>


This was updated in #3325. I'm not sure how you ended up with just some of the changes from that; maybe try exporting your commits as patches and reapply them to a clean branch.

kgaillot · 2024-02-01T15:34:21Z

Seeing the XML output reminds me that I forgot the schema portion of the crm_verify project :/ I'll open a new task for that ...

kgaillot · 2024-02-01T15:41:06Z

Seeing the XML output reminds me that I forgot the schema portion of the crm_verify project :/ I'll open a new task for that ...

Actually, crm_verify XML output only has status and errors, so it fits in the existing schemas. So, all we need to do is update the crm_mon schema for this project once it's ready.

kgaillot

This looks great.

Add crm_mon regression tests -- you can use one of the crm_verify_invalid*xml files to test invalid syntax and crm_mon.xml to test valid syntax. Test text and XML for each using defaults, and also test --include=verifications for text+valid and --exclude=verifications for XML+invalid

Is there anything left you're unsure about?

lib/pacemaker/pcmk_output.c

include/crm/common/output.h

lib/pacemaker/pcmk_output.c

tools/crm_mon.c

kgaillot · 2024-03-18T17:37:25Z

Add crm_mon regression tests -- you can use one of the crm_verify_invalid*xml files to test invalid syntax and crm_mon.xml to test valid syntax. Test text and XML for each using defaults, and also test --include=verifications for text+valid and --exclude=verifications for XML+invalid

Actually we have plenty of text/XML + valid tests, so we just need to add tests for invalid and --include/--exclude

kgaillot

Getting there! When you're close to happy with it, flatten the code commits and the test output commits for the next review. Thanks!

kgaillot · 2024-04-10T16:38:13Z

lib/pacemaker/pcmk_output.c

    pcmk__output_free(verify_out);

    if (verify_rc == pcmk_rc_ok) {
        if (pcmk_is_set(section_opts, pcmk_section_verify)) {
+            PCMK__OUTPUT_LIST_HEADER(out, false, rc, "Cluster Summary");


The way this macro works is that rc has to be initialized to pcmk_rc_no_output, and the macro will output the header the first time it's called and change rc to pcmk_rc_ok. Later calls will do nothing since the rc is changed. So, all calls for the same header have to use the same rc variable, which is why the header checks are all in the cluster-summary implementations.

This is a little different since we always want to output errors. Probably the easiest approach is to separate the verification itself from the output. We can always do the verification in the cluster-summary implementations, then output the message if the flag is set or there are errors.

To make sure I'm understanding this correctly, do you think the verification (pcmk__verify() call) should happen in cluster-summary and the output (PCMK__OUTPUT_LIST_HEADER() and out->list_item()) should happen in cluster-verify?

I was thinking cluster-summary should call pcmk__verify() and output the header, and cluster-verify would just output the message based on the rc. however at that point we might as well just do everything in cluster-summary and not have a separate message for cluster-verify

kgaillot · 2024-04-10T16:41:57Z

lib/pacemaker/pcmk_output.c

+    scheduler = pe_new_working_set();
+    scheduler->priv = verify_out;
+
+    verify_rc = pcmk__verify(scheduler, verify_out, scheduler->input);


You don't want a new scheduler object here, because it won't have input set to anything. To avoid output, you can save the initial priv value, reset it as you have here, then set it back after this

When I use the old scheduler object, there are some different failures:

Failed (rc=000): crm_resource - Try to move a resource to its existing location

Failed (rc=064): crm_resource - Move a resource from its existing location

Failed (rc=108): crm_resource - Move dummy to node1

Calling pcmk__verify() shouldn't have any effects on the scheduler object, not sure how that's possible

try rebasing on current main and see if that changes anything

also ditto for scheduler->priv, that could be causing output that's changing the tests

… summary

kgaillot · 2024-05-22T21:41:04Z

lib/pacemaker/pcmk_output.c

+
+    pcmk__output_new(&verify_out, "none", NULL, NULL);
+    verify_rc = pcmk__verify(scheduler, verify_out, scheduler->input);
+    pcmk__output_free(verify_out);


You also need to set scheduler->priv = verify_out before calling pcmk__verify() (and save and restore the original value) because the scheduler code will output messages there too (pcmk__output_cluster_status() sets scheduler->priv before calling the cluster-status message)

kgaillot · 2024-05-22T21:41:33Z

lib/pacemaker/pcmk_output.c

+            out->info(out, "CIB syntax is valid");
+        }
+    } else {
+        out->info(out, "CIB syntax has errors (for details, run crm_verify -LV).");


info() does nothing for XML, you have to add the XML element and attribute like you defined in the schema

kgaillot · 2024-05-22T21:42:26Z

lib/pacemaker/pcmk_output.c

+    scheduler = pe_new_working_set();
+    scheduler->priv = verify_out;
+
+    verify_rc = pcmk__verify(scheduler, verify_out, scheduler->input);


also ditto for scheduler->priv, that could be causing output that's changing the tests

kgaillot · 2024-05-22T21:51:13Z

lib/pacemaker/pcmk_output.c

    pcmk__output_free(verify_out);

    if (verify_rc == pcmk_rc_ok) {
        if (pcmk_is_set(section_opts, pcmk_section_verify)) {
+            PCMK__OUTPUT_LIST_HEADER(out, false, rc, "Cluster Summary");


I was thinking cluster-summary should call pcmk__verify() and output the header, and cluster-verify would just output the message based on the rc. however at that point we might as well just do everything in cluster-summary and not have a separate message for cluster-verify

kgaillot

I think I see what's happening ...

kgaillot · 2024-05-29T19:57:28Z

lib/pengine/pe_output.c

@@ -18,6 +18,8 @@
 #include <crm/common/xml.h>
 #include <crm/pengine/internal.h>

+#include <pcmki/pcmki_verify.h>


This is a problem ...

libpe_status can't use anything from libpacemaker, which is higher in the library stack. We could move pcmk__verify() to libpe_status except that it calls pcmk__schedule_actions() from libpacemaker. So, we'll have to move the cluster-summary messages from libpe_status (lib/pengine/pe_output.c) to libpacemaker (lib/pacemaker/pcmk_output.c). The only caller of cluster-summary is in libpacemaker anyway, and output messages are not public API, so there's no problem that way.

cluster-summary and other messages use get_node_feature_set(), so that will have to be exposed (with a pcmk__ prefix)

kgaillot · 2024-05-29T19:58:17Z

lib/pengine/pe_output.c

@@ -450,6 +458,28 @@ cluster_summary(pcmk__output_t *out, va_list args) {
                     scheduler->localhost, last_written, user, client, origin);
    }

+    // Use the existing scheduler, but avoid scheduler output
+    pcmk__output_new(&verify_out, "none", NULL, NULL);


check return value for errors

kgaillot · 2024-05-29T20:02:37Z

lib/pengine/pe_output.c

+    } else {
+        /* If there are verification errors, always print a statement about that, even if not requested */
+        PCMK__OUTPUT_LIST_HEADER(out, false, rc, "Cluster Summary");
+        out->list_item(out, NULL, "CIB syntax has errors (for details, run crm_verify -LV)");


looking at this again, let's omit the crm_verify arguments -- the user could be running against a CIB_file. it's up to the user to figure out what arguments they need.

kgaillot · 2024-05-29T20:37:41Z

lib/pengine/pe_output.c

+    priv_orig = scheduler->priv;
+    scheduler->priv = verify_out;
+
+    verify_rc = pcmk__verify(scheduler, verify_out, scheduler->input);


What's happening is that pcmk__verify() calls pcmk__schedule_actions(), and then crm_simulate calls pcmk__schedule_actions() again, and it's not happy with being called twice.

One of these two approaches should work:

Call pe_reset_working_set() after running pcmk__verify(). That will free scheduler->input, so you'll have to make a copy of that first and set it back afterward.

Create a new scheduler object and copy the input and now members.

waltdisgrace marked this pull request as draft January 17, 2024 15:58

kgaillot reviewed Jan 17, 2024

View reviewed changes

include/crm/common/output.h Outdated Show resolved Hide resolved

lib/common/output_xml.c Outdated Show resolved Hide resolved

lib/pacemaker/pcmk_output.c Outdated Show resolved Hide resolved

waltdisgrace force-pushed the 176 branch from 723b18b to 06184bb Compare January 31, 2024 17:36

kgaillot reviewed Jan 31, 2024

View reviewed changes

waltdisgrace force-pushed the 176 branch from 06184bb to 350055e Compare March 13, 2024 14:26

kgaillot reviewed Mar 18, 2024

View reviewed changes

waltdisgrace force-pushed the 176 branch from c01807e to e6436ac Compare April 10, 2024 16:10

kgaillot reviewed Apr 10, 2024

View reviewed changes

waltdisgrace added 6 commits May 22, 2024 01:25

Add verification information to crm_mon output

4efbf7f

Low: xml: clone crm_mon schema in preparation for changes

8bb5347

Low: xml: Update crm_mon schema to add verification status to cluster…

bf9f506

… summary

regression tests

1e3e264

changes to regression test ouput

d276584

debug 1

e93a0ad

kgaillot reviewed May 22, 2024

View reviewed changes

call pcmk__verify in cluster-summary

a18c0b1

waltdisgrace force-pushed the 176 branch from e6436ac to a18c0b1 Compare May 29, 2024 16:01

kgaillot reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t176 #3329

t176 #3329

waltdisgrace commented Jan 17, 2024

kgaillot left a comment

kgaillot commented Jan 17, 2024

kgaillot left a comment

kgaillot Jan 31, 2024

kgaillot commented Feb 1, 2024

kgaillot commented Feb 1, 2024

kgaillot left a comment

kgaillot commented Mar 18, 2024

kgaillot left a comment

kgaillot Apr 10, 2024

waltdisgrace May 22, 2024 •

edited

kgaillot May 22, 2024

kgaillot Apr 10, 2024

waltdisgrace May 22, 2024

kgaillot May 22, 2024

kgaillot May 22, 2024

kgaillot May 22, 2024

kgaillot May 22, 2024

kgaillot May 22, 2024 •

edited

kgaillot May 22, 2024

kgaillot May 22, 2024

kgaillot left a comment

kgaillot May 29, 2024

kgaillot May 29, 2024

kgaillot May 29, 2024

kgaillot May 29, 2024

t176 #3329

Are you sure you want to change the base?

t176 #3329

Conversation

waltdisgrace commented Jan 17, 2024

kgaillot left a comment

Choose a reason for hiding this comment

kgaillot commented Jan 17, 2024

kgaillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaillot commented Feb 1, 2024

kgaillot commented Feb 1, 2024

kgaillot left a comment

Choose a reason for hiding this comment

kgaillot commented Mar 18, 2024

kgaillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waltdisgrace May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaillot May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waltdisgrace May 22, 2024 •

edited

kgaillot May 22, 2024 •

edited