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

Remove job configs #145

Merged
merged 6 commits into from
May 28, 2020
Merged

Remove job configs #145

merged 6 commits into from
May 28, 2020

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented May 26, 2020

Summary

Scheduled jobs are no longer able to be disabled. They are still synchronized via the cluster job locks.

Ticket Link

Fixes #144
Fixes #135

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 26, 2020
@mickmister mickmister requested review from levb and larkox May 26, 2020 17:56
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2020

Codecov Report

Merging #145 into master will decrease coverage by 0.80%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   24.20%   23.40%   -0.81%     
==========================================
  Files          67       63       -4     
  Lines        2702     2594     -108     
==========================================
- Hits          654      607      -47     
+ Misses       1964     1909      -55     
+ Partials       84       78       -6     
Impacted Files Coverage Δ
server/jobs/renew_job.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dd13f8...cc8c63f. Read the comment docs.

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mickmister mickmister linked an issue May 27, 2020 that may be closed by this pull request
@levb levb requested a review from DHaussermann May 27, 2020 22:20
@levb levb removed the 2: Dev Review Requires review by a core committer label May 27, 2020
@levb levb requested a review from aaronrothschild May 27, 2020 22:20
@levb levb added the 1: PM Review Requires review by a product manager label May 27, 2020
Comment on lines 67 to 74
func (jm *JobManager) Close() error {
jm.env.Logger.Debugf("Deactivating all jobs due to plugin deactivation.")
jm.activeJobs.Range(func(k interface{}, v interface{}) bool {
job := v.(*activeJob)
err := jm.deactivateJob(job.RegisteredJob)
if err != nil {
jm.env.Logger.Debugf("Failed to deactivate job: %v", err)
jm.env.Logger.Warnf("Failed to deactivate %s job: %v", job.id, err)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lieut-data We are experiencing an issue testing the cluster jobs in HA. We are testing with a fresh cloud server with --size miniHA, which spins up a cluster with two nodes.

I will be referring to the jobs as a singular "job" (cluster.Job), as all the jobs are showing the same behavior. The job activates properly on plugin activation. The successful activation message is logged by both nodes by the jm.activateJob method. The job runs its first execution with no issues. However, the job does not run again when its configured interval comes around.

Disabling the plugin results in the call to job.Close() to hang. Tracing the logs provided by this PR, the Deactivating all jobs due to plugin deactivation message is logged once (due to the node blocking on disable, and not allowing the second node to begin disabling), and does not log the debug message in jm.deactivateJob, nor the warning in this function. Therefore the job.Close() method must be blocking.

I'm not sure where to begin with debugging this. I'd like to debug locally, but I haven't set up an HA environment on my local machine. How do you advise moving forward with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this behavior was occurring before this PR. Part of the reason for this PR is to simplify the logic, to minimize the surface area for errors like the ones described above to occur.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For anyone following this thread, see https://community-daily.mattermost.com/core/pl/auyr1cd1q7nu9ytmtb9a5fod8a for the resolution.)

@mickmister mickmister linked an issue May 28, 2020 that may be closed by this pull request
@DHaussermann
Copy link

Tested and passed

  • Tested this on an HA server
  • Jobs are working as expected
  • No locks found when disabling plugin
    LGTM!

@mickmister mickmister merged commit b549fad into master May 28, 2020
@mickmister mickmister deleted the remove-job-configs branch May 28, 2020 22:04
@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label May 28, 2020
ayusht2810 pushed a commit that referenced this pull request Feb 16, 2024
#192)

* Revert "Update main.go (#154)"

This reverts commit be4a281d0cc791d10e6e5ae917b325b2f054e475.

* Revert "[MM-33506] Use embed package to include plugin manifest (#145)"

This reverts commit ca9ee3c17c6920a636a33f378e17395afd6f329f.

* Revert "Don't generate manifest.ts (#127)"

This reverts commit 18d30b50bc7ba800c9f05bfd82970781db0aea3e.

* install-go-tools target, adopt gotestsum

* bring back make apply + automatic versioning

* Update build/manifest/main.go

Co-authored-by: Michael Kochell <[email protected]>

* suppress git describe error when no tags match

* make version/release notes opt-in

* fix whitespace in Makefile

* document version management options

---------

Co-authored-by: Michael Kochell <[email protected]>
wiggin77 pushed a commit that referenced this pull request Jul 4, 2024
* Sync with playbooks: install-go-tools, gotestsum, and dynamic versions (#192)

* Revert "Update main.go (#154)"

This reverts commit be4a281d0cc791d10e6e5ae917b325b2f054e475.

* Revert "[MM-33506] Use embed package to include plugin manifest (#145)"

This reverts commit ca9ee3c17c6920a636a33f378e17395afd6f329f.

* Revert "Don't generate manifest.ts (#127)"

This reverts commit 18d30b50bc7ba800c9f05bfd82970781db0aea3e.

* install-go-tools target, adopt gotestsum

* bring back make apply + automatic versioning

* Update build/manifest/main.go

Co-authored-by: Michael Kochell <[email protected]>

* suppress git describe error when no tags match

* make version/release notes opt-in

* fix whitespace in Makefile

* document version management options

---------

Co-authored-by: Michael Kochell <[email protected]>

* Fetch plugin logs from server (#193)

Co-authored-by: Jesse Hallam <[email protected]>

* [MM-231]: Fixed merge conflicts

* [MM-231] Fix flaky tests

---------

Co-authored-by: Jesse Hallam <[email protected]>
Co-authored-by: Michael Kochell <[email protected]>
Co-authored-by: Ben Schumacher <[email protected]>
Co-authored-by: kshitij <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1: PM Review Requires review by a product manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove scheduled job config options Implement verbose logging for HA jobs
6 participants