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

taskrunner: fix panic when a task that has a dynamic user is recovered #24739

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gabivlj
Copy link
Contributor

@gabivlj gabivlj commented Dec 20, 2024

Before this, the nomad client would crash with:

panic: assignment to entry in nil map

Description

We are seeing in our production clusters the following crash when this codepath is being hit.

Testing & Reproduction steps

The unit test I added doesn't pass without this change.

Contributor Checklist

  • Changelog Entry If this PR changes user-facing behavior, please generate and add a
    changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
    ensure regressions will be caught.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    and job configuration, please update the Nomad website documentation to reflect this. Refer to
    the website README for docs guidelines. Please also consider whether the
    change requires notes within the upgrade guide.

@gabivlj gabivlj requested review from a team as code owners December 20, 2024 01:07
Before this, the nomad client would crash with:
```
panic: assignment to entry in nil map
```
@edevil
Copy link

edevil commented Jan 8, 2025

Is anything missing here @jrasell @shoenig ?

@@ -64,7 +64,7 @@ func (h *dynamicUsersHook) Prestart(_ context.Context, request *interfaces.TaskP
if request.PreviousState != nil {
ugid, exists := request.PreviousState[dynamicUsersStateKey]
if exists {
response.State[dynamicUsersStateKey] = ugid
response.State = map[string]string{dynamicUsersStateKey: ugid}
Copy link
Member

Choose a reason for hiding this comment

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

@jrasell A total nit, but could we just move the response.State map initialization that happens near the end of this function, to the beginning of the function? My only rationale here is that being explicit about initializing the state upfront may help curb any future regressions.

Copy link
Member

Choose a reason for hiding this comment

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

@gabivlj let me know if this suggestion makes sense. Thanks!

@Juanadelacuesta Juanadelacuesta requested review from jrasell and removed request for shoenig January 22, 2025 10:04
@Juanadelacuesta Juanadelacuesta added type/bug stage/accepted Confirmed, and intend to work on. No timeline committment though. labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/crash type/bug
Projects
Development

Successfully merging this pull request may close these issues.

6 participants