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

Don't override maxWaitTimeout in job spec #619

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

ZhangShuaiyi
Copy link
Contributor

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

  • Do not set default job.MaxWaitTimeout in UnmarshalYAML
  • Override job.MaxWaitTimeout only if it is empty

Related Tickets & Documents

@ZhangShuaiyi ZhangShuaiyi force-pushed the bugfix/MaxWaitTimeout branch 2 times, most recently from 6855805 to 9660b87 Compare April 16, 2024 12:47
@ZhangShuaiyi ZhangShuaiyi changed the title Override job.MaxWaitTimeout when not config in job spec. Don't override maxWaitTimeout in job spec Apr 16, 2024
@rsevilla87 rsevilla87 added the bug Something isn't working label Apr 16, 2024
@rsevilla87 rsevilla87 requested review from shashank-boyapally and removed request for shashank-boyapally April 16, 2024 14:07
Comment on lines 322 to 325
log.Debugf("job.MaxWaitTimeout is zero, override by timeout:%s", timeout)
job.MaxWaitTimeout = timeout
} else {
log.Debugf("job.MaxWaitTimeout is non zero:%s", job.MaxWaitTimeout)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.Debugf("job.MaxWaitTimeout is zero, override by timeout:%s", timeout)
job.MaxWaitTimeout = timeout
} else {
log.Debugf("job.MaxWaitTimeout is non zero:%s", job.MaxWaitTimeout)
log.Debugf("job.MaxWaitTimeout is zero in %s, override by timeout: %s", job.Name, timeout)
job.MaxWaitTimeout = timeout
} else {
log.Debugf("job.MaxWaitTimeout is non zero %s: %s", job.Name, job.MaxWaitTimeout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

[root@devnode1 kubelet-density]# grep maxWaitTimeout kubelet-density.yml
    maxWaitTimeout: 10m
[root@devnode1 kubelet-density]# kube-burner --log-level debug init -c kubelet-density.yml 2>&1 | grep job.MaxWaitTimeout
time="2024-04-17 08:26:35" level=debug msg="job.MaxWaitTimeout is non zero in kubelet-density: 10m0s" file="job.go:325"
[root@devnode1 kubelet-density]# sed -i 's/maxWaitTimeout/#maxWaitTimeout/g' kubelet-density.yml
[root@devnode1 kubelet-density]# grep maxWaitTimeout kubelet-density.yml
    #maxWaitTimeout: 10m
[root@devnode1 kubelet-density]# kube-burner --log-level debug init -c kubelet-density.yml 2>&1 | grep job.MaxWaitTimeout
time="2024-04-17 08:27:24" level=debug msg="job.MaxWaitTimeout is zero in kubelet-density, override by timeout: 4h0m0s" file="job.go:322"
[root@devnode1 kubelet-density]#

Copy link
Member

@rsevilla87 rsevilla87 left a comment

Choose a reason for hiding this comment

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

Some small nits and lgtm :)

Signed-off-by: Shuaiyi Zhang <zhangsy28@lenovo.com>
Copy link
Member

@rsevilla87 rsevilla87 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@rsevilla87 rsevilla87 merged commit 8c72335 into kube-burner:main Apr 17, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] maxWaitTimeout in job yml does not work
2 participants