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

Execute parseable yaml files during runStage #130

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

frelon
Copy link
Collaborator

@frelon frelon commented Sep 26, 2024

Make default executor Graph method not completely error out when
encountering an unparseable yaml file.

Signed-off-by: Fredrik Lönnegren fredrik.lonnegren@suse.com

Make default executor Graph method not completely error out when
encountering an unparseable yaml file.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon requested a review from a team as a code owner September 26, 2024 12:49
Copy link
Collaborator

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

LGTM

@kkaempf
Copy link
Collaborator

kkaempf commented Sep 26, 2024

Not sure about this one 🤔
Esp. given that yip runs on the client (mostly unattended), erroring out completely might be a better option that just a logging a warning. Errors might be hard to track down ...

Side node: We really need to find a way to export state/logs from a node to elemental-operator (resp. the UI).

@frelon
Copy link
Collaborator Author

frelon commented Sep 26, 2024

Not sure about this one 🤔 Esp. given that yip runs on the client (mostly unattended), erroring out completely might be a better option that just a logging a warning. Errors might be hard to track down ...

Good point! I will probably need to test this with an upgrade as well, since the health-checker might detect this...

My original problem was that when installing a new system and accidentally leaving a malformed yaml in /system/oem, I would not be able to even log in to see what was the error.

Side node: We really need to find a way to export state/logs from a node to elemental-operator (resp. the UI).

Agree on the export of logs!

@davidcassany
Copy link
Collaborator

davidcassany commented Sep 26, 2024

Not sure about this one 🤔 Esp. given that yip runs on the client (mostly unattended), erroring out completely might be a better option that just a logging a warning. Errors might be hard to track down ...

Side node: We really need to find a way to export state/logs from a node to elemental-operator (resp. the UI).

Just giving a bit of more context of what we are trying to solve here. Error managing in yip and its integration in elemental is not sophisticated. If yip fails, by default, we are not marking as failed the actual service invoking it. The reason for that is to not prevent to the system to boot. So even if there are errors in yip the boot process continues normally unless we set the strict flag in elemental toolkit configuration.

If a malformed yaml in /oem gets into the system this could potentially prevent any yip setup to be executed at boot. So probably a better behavior could be simply accumulate all errors until the process is done and only after error out if something occurred.

Also a long standing idea we never implemented was to distinguish error types and react differently accordingly. As of today yip is binary: error true or false. We can't easily get the type of error (internal of yip or caused by yaml execution, etc.).

@kkaempf
Copy link
Collaborator

kkaempf commented Sep 26, 2024

I'm mostly concerned about "it somehow doesn't work" bug reports because somewhere, some yaml failed to get parsed and there's no way (besides ssh-ing into the node) to know what's going on.

Currently such a problem is very obvious, simply because the node wouldn't even boot. I agree that this is bad (you can't ssh into a non-booted node) and good (errors bubble up spectacuously 😆 ) at the same time.

Copy link
Collaborator

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Retracting my approval, after a second though I think we should do it slightly different.

pkg/executor/default.go Show resolved Hide resolved
In this commit we add back the error when trying to load a corrupt yaml
file, but we still execute the entire graph first, making the command
fail with an error about which path failed to load, but not breaking the
execution for other yaml files.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon changed the title Do not error out on malformed yaml Execute parseable yaml files during runStage Sep 27, 2024
@frelon
Copy link
Collaborator Author

frelon commented Sep 27, 2024

@kkaempf @davidcassany How do you feel about the new implementation? It will return the loading-error after executing the valid yaml-files.

Copy link
Collaborator

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Nice work 👍

pkg/executor/default_test.go Show resolved Hide resolved
@frelon frelon merged commit 327cfaa into rancher:main Sep 30, 2024
1 check passed
@frelon frelon deleted the continue-workflow-on-malformed-yaml branch September 30, 2024 11:46
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