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

Allow to pass in extra flags into the call to kexec #228

Closed
wants to merge 1 commit into from

Conversation

johbo
Copy link
Contributor

@johbo johbo commented Jun 5, 2024

Noticed that the call to kexec was hanging in some cases when I had a machine which did have a stale rbd volume mounted.

Based on the logs I found out that the calls to kexec were stuck because of the stale volume mounts. Using the --no-sync flag does work fine for this case.

I am not 100% sure if there could be a good reason why one wants to avoid this flag in the context of booting into the installer. If there are reasons, then we could maybe allow to pass in parameters via the call to nixos-anywhere. Please let me know if this would be better, then i will tweak the change.

There is a related PR into nixos-anywhere:

@@ -84,4 +84,4 @@ else
fi
# We will kexec in background so we can cleanly finish the script before the hosts go down.
# This makes integration with tools like terraform easier.
nohup sh -c "sleep 6 && '$SCRIPT_DIR/kexec' -e" &
nohup sh -c "sleep 6 && '$SCRIPT_DIR/kexec' -e --no-sync" &
Copy link
Member

Choose a reason for hiding this comment

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

Also most people probably use this to replace the old system,
but in case someone just needs this to perform some filesystem maintenance,
we shouldn't uncleanly unmount the filesystem.

I thought about having a timeout:

Suggested change
nohup sh -c "sleep 6 && '$SCRIPT_DIR/kexec' -e --no-sync" &
nohup sh -c "sleep 6 && (timeout 30 sync; '$SCRIPT_DIR/kexec' -e --no-sync)" &

However this also is not ideal because unlike the kexec version this actually performs the sync before all userspaces processes are stopped.

So I think this should be a flag instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could have a way to allow to pass flags to kexec in general instead of just having a --no-sync flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually also think now that this would be better. Allowing to pass in flags makes it flexible for all sorts of corner cases. Will try to work this out and push an update into this PR.

@johbo
Copy link
Contributor Author

johbo commented Jun 7, 2024

@Mic92 hope I got it figured out, struggled a bit with passing the extra flags around in a proper way. Did introduce the sh -c so that it also works if multiple flags are specified.

@johbo johbo changed the title Add flag "--no-sync" to kexec call Allow to pass in extra flags into the call to kexec Jun 7, 2024
@Mic92
Copy link
Member

Mic92 commented Jun 10, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Jun 10, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@Mic92
Copy link
Member

Mic92 commented Jun 10, 2024

Merged in #236

@Mic92 Mic92 closed this Jun 10, 2024
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.

2 participants