-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Mock driver plugin implementation and driver registration #4777
Conversation
ae35b59
to
60433ad
Compare
plugins/shared/catalog/testing.go
Outdated
@@ -1,3 +1,5 @@ | |||
// +build !release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. It shouldn't be imported by anything in the build, only tests. None of our other testing files do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started adding it because I did accidently import a testing file into release! Happy to remove though: it's a small thing.
nomad/testing.go
Outdated
@@ -1,3 +1,5 @@ | |||
// +build !release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
} | ||
) | ||
|
||
func PluginLoader(opts map[string]string) (map[string]interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a comment on all three of these public fields
select { | ||
case <-h.waitCh: | ||
d.logger.Debug("not killing task: already exited", "task_name", h.task.Name) | ||
case <-time.After(h.killAfter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense. It is waiting on unrelated timeouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right? I just ported the existing code and it confused me too: https://github.com/hashicorp/nomad/blob/master/client/driver/mock_driver.go#L380-L402
Should I do a grep and switch everything to kill_timeout
and remove kill_after
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickethier pointed out killTimeout isn't used. Removing that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I see. If not set the kill will happen immediately. So it is just a way to introduce a kill delay.
|
||
import "github.com/hashicorp/nomad/drivers/mock" | ||
|
||
func init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment whats going on here
drivers/mock/handle.go
Outdated
|
||
runFor time.Duration | ||
killAfter time.Duration | ||
killTimeout time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
killTimeout is not used and can be removed I think
drivers/mock/driver.go
Outdated
} | ||
|
||
func (d *Driver) DestroyTask(taskID string, force bool) error { | ||
//TODO is there anything else to do here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if handle exists, if state == running and !force return error.
If handle does not exist return drivers.ErrTaskNotFound
drivers/mock/driver.go
Outdated
} | ||
|
||
func (d *Driver) TaskEvents(netctx.Context) (<-chan *drivers.TaskEvent, error) { | ||
panic("not implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an eventer like other plugins and plumb it through here.
Do not register mock_driver on release builds.
from #4777 comments
60433ad
to
9bc211f
Compare
select { | ||
case <-h.waitCh: | ||
d.logger.Debug("not killing task: already exited", "task_name", h.task.Name) | ||
case <-time.After(h.killAfter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I see. If not set the kill will happen immediately. So it is just a way to introduce a kill delay.
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. |
Started by @nickethier - finished by @schmichael
This implements the mock driver as a plugin. All plugins are registered by default, but the mock driver is not registered for release builds.
I created files specifically for registering plugins to hopefully ease management of build tags (as we have a lot of platform specific drivers).
Why not use side-effect imports?
Before this PR drivers registered themselves in init methods and the
agent
package imported all drivers to register them.This PR moves registrations into the
catalog
package and references static variables within the driver packages.Pros of registration in
catalog
package (new approach)agent
package would have to manually register the plugins they needed.Cons of registration in
catalog
packagecatalog
package. Not an issue today, and hopefully never an issue in the future.