deploy: ApplyParams: do not write params.env if there are no updates #1186
Conversation
pkg/deploy/envParams.go
Outdated
| } | ||
| } | ||
|
|
||
| if count == 0 { |
There was a problem hiding this comment.
need a count?
or just a true/false enough?
if !needUpdateImage && !needUpdateEnv {
return nil
}
There was a problem hiding this comment.
I do not know a way to accumulate booleans. With every individual check it looks pretty clumsy. But actually I missed for some reason that bitwise operations work in go, so will replace += with |= to be safe from overflow :) (practically impossible here)
There was a problem hiding this comment.
but to call writeParamsToTmp() or not, this is based on if any key-value from passing-in map does not match the key-value from params.env, isnt it? regardless all of them mismatch or just one of them mismatch.
as for writeParamsToTmp(), it is a full dump all key-values into the temp. file
so i still do not get the reason behind why need count them.
if you have a first mistmatch in the updateMap, mark needUpdateImage/needUpdateEnv to true, isnt that enough? something like
func updateMap(m *map[string]string, key, val string) int {
old := (*m)[key]
if old != val {
(*m)[key] = val
needUpdateImage = true
}
}
There was a problem hiding this comment.
It should be a package scope variable then, which is not nice as well. But if you prefer that, I'll change that way
There was a problem hiding this comment.
And negated condition :)
There was a problem hiding this comment.
so i still do not get the reason behind why need count them.
There is no need to really count, it acts as boolean actually just to accumulate results (a bit like multierror does for error) since I cannot do var += true and var += false which is true if one of that was true
UPD: forgot to commit actual changes, ykaliuta@4e93635
There was a problem hiding this comment.
It should be a package scope variable then, which is not nice as well. But if you prefer that, I'll change that way
Actually, it's more serious issue. Since it addresses the possible race, package scoped variable requires synchronized access then (mutex). Basically, setting it "true" from two invocations does not make real harm with the implementation, since tmpfile name is unique, update of boolean is atomic and params.env file update with os.Rename() is atomic as well, but it's not nice too.
There was a problem hiding this comment.
Actually, it's more serious issue. Since it addresses the possible race, package scoped variable requires
synchronized access then (mutex). Basically, setting it "true" from two invocations does not make real harm with the implementation, since tmpfile name is unique, update of boolean is atomic and params.env file update with os.Rename() is atomic as well, but it's not nice too.
Forget what I said. It's an issue since there are 2 places of writes: before the loop and when a field gets updated. They may interlace and previously set "true" be reset with next invocation "false".
There was a problem hiding this comment.
In general, these params.env corrections look like one time initialization operation and should not be called for every reconciliation.
There was a problem hiding this comment.
In general, these params.env corrections look like one time initialization operation and should not be called for every reconciliation.
#1191 like that, what do you think?
ykaliuta
force-pushed
the
apply-params-save-writes
branch
4 times, most recently
from
72b3bd7
to
8bdc195
Compare
Optimize file operations, do not rewrite params.env on every reconcile if no updates for it. In practice updates happen only on the first run. Since updates happen in 2 loops, move actual update to own function which returns "number of updates" (actually 0 or 1, so acts as boolean) and use bitwise operations to accumulate updates for every field. It could be possible to use global boolean flag, but since it requires to be initiated before the loops and set when updates happen, the function should be guarded with mutex. Otherwise "true" value from one invocation can be reset to "false" by the next invocation". Signed-off-by: Yauheni Kaliuta <[email protected]>
ykaliuta
force-pushed
the
apply-params-save-writes
branch
from
8bdc195
to
b5a6168
Compare
zdtsw
requested review from
VaishnaviHire
and removed request for
jackdelahunt
|
@ykaliuta @VaishnaviHire lets try sort this PR out tomorrow/this week. the check part will somehow help preventing wrong image being used. |
With pleasure :) Anything required from my side? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
i am fine with this change, i thought it got merged weeks ago, but when i checked code yesterday found the PR was still open. |
openshift-merge-bot
bot
merged commit 450e19f
into
opendatahub-io:incubation
…pendatahub-io#1186) Optimize file operations, do not rewrite params.env on every reconcile if no updates for it. In practice updates happen only on the first run. Since updates happen in 2 loops, move actual update to own function which returns "number of updates" (actually 0 or 1, so acts as boolean) and use bitwise operations to accumulate updates for every field. It could be possible to use global boolean flag, but since it requires to be initiated before the loops and set when updates happen, the function should be guarded with mutex. Otherwise "true" value from one invocation can be reset to "false" by the next invocation". Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 450e19f)
…pendatahub-io#1186) Optimize file operations, do not rewrite params.env on every reconcile if no updates for it. In practice updates happen only on the first run. Since updates happen in 2 loops, move actual update to own function which returns "number of updates" (actually 0 or 1, so acts as boolean) and use bitwise operations to accumulate updates for every field. It could be possible to use global boolean flag, but since it requires to be initiated before the loops and set when updates happen, the function should be guarded with mutex. Otherwise "true" value from one invocation can be reset to "false" by the next invocation". Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 450e19f)
Optimize file operations, do not rewrite params.env on every reconcile if no updates for it. In practice updates happen only on the first run.
Jira: https://issues.redhat.com/browse/RHOAIENG-11594
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria