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

Termination grace period is multiplied by time.Second erronously #5046

Closed
alexec opened this issue Feb 8, 2021 · 8 comments · Fixed by #5049
Closed

Termination grace period is multiplied by time.Second erronously #5046

alexec opened this issue Feb 8, 2021 · 8 comments · Fixed by #5049
Labels
Milestone

Comments

@alexec
Copy link
Contributor

alexec commented Feb 8, 2021

err = WaitForTermination(ctx, c, containerID, terminationGracePeriodDuration*time.Second)

@tczhao

@alexec alexec added the type/bug label Feb 8, 2021
@tczhao
Copy link
Member

tczhao commented Feb 8, 2021

Sorry, I don't understand what the issue is. Do you mean that the terminationGracePeriodDuration should embed the time.Second information
Could you point me in the right direction?

@alexec
Copy link
Contributor Author

alexec commented Feb 8, 2021

const (
	Nanosecond  Duration = 1
	Microsecond          = 1000 * Nanosecond
	Millisecond          = 1000 * Microsecond
	Second               = 1000 * Millisecond
	Minute               = 60 * Second
	Hour                 = 60 * Minute
)

10 seconds * 10 seconds = 1000 * Millisecond * 1000 * Millisecond = a very big number and not what I think you want

@alexec alexec added this to the v3.0 milestone Feb 8, 2021
@tczhao
Copy link
Member

tczhao commented Feb 8, 2021

It is working like this at the moment

seconds := 10 // from godoc time example
fmt.Print(time.Duration(seconds)*time.Second) // prints 10s
terminationGracePeriodDuration := time.Duration(int64 terminationGracePeriodSecond)
terminationGracePeriodDuration * time.second

The terminationGracePeriodDuration is a multiplier factor
(A Duration represents the elapsed time between two instants as an int64 nanosecond count)

I think we are using it correctly

@alexec
Copy link
Contributor Author

alexec commented Feb 8, 2021

What about my example?

terminationGracePeriodDuration:=10*time.Second

println(terminationGracePeriodDuration*time.Second)

@alexec
Copy link
Contributor Author

alexec commented Feb 8, 2021

Should terminationGracePeriodDuration be int not time.Second?

@tczhao
Copy link
Member

tczhao commented Feb 8, 2021

Yes, your example does produce a very big number

but in our case, we are casting it using time.Duration instead of time.Second

// GetTerminationGracePeriodDuration returns the terminationGracePeriodSeconds of podSpec in Time.Duration format
func (we *WorkflowExecutor) GetTerminationGracePeriodDuration(ctx context.Context) (time.Duration, error) {
	pod, err := we.getPod(ctx)
	if err != nil {
		return time.Duration(0), err
	}
	terminationGracePeriodDuration := time.Duration(*pod.Spec.TerminationGracePeriodSeconds)
	return terminationGracePeriodDuration, nil
}

*pod.Spec.TerminationGracePeriodSeconds is int64

@alexec
Copy link
Contributor Author

alexec commented Feb 8, 2021

To be clear, time.Second is specifically defined as 1000 * Millisecond.

It NEVER makes sense to multiple time.Duration by time.Second

This is the fix to the code:

terminationGracePeriodDuration := *pod.Spec.TerminationGracePeriodSeconds * time.Second

@tczhao
Copy link
Member

tczhao commented Feb 8, 2021

ok, I get your points now.
This definitely makes more sense

alexec added a commit to alexec/argo-workflows that referenced this issue Feb 8, 2021
@alexec alexec linked a pull request Feb 8, 2021 that will close this issue
1 task
alexec added a commit to alexec/argo-workflows that referenced this issue Feb 9, 2021
alexec added a commit to alexec/argo-workflows that referenced this issue Feb 9, 2021
alexec added a commit that referenced this issue Feb 10, 2021
@simster7 simster7 mentioned this issue Feb 16, 2021
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants