[CLOYSTER-165] Generate answer file based on TUI data #91

arthurmco · 2025-01-13T16:30:57Z

Dump the data from the cluster, filled by the TUI, into an answer file.

Useful for debugging

src/cluster.cpp

viniciusferrao · 2025-01-13T16:54:42Z

src/answerfile.cpp

+
+void AnswerFile::dumpNodes(inifile& ini)
+{
+    size_t counter = 1;


What if the cluster does not have any nodes? It's a workstation.

Then, how the answer file would be?

The node group is optional, or it should be, so you'll automatically figure out if it has nodes or not.

viniciusferrao · 2025-01-13T16:55:25Z

src/answerfile.cpp

 void AnswerFile::loadNetwork(const std::string& networkSection,
-    NetworkType& network, bool optionalNameservers)
+    AFNetwork& network, bool optionalNameservers)


I really dislike this AFNetwork type. What this means?

I'm aware that isn't this PR fault, but this type does not makes sense.

The question is: why was this function generic?

the AFNetwork type, from what I saw, stores network parameters

Probably bad implementation. It should fill the Cluster object without temporary storages.

viniciusferrao · 2025-01-13T16:56:15Z

include/cloysterhpc/answerfile.h

+     * Do the inverse of `loadOptions`, i.e, move the stored settings
+     * into the answerfile.
+     */
+    void dumpOptions(inifile& ini);


const inifile&?

You're right. I vacillated here

viniciusferrao · 2025-01-13T16:57:45Z

include/cloysterhpc/answerfile.h

@@ -188,13 +194,17 @@ class AnswerFile {
     */
    void loadExternalNetwork();

+    void dumpExternalNetwork(inifile& ini);


inifile is passed too many times. It should be a member.

viniciusferrao · 2025-01-13T16:59:42Z

src/answerfile.cpp

+
+void AnswerFile::dumpPostfix(inifile& ini)
+{
+    if (!postfix.enabled) {


The source of this seems strange. Is it a bool on mail class? If yes that's a bug. The class should be std::optional.

viniciusferrao · 2025-01-13T17:00:10Z

src/answerfile.cpp

+        case Postfix::Profile::Relay:
+            ini.setValue("postfix.relay", "server", postfix.smtp->server);
+            ini.setValue(
+                "postfix.relay", "port", std::to_string(postfix.smtp->port));


std::uint16_t

viniciusferrao · 2025-01-13T17:00:32Z

src/answerfile.cpp

+    dumpExternalNetwork(ini);
+    dumpManagementNetwork(ini);
+    dumpApplicationNetwork(ini);
+    dumpInformation(ini);
+    dumpTimeSettings(ini);
+    dumpHostnameSettings(ini);
+    dumpSystemSettings(ini);


You see, it should be a member.

dhilst · 2025-01-14T23:30:43Z

_ No description provided. _

Arthur, plz give an precise description of the feature in the PR description (:

viniciusferrao · 2025-01-16T15:17:07Z

I don't understand why Github shows changes from PR's that were already merged. I could not read the code anymore because everything is mixed. Is that how this works?

dhilst · 2025-01-16T17:33:57Z

conanfile.txt

+[requires]
+cli11/[>=2.4.0 <2.5.0]
+spdlog/[>=1.14.0 <1.15.0]
+fmt/[>=10.0.0 <12.0.0]
+boost/[>=1.83.0 <1.84.0]
+magic_enum/[>=0.9.0 <0.10.0]
+gsl-lite/[>=0.41.0 <0.42.0]
+doctest/[>=2.4.0 <2.5.0]
+cryptopp/[>=8.9.0 <8.10.0]
+sdbus-cpp/[>=2.0.0 <2.1.0]
+
+[layout]
+cmake_layout


I guess this file need to be deleted in favor of connanfile.py, right?
@viniciusferrao

dhilst · 2025-01-16T17:44:46Z

include/cloysterhpc/answerfile.h

+struct AFNode {
+    std::optional<std::string> prefix;
+    std::optional<std::string> padding;
+    std::optional<address> start_ip = address();
+    std::optional<std::string> hostname;
+    std::optional<std::string> root_password;
+    std::optional<std::string> mac_address;
+    std::optional<std::string> sockets;
+    std::optional<std::string> cores_per_socket;
+    std::optional<std::string> threads_per_core;
+    std::optional<std::string> bmc_address;
+    std::optional<std::string> bmc_username;
+    std::optional<std::string> bmc_password;
+    std::optional<std::string> bmc_serialport;
+    std::optional<std::string> bmc_serialspeed;
+};


Is prefix and padding sensical for a single node? Remove it if it doesn't please.

Also, I guess it is ip instead of start_ip?

And some fields like sockets and core_pre_socket should be positive integers, instead of strings, @viniciusferrao uint16_t again?

Prefix is the node prefix, like: n or node. The padding is the number of digits a sequence should have, example, a padding of 1 will produce n01, 2 will be node001. None will be just n1, or node1.

Finally the start_ip is considering a formation of nodes, that will be filled in sequence.

So yes, it seems misplaced, because it looks like it should be calculated and not hardcoded.

Regarding the other fields, yes, they must have strong typing. However I'm not sure how SimpleIni handles this. I very much dislike everything being std::string.

dhilst · 2025-01-16T17:51:28Z

Dump the data from the cluster, filled by the TUI, into an answer file.

Useful for debugging

How can I run the TUI and get the anwerfile without running the remaining of the installation?

viniciusferrao

Can it also possible to write answerfile in extension instead of AF? Maybe answer_file?

Let's try to merge this as soon as possible so we can move on to glibmm.

dhilst · 2025-01-17T14:30:43Z

Can it also possible to write answerfile in extension instead of AF? Maybe answer_file?

Let's try to merge this as soon as possible so we can move on to glibmm.

The only thing missing is to generate the answerfile and stop (do not continue the installation), the remaining of the code LGTM

viniciusferrao · 2025-01-17T15:05:29Z

Can it also possible to write answerfile in extension instead of AF? Maybe answer_file?
Let's try to merge this as soon as possible so we can move on to glibmm.

The only thing missing is to generate the answerfile and stop (do not continue the installation), the remaining of the code LGTM

That can be (not should) on main(). Just before the start of the executionEngine it should dump and return.

Signed-off-by: Vinícius Ferrão <[email protected]> Fixed more outrageous bugs. The program is now able to at least boot a compute node again Signed-off-by: Vinícius Ferrão <[email protected]> Disable cosmetic warnings Pin spdlog version Run clang-format Handle copy to temporary file error Fix message in CMakeLists.txt Handle error in answerfile minor version Fix routing error disabling the VM network Enable Managed network again Fix answerfile version handling Fix copyFile implementation Fix typos, change variables names and fix throw. * Fix typos in the documentation. * Fix typos on LOG messages. * Change variable names to be more accurate with the context. * Rethrow should also follow the golden rule of: throw by value and catch by reference. Signed-off-by: Vinícius Ferrão <[email protected]> Clearning more warnings Fix the last warnings Remove std::exit call, and raise an exception instead Run clang-format Added version ranges to conanfile.txt This change will keep the project more reliable while still getting updates from upstream on minor versions. Signed-off-by: Vinícius Ferrão <[email protected]> Last try to fix clang-format Signed-off-by: Vinícius Ferrão <[email protected]>

This parameter was unneeded, we can use the member m_ini parameter

dhilst · 2025-01-20T13:52:33Z

Fix the warning

dhilst · 2025-01-20T13:55:35Z

src/main.cpp

+        LOG_TRACE("Starting execution engine");
+        model->dumpData("answerfile.result.ini");
+
+        return EXIT_SUCCESS;


This will stop the execution unconditionally, remove it. To generate the file without executing it we can already use dry run

Also, it leads to another warning, please fix it

I'm getting a permission error when executing it, we should add a command line option to where the answerfile should be generated, --dump-answerfile=<PATH> and use answerfile-<DATE>.conf as default, where <DATE> is the current date. This will prevent anwerfiles to be rewritten.

I'm having weird messages at the background of the TUI, I guess this is a bug?
, maybe this is because of the -l6?

Confirming, yes it is because of the -l6, it's okay I guess. @viniciusferrao What do you think? Is -l6 supposed to be used by the end user?

I'm getting a permission error when executing it, we should add a command line option to where the answerfile should be generated, --dump-answerfile=<PATH> and use answerfile-<DATE>.conf as default, where <DATE> is the current date. This will prevent anwerfiles to be rewritten.

I'm having weird messages at the background of the TUI, I guess this is a bug? , maybe this is because of the -l6?

Confirming, yes it is because of the -l6, it's okay I guess. @viniciusferrao What do you think? Is -l6 supposed to be used by the end user?

This is and isn't a bug at the same time. If you see other applications, when there's warnings or errors it overwrites the newt library, which is expected.

But what is happening here is that the log should be preferrabily written to a file instead of stdout. If you call the TUI it's obvious that the output should be on a file.

If running in unattended mode it should be on stdout. That's the problem here.

The view isn't implemented correctly. However even after fixing this will still occour, however much less, and many times it will not even happen. Errors should be thrown using the TUI composing.

For now I would leave it as is.

One is for the lack of one, the other is code after one

src/presenter/PresenterNodesOperationalSystem.cpp

src/presenter/PresenterNetwork.cpp

viniciusferrao · 2025-01-20T15:37:28Z

include/cloysterhpc/presenter/PresenterNetwork.h

@@ -42,6 +42,8 @@ class NetworkCreator {
    bool checkIfInterfaceRegistered(std::string_view interface);

    void saveNetworksToModel(Cluster& model);
+
+    size_t getSelectedInterfaces();


std::size_t.

It's the same thing, no?

dhilst · 2025-01-20T19:16:15Z

@viniciusferrao If we run with -a what should be the behavior? It should generate another answerfile? This would be useful for testing because the input and output must match.

I think we should add an --dump-answerfile=<PATH> option that dumps the answerfile even if -a is passed in the command line

Here is a behavioral specification suggestion:

--dump-anwerfile=output-answerfile.conf and no -a: Invokes the TUI, ask the question to the user and generate output-answerfile.conf at the end of the questions
-a input-answerfile.conf --dump-anwerfile=output-answerfile.conf: generates output-answerfile.conf and diff input-answerfile.conf output-answerfile.conf must output no lines (both files are equal (as long as input-answerfile.conf was generated by a previous successful run with --dump-answerfile=input-answerfile.conf))
-a input-answerfile.conf (and no --dump-anwerfile): No answerfile is generated
No -a and no --dump-answerfile: Generates answerfile-<DATE>.conf where <DATE> is a date in the format YYYY-mm-dd-HHMM

dhilst · 2025-01-20T20:19:37Z

I got a segfault after entering the node information, not sure yet where (installing clion to find out) meanwhile, here is the gdb stacktrace (for future reference)

arthurmco · 2025-01-21T13:04:15Z

@dhilst OK, we will solve this in a future PR

sonarqubecloud · 2025-01-21T13:22:10Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

arthurmco requested review from viniciusferrao and lbgracioso January 13, 2025 16:30

arthurmco self-assigned this Jan 13, 2025

dhilst removed the request for review from lbgracioso January 13, 2025 16:33

dhilst reviewed Jan 13, 2025

View reviewed changes

src/cluster.cpp Outdated Show resolved Hide resolved

viniciusferrao reviewed Jan 13, 2025

View reviewed changes

Base automatically changed from fix-bugs to master January 13, 2025 21:04

arthurmco force-pushed the cloyster-answerfile-dump branch from 6e3138f to 89de020 Compare January 16, 2025 17:24

arthurmco requested review from viniciusferrao and dhilst January 16, 2025 17:27

dhilst marked this pull request as ready for review January 16, 2025 17:31

dhilst reviewed Jan 16, 2025

View reviewed changes

viniciusferrao requested changes Jan 16, 2025

View reviewed changes

arthurmco force-pushed the cloyster-answerfile-dump branch 2 times, most recently from a47101a to 0be1187 Compare January 20, 2025 09:44

viniciusferrao and others added 7 commits January 20, 2025 06:45

Add code to dump answerfile information

d78fcbe

Start adding code to dump model (aka cluster) data

1c0730a

Dump cluster data to answerfile

bb5b798

Remove the ini parameter in most of answerfile functions

5008c6a

This parameter was unneeded, we can use the member m_ini parameter

Reformat some files

472949b

Dump the answerfile

58ba2db

arthurmco force-pushed the cloyster-answerfile-dump branch from 0be1187 to 58ba2db Compare January 20, 2025 09:46

arthurmco requested a review from viniciusferrao January 20, 2025 09:46

arthurmco requested review from dhilst and removed request for viniciusferrao January 20, 2025 09:46

dhilst reviewed Jan 20, 2025

View reviewed changes

arthurmco added 2 commits January 20, 2025 11:53

presenter: fix issues when counting available nics

1ace1ff

Fix some warnings regarding return statements

0ee27bb

One is for the lack of one, the other is code after one

arthurmco requested a review from viniciusferrao January 20, 2025 15:11

viniciusferrao requested changes Jan 20, 2025

View reviewed changes

arthurmco added 2 commits January 21, 2025 10:02

Fix typing and remote URL

141654a

Make the dump answerfile name passable by parameter

bb6d7b7

arthurmco requested a review from viniciusferrao January 21, 2025 13:04

Fix lint

e3a9cc0

arthurmco requested a review from dhilst January 21, 2025 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLOYSTER-165] Generate answer file based on TUI data #91

[CLOYSTER-165] Generate answer file based on TUI data #91

arthurmco commented Jan 13, 2025 •

edited

Loading

viniciusferrao Jan 13, 2025

arthurmco Jan 14, 2025

viniciusferrao Jan 15, 2025

viniciusferrao Jan 13, 2025

arthurmco Jan 14, 2025

viniciusferrao Jan 15, 2025

viniciusferrao Jan 13, 2025

arthurmco Jan 14, 2025

viniciusferrao Jan 13, 2025

viniciusferrao Jan 13, 2025

viniciusferrao Jan 13, 2025

dhilst Jan 14, 2025

viniciusferrao Jan 15, 2025

viniciusferrao Jan 13, 2025

dhilst commented Jan 14, 2025

viniciusferrao commented Jan 16, 2025

dhilst Jan 16, 2025

dhilst Jan 16, 2025

viniciusferrao Jan 16, 2025

dhilst commented Jan 16, 2025

viniciusferrao left a comment •

edited

Loading

dhilst commented Jan 17, 2025

viniciusferrao commented Jan 17, 2025 •

edited

Loading

dhilst commented Jan 20, 2025

dhilst Jan 20, 2025

dhilst Jan 20, 2025 •

edited

Loading

dhilst Jan 20, 2025 •

edited

Loading

viniciusferrao Jan 20, 2025

viniciusferrao Jan 20, 2025

arthurmco Jan 21, 2025

dhilst commented Jan 20, 2025 •

edited

Loading

dhilst commented Jan 20, 2025

arthurmco commented Jan 21, 2025

sonarqubecloud bot commented Jan 21, 2025

[CLOYSTER-165] Generate answer file based on TUI data #91

Are you sure you want to change the base?

[CLOYSTER-165] Generate answer file based on TUI data #91

Conversation

arthurmco commented Jan 13, 2025 • edited Loading

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

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

dhilst commented Jan 14, 2025

viniciusferrao commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilst commented Jan 16, 2025

viniciusferrao left a comment • edited Loading

Choose a reason for hiding this comment

dhilst commented Jan 17, 2025

viniciusferrao commented Jan 17, 2025 • edited Loading

dhilst commented Jan 20, 2025

Choose a reason for hiding this comment

dhilst Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

dhilst Jan 20, 2025 • edited Loading

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

dhilst commented Jan 20, 2025 • edited Loading

dhilst commented Jan 20, 2025

arthurmco commented Jan 21, 2025

sonarqubecloud bot commented Jan 21, 2025

Quality Gate passed

arthurmco commented Jan 13, 2025 •

edited

Loading

viniciusferrao left a comment •

edited

Loading

viniciusferrao commented Jan 17, 2025 •

edited

Loading

dhilst Jan 20, 2025 •

edited

Loading

dhilst Jan 20, 2025 •

edited

Loading

dhilst commented Jan 20, 2025 •

edited

Loading