Update task timer use duration #5023

christoph-zededa · 2025-06-26T16:40:15Z

Description

this is a follow-up of #4967 (comment)
pillar/updateTaskTimer: switch to less-error prone

time.Duration

How to test and validate this PR

same as #4967

Changelog notes

No user-visible changes.

PR Backports

14.5-stable: no
13.4-stable: no

Checklist

I've provided a proper description
I've added the proper documentation
I've tested my PR on amd64 device
I've tested my PR on arm64 device
I've written the test verification instructions
I've set the proper labels to this PR
I've checked the boxes above, or I've provided a good reason why I didn't
check them.

milan-zededa · 2025-06-26T16:49:26Z

pkg/pillar/types/global.go

 }

+// GlobalValueDuration - Gets a duration global setting value
+func (configPtr *ConfigItemValueMap) GlobalValueDuration(key GlobalSettingKey) time.Duration {


I'm not sure if this function is good idea because it assumes that the time unit is seconds.
So it would be bug to use it for example for: goroutine.leak.detection.check.interval.minutes

Or kubevirt.drain.timeout (hours)

If we had no such options (expecting minutes or hours), it might even be a good idea: users would know that all intervals and durations should be in seconds. But unfortunately it will not work :(

what about renaming it to GlobalValueDurationSeconds?

On its own, this type is useful, and we could consider using it exclusively in the future. However, I am uncertain about replacing the existing options with it. I expect (though I’m not entirely sure) that doing so could lead to issues with backward compatibility. If the value is serialized, we cannot interpret an integer as a duration.
@milan-zededa and @eriknordmark can clarify it here.

TLDR:
I'm voting for 1) the new type and function 2) against applying it to already existing props, at least non-seconds ones

I think it's also a good idea to add multiple types like DurationSeconds, DurationMinutes, DurationHours, DurationDays... and use them accordingly to what is expected from a parameter

milan-zededa · 2025-06-26T16:50:30Z

pkg/pillar/cmd/zedagent/handlecertconfig.go

-	const shortTime = 120 // Two minutes
-	certInterval := ctx.globalConfig.GlobalValueInt(types.CertInterval)
+	const shortTime = 2 * time.Minute
+	certInterval := time.Duration(ctx.globalConfig.GlobalValueInt(types.CertInterval)) * time.Second


I think you want to use GlobalValueDuration here, but see my other comment.

christoph-zededa · 2025-06-27T11:19:24Z

regarding spdx check:

Checking pkg/pillar/zedcloud/proxy.go
  - SPDX-License-Identifier: missing SPDX-License-Identifier!
  - Copyright: does not have the copyright!

seems the tool is wrong @OhmSpectator as I see this in this file:

    1  // Copyright 2017 The Go Authors. All rights reserved.                                                                                                                 
    2  // Use of this source code is governed by a BSD-style                                                                                                                  
    3  // license that can be found in the LICENSE file.                                                                                                                      
    4                                                                                                                                                                         
    5  // Package httpproxy provides support for HTTP proxy determination                                                                                                     
    6  // based on environment variables, as provided by net/http's                                                                                                           
    7  // ProxyFromEnvironment function.                                                                                                                                      
    8  //                                                                                                                                                                     
    9  // The API is not subject to the Go 1 compatibility promise and may change at                                                                                          
   10  // any time.                                                                                                                                                           
   11  // package httpproxy                                                                                                                                                   
   12  package zedcloud

OhmSpectator · 2025-06-27T11:23:56Z

I'm now curious, where did we take this file from?
The tool checks for a proper SPDX-formatted license (https://spdx.github.io/spdx-spec/v3.0.1/annexes/spdx-license-expressions/). I'll think what to do in this case. One option I see is to format it properly.

christoph-zededa · 2025-06-27T11:26:27Z

I'm now curious, where did we take this file from? The tool checks for a proper SPDX-formatted license (https://spdx.github.io/spdx-spec/v3.0.1/annexes/spdx-license-expressions/). I'll think what to do in this case. One option I see is to format it properly.

And I am curious what this PR has to do with proxy.go ...

christoph-zededa · 2025-06-27T11:33:01Z

I'm now curious, where did we take this file from? The tool checks for a proper SPDX-formatted license (https://spdx.github.io/spdx-spec/v3.0.1/annexes/spdx-license-expressions/). I'll think what to do in this case. One option I see is to format it properly.

And I am curious what this PR has to do with proxy.go ...

ah, rebase helped

rene

LGTM

eriknordmark · 2025-07-07T22:38:33Z

I'm now curious, where did we take this file from? The tool checks for a proper SPDX-formatted license (https://spdx.github.io/spdx-spec/v3.0.1/annexes/spdx-license-expressions/). I'll think what to do in this case. One option I see is to format it properly.

There clearly isn't a SPDX license in that header.
I wonder if our copyright check looks for (c) or (C).

eriknordmark

Introducing the GlobalValueDurationSeconds() adds clarity but should we also define Seconds as a type in global.go (even if it looks the same as an Int) so that the definition of the value in global.go ends up using Seconds i.e., introduce a
configItemSpecMap.AddSecondsItem() and use it for all of the seconds time values?

If we do this, why not also introduce the Minutes and Hours function to parallel this - that makes the definitions in global.h a lot clearer.

time.Duration Signed-off-by: Christoph Ostarek <[email protected]>

OhmSpectator · 2025-07-30T16:54:36Z

@christoph-zededa, are you planning to address the comments? This one? https://github.com/lf-edge/eve/pull/5023/files#r2169484219

OhmSpectator · 2025-07-31T04:53:58Z

EVE Upgrade fails all the time... It's better to check the logs.

europaul

I think it's a very good idea to convert the parameters to time.Duration as soon as possbile. Very nice @christoph-zededa

europaul · 2025-07-31T10:42:54Z

pkg/pillar/types/global.go

+	defaultDuration, minDuration, maxDuration time.Duration) {
+
+	if defaultDuration < minDuration || defaultDuration > maxDuration {
+		logrus.Fatalf("Adding int item %s failed. Value does not meet given min/max criteria", key)


Suggested change

logrus.Fatalf("Adding int item %s failed. Value does not meet given min/max criteria", key)

logrus.Fatalf("Adding duration item %s failed. Value does not meet given min/max criteria", key)

europaul · 2025-07-31T10:43:20Z

pkg/pillar/types/global.go

 	AgentSettings map[AgentSettingKey]ConfigItemSpec
 }

+// AddDurationItem - Adds integer item to specMap


Suggested change

// AddDurationItem - Adds integer item to specMap

// AddDurationItem - Adds duration item to specMap

europaul · 2025-07-31T10:51:50Z

pkg/pillar/types/global.go

 	case ConfigItemTypeTriState:
 		return FormatTriState(val.TriStateValue)
+	case ConfigItemTypeDuration:
+		return fmt.Sprintf("%d", uint64(val.DurationValue.Seconds()))


maybe it would be better to use

Suggested change

return fmt.Sprintf("%d", uint64(val.DurationValue.Seconds()))

return fmt.Sprintf("%v", val.DurationValue)

I don't want to change it to show floats as of now it was always an integer with seconds.
Do you think it does not matter?

europaul · 2025-07-31T10:59:33Z

pkg/pillar/types/global.go

 }

+// GlobalValueDuration - Gets a duration global setting value
+func (configPtr *ConfigItemValueMap) GlobalValueDuration(key GlobalSettingKey) time.Duration {


I think it's also a good idea to add multiple types like DurationSeconds, DurationMinutes, DurationHours, DurationDays... and use them accordingly to what is expected from a parameter

christoph-zededa · 2025-07-31T11:34:19Z

@europaul

I think it's also a good idea to add multiple types like DurationSeconds, DurationMinutes, DurationHours, DurationDays... and use them accordingly to what is expected from a parameter

I don't understand why you need this? F.e. for hours you can just do:
c.GlobalValueDuration("mykey").Hours()

christoph-zededa · 2025-07-31T12:36:29Z

/rerun red

(hoping to gather collect-info this time)

europaul · 2025-07-31T12:48:55Z

@europaul

I think it's also a good idea to add multiple types like DurationSeconds, DurationMinutes, DurationHours, DurationDays... and use them accordingly to what is expected from a parameter

I don't understand why you need this? F.e. for hours you can just do: c.GlobalValueDuration("mykey").Hours()

you have to know how to convert the integer that you get out of the config to time.Duration for each parameter (because it could mean mins, secs, hours etc).

I'll try to create a PR for this soon

christoph-zededa · 2025-07-31T13:44:15Z

should we also define Seconds as a type in global.go

No, because it will fail during upgrade as /persist/status/zedagent/ConfigItemValueMap/global.json is at one point storing Integer and later trying to retrieve Seconds/Duration.

So, I think I should reset this PR to ce032ad .

Stacktrace:

goroutine 2730 [running]:
github.com/lf-edge/eve/pkg/pillar/agentlog.getStacks(0x0)
	/pillar/agentlog/agentlog.go:467 +0x4a
github.com/lf-edge/eve/pkg/pillar/agentlog.printStack(0xc0006dc6a8, {0x7ffc6464af6f, 0x6}, 0x915)
	/pillar/agentlog/agentlog.go:235 +0x45
github.com/lf-edge/eve/pkg/pillar/agentlog.initImpl.func1()
	/pillar/agentlog/agentlog.go:550 +0x25
github.com/sirupsen/logrus.runHandler(0xc000059e28?)
	/pillar/vendor/github.com/sirupsen/logrus/alt_exit.go:39 +0x33
github.com/sirupsen/logrus.runHandlers(...)
	/pillar/vendor/github.com/sirupsen/logrus/alt_exit.go:44
github.com/sirupsen/logrus.(*Logger).Exit(0x4c99ae0, 0x1)
	/pillar/vendor/github.com/sirupsen/logrus/logger.go:343 +0x47
github.com/sirupsen/logrus.(*Logger).Fatalf(0x4c99ae0, {0x34ca0f1?, 0x1d?}, {0xc000059e28?, 0xc000c1fe30?, 0x130f55f?})
	/pillar/vendor/github.com/sirupsen/logrus/logger.go:191 +0x45
github.com/sirupsen/logrus.Fatalf(...)
	/pillar/vendor/github.com/sirupsen/logrus/exported.go:224
github.com/lf-edge/eve/pkg/pillar/types.(*ConfigItemValueMap).GlobalValueInt(0xc0001f4a08?, {0x34b70be, 0x1d})
	/pillar/types/global.go:769 +0x125
github.com/lf-edge/eve/pkg/pillar/cmd/zedagent.hardwareHealthTimerTask(0xc0001f4a08, 0xc000dc1ab0)
	/pillar/cmd/zedagent/handlemetrics.go:1017 +0x9b
created by github.com/lf-edge/eve/pkg/pillar/cmd/zedagent.Run in goroutine 20
	/pillar/cmd/zedagent/zedagent.go:540 +0x1429

OhmSpectator

The upgrade path is broken, so I am submitting a change request to avoid merging it for now.

christoph-zededa requested review from OhmSpectator, eriknordmark, milan-zededa and rouming as code owners June 26, 2025 16:40

christoph-zededa force-pushed the updateTaskTimer_use_duration branch from 541e3a8 to 1b9c59a Compare June 26, 2025 16:40

github-actions bot requested a review from uncleDecart June 26, 2025 16:40

christoph-zededa added the side-quest A worthy adventure, but not essential for victory. Tackle when the main quest is safe! label Jun 26, 2025

christoph-zededa requested a review from rene June 26, 2025 16:41

milan-zededa reviewed Jun 26, 2025

View reviewed changes

christoph-zededa force-pushed the updateTaskTimer_use_duration branch from 1b9c59a to ce032ad Compare June 27, 2025 11:15

github-actions bot requested a review from milan-zededa June 27, 2025 11:16

christoph-zededa force-pushed the updateTaskTimer_use_duration branch from 23639d2 to cb64fc1 Compare June 27, 2025 11:32

rene approved these changes Jun 30, 2025

View reviewed changes

eriknordmark reviewed Jul 7, 2025

View reviewed changes

christoph-zededa force-pushed the updateTaskTimer_use_duration branch from cb64fc1 to f148e8e Compare July 29, 2025 15:13

github-actions bot requested a review from eriknordmark July 29, 2025 15:14

christoph-zededa force-pushed the updateTaskTimer_use_duration branch 2 times, most recently from 5a337ed to a7b76b5 Compare July 30, 2025 15:31

pillar/updateTaskTimer: switch to less-error prone

ccfcb68

time.Duration Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the updateTaskTimer_use_duration branch from a7b76b5 to ccfcb68 Compare July 30, 2025 15:31

europaul reviewed Jul 31, 2025

View reviewed changes

OhmSpectator suggested changes Jul 31, 2025

View reviewed changes

	logrus.Fatalf("Adding int item %s failed. Value does not meet given min/max criteria", key)
	logrus.Fatalf("Adding duration item %s failed. Value does not meet given min/max criteria", key)

	// AddDurationItem - Adds integer item to specMap
	// AddDurationItem - Adds duration item to specMap

	return fmt.Sprintf("%d", uint64(val.DurationValue.Seconds()))
	return fmt.Sprintf("%v", val.DurationValue)

Update task timer use duration #5023

Are you sure you want to change the base?

Update task timer use duration #5023

Conversation

christoph-zededa commented Jun 26, 2025 • edited by OhmSpectator Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test and validate this PR

Changelog notes

PR Backports

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christoph-zededa commented Jun 27, 2025

Uh oh!

OhmSpectator commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christoph-zededa commented Jun 27, 2025

Uh oh!

christoph-zededa commented Jun 27, 2025

Uh oh!

rene left a comment

Choose a reason for hiding this comment

Uh oh!

eriknordmark commented Jul 7, 2025

Uh oh!

eriknordmark left a comment

Choose a reason for hiding this comment

Uh oh!

OhmSpectator commented Jul 30, 2025

Uh oh!

OhmSpectator commented Jul 31, 2025

Uh oh!

europaul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christoph-zededa commented Jul 31, 2025

Uh oh!

christoph-zededa commented Jul 31, 2025

Uh oh!

europaul commented Jul 31, 2025

Uh oh!

christoph-zededa commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OhmSpectator left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

christoph-zededa commented Jun 26, 2025 •

edited by OhmSpectator

Loading

OhmSpectator commented Jun 27, 2025 •

edited

Loading

christoph-zededa commented Jul 31, 2025 •

edited

Loading