prevent double job status update; periodic job count miscalculation #9768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drewbailey
requested review from
tgross and
notnoop
and removed request for
tgross
drewbailey
force-pushed
the
b/prevent-double-job-status-update
branch
from
2e5b57a to
1c37af8
Compare
tgross
requested changes
Jan 11, 2021
| require.NotEmpty(t, childID) | ||
|
|
||
| testutil.WaitForResult(func() (bool, error) { | ||
| status, err := e2eutil.JobInspectTemplate(jobID, `{{with index . 1}}{{printf "%s" .Status}}{{end}}`) |
tgross
approved these changes
Jan 11, 2021
drewbailey
changed the title
notnoop
approved these changes
Jan 12, 2021
drewbailey
force-pushed
the
b/prevent-double-job-status-update
branch
from
93a65ef to
74030f7
Compare
|
After the initial review, there were multiple state_store specs failing. Removing the This fixed most the failing tests, but |
drewbailey
force-pushed
the
b/prevent-double-job-status-update
branch
from
74030f7 to
a4405a9
Compare
shoenig
approved these changes
Jan 19, 2021
drewbailey
commented
Jan 21, 2021
| }) | ||
|
|
||
| t.Run("no listeners", func(t *testing.T) { | ||
| result := gatewayBindAddresses(&structs.ConsulIngressConfigEntry{Listeners: nil}) | ||
| require.Nil(t, result) | ||
| require.Empty(t, result) |
There was a problem hiding this comment.
@shoenig requested re-review, this commit seems to deviate from how a bit of the consul gateway empty values are handled
shoenig
reviewed
Jan 22, 2021
shoenig
approved these changes
Jan 22, 2021
#8435 introduced atomic eval insertion iwth job (de-)registration. This change removes a now obsolete guard which checked if the index was equal to the job.CreateIndex, which would empty the status. Now that the job regisration eval insetion is atomic with the registration this check is no longer necessary to set the job statuses correctly.
fix nil assertions for empty map rm unnecessary guard
drewbailey
force-pushed
the
b/prevent-double-job-status-update
branch
from
b1c5848 to
f35eee3
Compare
drewbailey
changed the title
backspace
pushed a commit
that referenced
this pull request
Jan 22, 2021
* Prevent Job Statuses from being calculated twice #8435 introduced atomic eval insertion iwth job (de-)registration. This change removes a now obsolete guard which checked if the index was equal to the job.CreateIndex, which would empty the status. Now that the job regisration eval insetion is atomic with the registration this check is no longer necessary to set the job statuses correctly. * test to ensure only single job event for job register * periodic e2e * separate job update summary step * fix updatejobstability to use copy instead of modified reference of job * update envoygatewaybindaddresses copy to prevent job diff on null vs empty * set ConsulGatewayBindAddress to empty map instead of nil fix nil assertions for empty map rm unnecessary guard
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#8435 introduced atomic eval insertion when registering/de-registering jobs.
This change removes a now obsolete guard which checked if the index was
equal to the job.CreateIndex, which would empty the status.
Now that the job regisration eval insertion is atomic with the registration
this check is no longer necessary to set the job statuses correctly
By preventing the double job status calculation, we also need to ensure that job summary is properly calculated. This ensures the job summary is calculated correctly, even if the job status does not change.
fixes #8692