feat: Update controller logic to handle stale SriovNetworkNodeState CRs with delay #798
Conversation
|
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 12122609914Details
💛 - Coveralls |
| // keep until time annotation is not set, set a new value with default or configured offset and update the object | ||
| delayMinutes, err := strconv.Atoi(os.Getenv("STALE_NODE_STATE_CLEANUP_DELAY")) | ||
| if err != nil || delayMinutes <= 0 { | ||
| delayMinutes = 30 // keep objects for 30 minutes by default |
There was a problem hiding this comment.
Let's add some logging here to indicate env variable is incorrect
There was a problem hiding this comment.
nit: we've got unused CleanupTimeout [1] variable. maybe we need to change it's value and use it here
[1]
sriov-network-operator/test/util/util.go
Line 34 in 68b6c02
There was a problem hiding this comment.
I think we should not use this one because it is in "test" package. But I agree that we can create constant with default value.
There was a problem hiding this comment.
I added constant to hold the default value
| @@ -27,6 +27,9 @@ operator: | |||
| resourcePrefix: "openshift.io" | |||
| cniBinPath: "/opt/cni/bin" | |||
| clusterType: "kubernetes" | |||
| # minimal amount of time (in minutes) the operator will wait before removing | |||
| # stale SriovNetworkNodeState objects (objects that doesn't match node with the daemon) | |||
| staleNodeStateCleanupDelay: "30" | |||
There was a problem hiding this comment.
nit: staleNodeStateCleanupDelayMinutes ? or is that too long in your opinion ?
| err := r.Delete(ctx, &ns, &client.DeleteOptions{}) | ||
| if err != nil { | ||
| logger.Error(err, "Fail to Delete", "SriovNetworkNodeState CR:", ns.GetName()) | ||
| if err := r.handleStaleNodeState(ctx, &ns); err != nil { |
There was a problem hiding this comment.
note to self: this is being called every ~5 min (resync period) worst case or when policy is updated or when policy/node changed
|
CI failure is not related to the change. The same failure occurs on the PR with dummy changes #800 |
ykulazhenkov
force-pushed
the
pr-keep-stale-node-state
branch
from
4245aa3
to
13dc502
Compare
|
@e0ne @adrianchiris I addressed your comments. I also changed behavior a bit to completely avoid any delay in case if |
ykulazhenkov
force-pushed
the
pr-keep-stale-node-state
branch
from
13dc502
to
994ddbf
Compare
ykulazhenkov
force-pushed
the
pr-keep-stale-node-state
branch
2 times, most recently
from
1898df7
to
4ea6ce0
Compare
…Rs with delay - Changed the logic in the sriov-network-operator controller to handle stale SriovNetworkNodeState CRs (those with no matching Nodes with daemon). - Introduced a delay (30 minutes by default) before removing stale state CRs to manage scenarios where the user temporarily removes the daemon from the node but does not want to lose the state stored in the SriovNetworkNodeState. - Added the `STALE_NODE_STATE_CLEANUP_DELAY_MINUTES` environment variable to configure the required delay in minutes (default is 30 minutes).
ykulazhenkov
force-pushed
the
pr-keep-stale-node-state
branch
from
4ea6ce0
to
5ad4ae9
Compare
Update controller logic to handle stale SriovNetworkNodeState CRs with delay
STALE_NODE_STATE_CLEANUP_DELAY_MINUTESenvironment variable to configure the required delay in minutes (default is 30 minutes).This functionality especially useful when the OFED container is in use. As the OFED driver loads on the host, the sriov-config-daemon is removed from this node (achieved using configDaemon nodeselector). Since loading the driver can take a considerable amount of time, we want to ensure that the SriovNetworkNodeState is not lost during this process.